Skip to content

[1/4] Transition of get-value config command #49601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yunchipang
Copy link
Contributor

@yunchipang yunchipang commented Apr 23, 2025

partially closes: #45664
parent issue: #45661

implements get-value config command. unit tests are added.

@yunchipang
Copy link
Contributor Author

yunchipang commented Apr 23, 2025

@bugraoz93 I think I’m starting to get a good grasp of how the transitioning works. However, I’m currently hitting this error:

root@a93cfff4a88c:/opt/airflow# airflowctl config get-value --section core --option dags_folder
Usage: airflowctl config [-h] COMMAND ...

Perform Config operations

Positional Arguments:
  COMMAND
    get       Perform get operation

Optional Arguments:
  -h, --help  show this help message and exit

airflowctl config command error: argument COMMAND: invalid choice: 'get-value' (choose from 'get'), see help above.

It seems like my get-value config ActionCommand isn’t being successfully registered in cli_config.py, and I haven’t been able to figure out why. Could you take a quick look and see if there’s something I might’ve missed?

Note: This PR is still in draft—only get_value has been migrated so far. I’ll complete the rest once I get past this blocker 😆

Thanks a lot!

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 for the PR!

Basic commands are already generated automatically via 1-1 operation-command mapping.
Operations
https://github.com/apache/airflow/blob/main/airflow-ctl/src/airflowctl/api/operations.py
Auto Generation


If you run airflowctl --help or more specifically airflowctl config --help. For example, in this case, all methods you added should stay and get-value shouldn't be there because get comes from the generated part.

@bugraoz93 I think I’m starting to get a good grasp of how the transitioning works. However, I’m currently hitting this error:

airflowctl config command error: argument COMMAND: invalid choice: 'get-value' (choose from 'get'), see help above.
It seems like my get-value config ActionCommand isn’t being successfully registered in cli_config.py, and I haven’t been able to figure out why. Could you take a quick look and see if there’s something I might’ve missed?

I think one overrides the other since both GroupCommand have the same name. This gives precedence to one.
I have created a PR for this. When it is merged, you will be safely included commands and test it.
#49674

@yunchipang yunchipang force-pushed the transition-of-config-command branch from b2fa0f8 to 78da866 Compare April 24, 2025 16:28
@yunchipang yunchipang force-pushed the transition-of-config-command branch from 3849c89 to b3c280e Compare April 30, 2025 23:14
@yunchipang yunchipang marked this pull request as ready for review May 1, 2025 00:08
@yunchipang yunchipang requested review from kaxil and potiuk as code owners May 1, 2025 00:08
@yunchipang yunchipang requested a review from bugraoz93 May 1, 2025 00:13
@yunchipang
Copy link
Contributor Author

yunchipang commented May 1, 2025

@bugraoz93 I wanted to limit this PR to only transitions the get-value config command, as this might get too large and hard to review if all 4 commands (get-value, list, lint, update) are implemented. Unit tests are added, please take a look and let me know your thoughts.

I have one concern here, in the old airflow CLI, get is not displayed as a command (see below).
image

However, with the new auto-gen of commands, get will be listed as one of the available commands when I use airflowctl config -h. I am not sure is this is intended, or we should find a way to undo that.
image

note: trying to fix CI error!

@yunchipang yunchipang changed the title [WIP] Transition of config command [1/4] Transition of get-value config command May 1, 2025
@bugraoz93
Copy link
Contributor

@bugraoz93 I wanted to limit this PR to only transitions the get-value config command, as this might get too large and hard to review if all 4 commands (get-value, list, lint, update) are implemented. Unit tests are added, please take a look and let me know your thoughts.

I have one concern here, in the old airflow CLI, get is not displayed as a command (see below).
image

However, with the new auto-gen of commands, get will be listed as one of the available commands when I use airflowctl config -h. I am not sure is this is intended, or we should find a way to undo that.
image

note: trying to fix CI error!

Hey @yunchipang, it is okay to do it step by step and command by com and. For get-value, as I mentioned in my previous comment.

Basic commands are already generated automatically via 1-1 operation-command mapping.
Operations
https://github.com/apache/airflow/blob/main/airflow-ctl/src/airflowctl/api/operations.py
Auto Generation


If you run airflowctl --help or more specifically airflowctl config --help. For example, in this case, all methods you added should stay and get-value shouldn't be there because get comes from the generated part.

This is expected, and we shouldn't implement anything that comes from generated commands from operations. This is not a problem but intended behavior. They will serve to the same purpose and do the same thing. So please skip get-value. Only focus on the capability other than generated ones.

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 Config Command
2 participants