Repository navigation
airflowctl: action_on_existence flag returns empty success & errors in variables import #51310
Description
Activity
Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.
- changed the title
[-]airflowctl: action_on_existence flag returns empty success & errors arrays in variables import[/-][+]airflow: action_on_existence flag returns empty success & errors in `variables import`[/+]on Jun 2, 2025 - changed the title
[-]airflow: action_on_existence flag returns empty success & errors in `variables import`[/-][+]airflowctl: action_on_existence flag returns empty success & errors in `variables import`[/+]on Jun 2, 2025 I think testing with Postman and seeing if it is working for all bulk operations should be enough. Thanks for creating it! I assigned to you
Reacted by jj.leeReacted by jj.lee@bugraoz93 I found potential problem!
Airflow REST API documentation indicates that bulk variables endpoint returns a BulkResponse. However, bulk method in VariableOperation within airflowctl currently returns BulkActionResponse. This discrepancy could lead to inconsistencies in handling API responses.
REST API response bulk variables.
BulkResponse response scheme
This diff might also exist in
PoolOperation's bulk method.
Should we consider updating these methods to returnBulkResponseto align with the API spec? If you can provide guidance or direction, I'd be happy to create a PR to address this.If we'll update methods to return BulkResponse, how about implementing it as follows?
screencast.2025-06-07.21-53-43.mp4
If we'll update methods to return BulkResponse, how about implementing it as follows?
screencast.2025-06-07.21-53-43.mp4
Sure, great catch @jx2lee! Thanks! Feel free to create a PR. We can indeed use the same model in airflowctl and fix this one. It would be easier to discuss over the code :) You did the change in operations right?
Reacted by jj.leeYou did the change in operations right?
@bugraoz93 Sure. I did change in operation, to BulkResponse. Once the output format is defined or guide is provided, I can quickly make it.
Below pseudo code.diff --git a/airflow-ctl/src/airflowctl/api/operations.py b/airflow-ctl/src/airflowctl/api/operations.py index 00f7601348..485d157ad3 100644 --- a/airflow-ctl/src/airflowctl/api/operations.py +++ b/airflow-ctl/src/airflowctl/api/operations.py @@ -33,6 +33,7 @@ from airflowctl.api.datamodels.generated import ( BackfillCollectionResponse, BackfillPostBody, BackfillResponse, + BulkResponse, BulkActionResponse, BulkBodyConnectionBody, BulkBodyPoolBody, @@ -676,7 +677,7 @@ class VariablesOperations(BaseOperations): """CRUD multiple variables.""" try: self.response = self.client.patch("variables", json=variables.model_dump()) - return BulkActionResponse.model_validate_json(self.response.content) + return BulkResponse.model_validate_json(self.response.content) except ServerResponseError as e: raise ediff --git a/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py b/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py index 1c31828a37..b69c3ae0ab 100644 --- a/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py +++ b/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py @@ -35,7 +35,8 @@ from airflowctl.api.datamodels.generated import ( @provide_api_client(kind=ClientKind.CLI) def import_(args, api_client=NEW_API_CLIENT): """Import variables from a given file.""" - success_message = "[green]Import successful! success: {success}, errors: {errors}[/green]" + success_message = "[green]Import successful! success: {success}[/green]" + errors_message = "[red]But errors occured, errors: {errors}[/red]" if not os.path.exists(args.file): rich.print(f"[red]Missing variable file: {args.file}") sys.exit(1) @@ -71,8 +72,10 @@ def import_(args, api_client=NEW_API_CLIENT): ] ) result = api_client.variables.bulk(variables=bulk_body) - rich.print(success_message.format(success=result.success, errors=result.errors)) - return result.success, result.errors + rich.print(success_message.format(success=result.create.success)) + if result.create.errors: + rich.print(errors_message.format(errors=result.create.errors)) + return result.create.success, result.create.errorsYou did the change in operations right?
@bugraoz93 Sure. I did change in operation, to BulkResponse. Once the output format is defined or guide is provided, I can quickly make it.
Below pseudo code.diff --git a/airflow-ctl/src/airflowctl/api/operations.py b/airflow-ctl/src/airflowctl/api/operations.py index 00f7601348..485d157ad3 100644 --- a/airflow-ctl/src/airflowctl/api/operations.py +++ b/airflow-ctl/src/airflowctl/api/operations.py @@ -33,6 +33,7 @@ from airflowctl.api.datamodels.generated import ( BackfillCollectionResponse, BackfillPostBody, BackfillResponse, + BulkResponse, BulkActionResponse, BulkBodyConnectionBody, BulkBodyPoolBody, @@ -676,7 +677,7 @@ class VariablesOperations(BaseOperations): """CRUD multiple variables.""" try: self.response = self.client.patch("variables", json=variables.model_dump()) - return BulkActionResponse.model_validate_json(self.response.content) + return BulkResponse.model_validate_json(self.response.content) except ServerResponseError as e: raise ediff --git a/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py b/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py index 1c31828a37..b69c3ae0ab 100644 --- a/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py +++ b/airflow-ctl/src/airflowctl/ctl/commands/variable_command.py @@ -35,7 +35,8 @@ from airflowctl.api.datamodels.generated import ( @provide_api_client(kind=ClientKind.CLI) def import_(args, api_client=NEW_API_CLIENT): """Import variables from a given file.""" - success_message = "[green]Import successful! success: {success}, errors: {errors}[/green]" + success_message = "[green]Import successful! success: {success}[/green]" + errors_message = "[red]But errors occured, errors: {errors}[/red]" if not os.path.exists(args.file): rich.print(f"[red]Missing variable file: {args.file}") sys.exit(1) @@ -71,8 +72,10 @@ def import_(args, api_client=NEW_API_CLIENT): ] ) result = api_client.variables.bulk(variables=bulk_body) - rich.print(success_message.format(success=result.success, errors=result.errors)) - return result.success, result.errors + rich.print(success_message.format(success=result.create.success)) + if result.create.errors: + rich.print(errors_message.format(errors=result.create.errors)) + return result.create.success, result.create.errorsWe always send create action, so it might be good to only print success and errors rather than for all actions. We can bring those functionalities in future releases such as in place updates. It doesn't need to be particularly in the current version
Reacted by jj.leeWe always send create action, so it might be good to only print success and errors rather than for all actions. We can bring those functionalities in future releases such as in place updates. It doesn't need to be particularly in the current version
Thanks for sharing your view! I agreed, make PR soon. 👍🏽
Apache Airflow version
3.0.1
If "Other Airflow 2 version" selected, which one?
No response
What happened?
related: #50908
details: #50908 (comment)
cc. @bugraoz93 (comment)
What you think should happen instead?
With --action_on_existence fail, request should return 200 OK with right results.errors / results.success. But, empty
How to reproduce
When importing variables with
--action_on_existenceflag set to fail, overwrite, or skip,the API responds
200 OK, but bothresults.successandresults.errorsare empty.This makes it impossible to determine whether the existing variables were handled as expected.
Operating System
osx
Versions of Apache Airflow Providers
No response
Deployment
Other
Deployment details
No response
Anything else?
Reproduction Steps
[ {"key": "foo", "value": "1"}, {"key": "bar", "value": "2"} ][ {"key": "foo", "value": "99"}, {"key": "bar", "value": "88"} ]You'll found empty success/errors.
Are you willing to submit PR?
Code of Conduct