-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Respect apps flags for api_server_command
#52929
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
Respect apps flags for api_server_command
#52929
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures the --apps flag is respected outside of dev mode and that the AIRFLOW_API_APPS environment variable is always restored to its original value.
- Introduces a
with_api_apps_envdecorator to manage setting/tearing down the env var - Refactors
api_serverto use the decorator and removes inline env manipulation - Updates
test_api_server_command.pyto parameterize scenarios for dev/non-dev modes and original env states
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| airflow-core/tests/unit/cli/commands/test_api_server_command.py | Parameterize and expand test_api_apps_env to cover all flag permutations and teardown |
| airflow-core/src/airflow/cli/commands/api_server_command.py | Add with_api_apps_env decorator, apply to api_server, remove manual env handling, update type ignore on access_log |
Comments suppressed due to low confidence (1)
airflow-core/tests/unit/cli/commands/test_api_server_command.py:121
- [nitpick] The test verifies that
AIRFLOW_API_APPSis cleaned up, but doesn’t assert that it was initially set to the expected value. Consider adding anassertonmock_environ.__setitem__for the first assignment.
api_server_command.api_server(parsed_args)
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how much we want to configure apps in prod - I guess we might find it useful in some deployments. But should not we describe it in documentation why to do it and how to do it?
#43103 . Feel free to take it over @jason810496 |
IMO, it might be useful for scaling API server for Task Execution and Core API separately or for users that just need to run Task Execution API-server solely.
Thanks for bringing up the related issue! I will raise another for the documentation side. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is useful for people that want to deploy a standalone execution api server. (Scaling, isolation etc.)
airflow-core/tests/unit/cli/commands/test_api_server_command.py
Outdated
Show resolved
Hide resolved
airflow-core/tests/unit/cli/commands/test_api_server_command.py
Outdated
Show resolved
Hide resolved
874657d to
900003b
Compare
a41c768 to
824b711
Compare
824b711 to
19d8a8a
Compare
Backport failed to create: v3-0-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker c0c41ff v3-0-testThis should apply the commit to the v3-0-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue |
* Respect apps flags for api_server_command * Refactor test_api_apps_env
|
@jason810496 needs manual cherry picking I guess, or this fix won't be in the next patch release. |
|
Thanks for the reminder! I will raise backport PR when I back to home |
* Respect apps flags for api_server_command * Refactor test_api_apps_env (cherry picked from commit c0c41ff)
|
Cool, thanks. Please do when you get time
|
* Respect apps flags for api_server_command * Refactor test_api_apps_env (cherry picked from commit c0c41ff)
* Respect apps flags for api_server_command * Refactor test_api_apps_env (cherry picked from commit c0c41ff)
) * Respect apps flags for api_server_command * Refactor test_api_apps_env (cherry picked from commit c0c41ff)
related: #52860
Why
--appsflag with--devmode.AIRFLOW_API_APPSenvironment to specify which apps are going to initialize increate_appcall.AIRFLOW_API_APPSenv is no restored properly, we should set back to original value if it's set before theairlow api-serverrun.What
with_api_apps_envdecorator with proper teardowntest_api_apps_envtest with more scenario including all apps currently available, dev mode or not, and original env is set or not.