You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "yunchipang (via GitHub)" <gi...@apache.org> on 2025/04/23 06:08:35 UTC
[PR] [WIP] Transition of config command [airflow]
yunchipang opened a new pull request, #49601:
URL: https://github.com/apache/airflow/pull/49601
closes: #45664
related: #45661
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1/4] Transition of get-value config command [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #49601:
URL: https://github.com/apache/airflow/pull/49601#issuecomment-2844220135
> @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).
> <img width="855" alt="image" src="https://github.com/user-attachments/assets/0f8f5607-6fe2-4b5d-a435-2e0b2d6833f9" />
>
> 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.
> <img width="433" alt="image" src="https://github.com/user-attachments/assets/7d87eb8a-fb19-422e-be4c-683249d7e68a" />
>
> 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
https://github.com/apache/airflow/blob/879b6810ba496d7471761a3b6e5b2c5b953b358e/airflow-ctl/src/airflowctl/ctl/cli_config.py#L212
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Transition of config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #49601:
URL: https://github.com/apache/airflow/pull/49601#discussion_r2059434505
##########
airflow-ctl/tests/airflow_ctl/ctl/commands/test_config_command.py:
##########
@@ -0,0 +1,78 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import contextlib
+from io import StringIO
+
+from airflowctl.api.client import ClientKind
+from airflowctl.ctl import cli_parser
+from airflowctl.ctl.commands import config_command
+
+
+class TestCliConfigGetValue:
+ @classmethod
+ def setup_class(cls):
+ cls.parser = cli_parser.get_parser()
+
+ def test_should_display_value(self, api_client_maker):
+ api_client = api_client_maker(
+ path="/api/v2/config/section/core/option/test_key",
+ # sample response: sections=[ConfigSection(name='core', options=[ConfigOption(key='dags_folder', value='/files/dags')])]
+ response_json={
+ "sections": [{"name": "core", "options": [{"key": "test_key", "value": "test_value"}]}]
+ },
+ expected_http_status_code=200,
+ kind=ClientKind.CLI,
+ )
+
+ args = self.parser.parse_args(["config", "get-value", "--section", "core", "--option", "test_key"])
+
+ with contextlib.redirect_stdout(StringIO()) as temp_stdout:
+ config_command.get_value(args, api_client=api_client)
+
+ assert temp_stdout.getvalue().strip() == "test_value"
+
+ def test_should_not_raise_exception_when_section_for_config_with_value_defined_elsewhere_is_missing(
Review Comment:
@bugraoz93 I was copying the logic in `airflow-core/tests/unit/cli/commands/test_config_command.py`, however I was not quite understanding it. Especially the difference between `test_should_not_raise_exception_when_section_for_config_with_value_defined_elsewhere_is_missing` and `test_should_raise_exception_when_option_is_missing`. The API client raises 404 for both cases, then why are they handled differently? Could you kindly clarify? Thanks a lot!
The test class passes for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1/4] Transition of get-value config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #49601:
URL: https://github.com/apache/airflow/pull/49601#issuecomment-2848780100
@bugraoz93 Thanks so much for the clarification! Just making sure I fully understand — since `get` is already handled by auto-gen, I should only focus on implementing `list`, `lint` and `update`, correct?
Also, when running `airflowctl config -h`, I should expect to see four commands: `get`, `list`, `lint`, and `update`, meaning `get-value` will no longer appear as an option.
Please let me know if I’ve misunderstood anything!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Transition of config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #49601:
URL: https://github.com/apache/airflow/pull/49601#issuecomment-2826259682
@bugraoz93 awesome. feel free to ping me when this pr is unblocked. i'll continue with the rest transition. thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1/4] Transition of get-value config command [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #49601:
URL: https://github.com/apache/airflow/pull/49601#issuecomment-2848787579
> @bugraoz93 Thanks so much for the clarification! Just making sure I fully understand — since `get` is already handled by auto-gen, I should only focus on implementing `list`, `lint` and `update`, correct?
>
> Also, when running `airflowctl config -h`, I should expect to see four commands: `get`, `list`, `lint`, and `update`, meaning `get-value` will no longer appear as an option.
>
> Please let me know if I’ve misunderstood anything!
@yunchipang Yes, you should focus on `list`, `lint` and `update`. It is okay to have some duplicate code, we are expecting a bit of code duplication with `Airflow CLI config command` in ([airflow-core/src/airflow/cli/commands/config_command.py](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/cli/commands/config_command.py)) at the beginning until we deprecate and move the same behaviours from `Airflow CLI`.
At the end of all transitions, you should expect to see `get`, `list`, `lint`, and `update` at the end of `airflowctl config -h`. We don't necessarily need to follow the same command naming as `Airflow CLI`, it is already a new tool, the important thing is consistency between all commands. For example `get command` exists in `airflowctl connections`, `airflowctl pools` or `airflowctl variables`, etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Transition of config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #49601:
URL: https://github.com/apache/airflow/pull/49601#issuecomment-2843776282
@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).
<img width="855" alt="image" src="https://github.com/user-attachments/assets/0f8f5607-6fe2-4b5d-a435-2e0b2d6833f9" />
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.
<img width="433" alt="image" src="https://github.com/user-attachments/assets/7d87eb8a-fb19-422e-be4c-683249d7e68a" />
thanks a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [1/4] Transition of `get` config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang closed pull request #49601: [1/4] Transition of `get` config command
URL: https://github.com/apache/airflow/pull/49601
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Transition of config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #49601:
URL: https://github.com/apache/airflow/pull/49601#issuecomment-2823181359
@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?
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Transition of config command [airflow]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #49601:
URL: https://github.com/apache/airflow/pull/49601#discussion_r2059434505
##########
airflow-ctl/tests/airflow_ctl/ctl/commands/test_config_command.py:
##########
@@ -0,0 +1,78 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import contextlib
+from io import StringIO
+
+from airflowctl.api.client import ClientKind
+from airflowctl.ctl import cli_parser
+from airflowctl.ctl.commands import config_command
+
+
+class TestCliConfigGetValue:
+ @classmethod
+ def setup_class(cls):
+ cls.parser = cli_parser.get_parser()
+
+ def test_should_display_value(self, api_client_maker):
+ api_client = api_client_maker(
+ path="/api/v2/config/section/core/option/test_key",
+ # sample response: sections=[ConfigSection(name='core', options=[ConfigOption(key='dags_folder', value='/files/dags')])]
+ response_json={
+ "sections": [{"name": "core", "options": [{"key": "test_key", "value": "test_value"}]}]
+ },
+ expected_http_status_code=200,
+ kind=ClientKind.CLI,
+ )
+
+ args = self.parser.parse_args(["config", "get-value", "--section", "core", "--option", "test_key"])
+
+ with contextlib.redirect_stdout(StringIO()) as temp_stdout:
+ config_command.get_value(args, api_client=api_client)
+
+ assert temp_stdout.getvalue().strip() == "test_value"
+
+ def test_should_not_raise_exception_when_section_for_config_with_value_defined_elsewhere_is_missing(
Review Comment:
@bugraoz93 I was copying the logic in `airflow-core/tests/unit/cli/commands/test_config_command.py`, however I was not quite understanding it. Especially the difference between `test_should_not_raise_exception_when_section_for_config_with_value_defined_elsewhere_is_missing` and `test_should_raise_exception_when_option_is_missing`. The API client raises 404 for both cases, then why are they handled differently? Could you kindly clarify? Thanks a lot!
The test class passes for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org