Skip to content

Conversation

@jx2lee
Copy link
Contributor

@jx2lee jx2lee commented May 21, 2025

closes: #45669

Known Issues

action_on_existence Not Applied in Bulk Requests:

  • when performing bulk variable imports, action_on_existence parameter does not behave as expected.
  • Specifically, importing variables that already exist in the system does not trigger the appropriate error handling or overwrite logic, depending on the specified action.
  • This results in ambiguous outcomes, as the response lacks clear indicators of success or failure for individual items.

To address this, a separate issue has been created: #TO_BE_CONTINUED.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@jx2lee
Copy link
Contributor Author

jx2lee commented May 24, 2025

@bugraoz93
I have tested the import/export functionality, and overall, it works well.
However, I’ve encountered issue when using the action_on_existence parameter during import. Specifically, when I set it fail, overwrite, or skip, the behavior doesn’t align with expectations.

screencast.2025-05-24.16-47-53.mp4
  • situation: existed variables, contained existed variables in json(to upload), import with "fail"
  • expected: result.errors not empty
  • actual: results.errors empty

In particular, the bulk response returns a 200 status code, but both result.success and result.errors are empty. This makes it challenging to determine if the operation was successful or if there were any issues.

Could you please advise on the following:

  • Is the bulk request command functioning as intended?
  • Is there a potential issue in the current code?
  • Are there specific areas in the codebase I should examine to understand this behavior better?

Any guidance would be greatly appreciated! 🙏🏽
(Once above issue is resolved, I'll write unit tests.)

@jx2lee jx2lee force-pushed the airflowctl-variable branch from 2f0cb0b to 0ac3cbb Compare May 25, 2025 13:11
@bugraoz93
Copy link
Contributor

Thanks for the progress @jx2lee! If something is wrong, we should fix it on the API end. Don't need to be in the scope of this PR for sure. I am going to check your questions in detail soon and will address them.

@jx2lee jx2lee force-pushed the airflowctl-variable branch 3 times, most recently from e9ccc78 to 336ff23 Compare May 26, 2025 15:03
@jx2lee
Copy link
Contributor Author

jx2lee commented May 26, 2025

@bugraoz93
Thank you for your response and support!
As you mentioned, it's appropriate to address above issue outside scope of current PR. I'll create a separate issue and link it accordingly.

Additionally, I've confirmed that the import/export commands are functioning correctly.
Summary:

  • I'll investigate the issue separately and create a dedicated issue with a link.
  • Confirmed that the import/export commands are working as expected.

@jx2lee jx2lee force-pushed the airflowctl-variable branch from 336ff23 to 1de0607 Compare May 26, 2025 15:42
@jx2lee jx2lee marked this pull request as ready for review May 26, 2025 15:42
@jx2lee jx2lee requested review from bugraoz93, kaxil and potiuk as code owners May 26, 2025 15:42
@bugraoz93
Copy link
Contributor

@bugraoz93 Thank you for your response and support! As you mentioned, it's appropriate to address above issue outside scope of current PR. I'll create a separate issue and link it accordingly.

Additionally, I've confirmed that the import/export commands are functioning correctly. Summary:

  • I'll investigate the issue separately and create a dedicated issue with a link.
  • Confirmed that the import/export commands are working as expected.

Make sense! That is the way to go. 🚀 Please cc me so I can follow up too :)

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Great work!

@jx2lee
Copy link
Contributor Author

jx2lee commented May 27, 2025

@bugraoz93 Thanks! I'll make issue, soon 👍🏽

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change required after a change on the API side. Could you please adopt?

@jx2lee
Copy link
Contributor Author

jx2lee commented May 28, 2025

@bugraoz93 Sure, I'll adopt tonight.

@jx2lee jx2lee requested a review from bugraoz93 May 28, 2025 11:53
@jx2lee jx2lee force-pushed the airflowctl-variable branch from 57dbf33 to 877b018 Compare May 28, 2025 12:03
@bugraoz93
Copy link
Contributor

Thanks, @jx2lee!

@bugraoz93 bugraoz93 merged commit 9bc32d9 into apache:main May 28, 2025
52 checks passed
sanederchik pushed a commit to sanederchik/airflow that referenced this pull request Jun 7, 2025
* init import/export in airflowctl variables

* BulkCreateActionVariableBody.action to str
jose-lehmkuhl pushed a commit to jose-lehmkuhl/airflow that referenced this pull request Jul 11, 2025
* init import/export in airflowctl variables

* BulkCreateActionVariableBody.action to str
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AIP-81 Transition of Variable Command

2 participants