Re: [PR] Respect `apps` flags for api_server_command [airflow]
kaxil commented on PR #52929: URL: https://github.com/apache/airflow/pull/52929#issuecomment-3075319134 Cool, thanks. Please do when you get time > Thanks for the reminder! I will raise backport PR when I back to home -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on PR #52929: URL: https://github.com/apache/airflow/pull/52929#issuecomment-3073009624 Thanks for the reminder! I will raise backport PR when I back to home -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
pierrejeambrun commented on PR #52929: URL: https://github.com/apache/airflow/pull/52929#issuecomment-3072972455 @jason810496 needs manual cherry picking I guess. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
github-actions[bot] commented on PR #52929: URL: https://github.com/apache/airflow/pull/52929#issuecomment-3068743979 ### Backport failed to create: v3-0-test. View the failure log Run details Status Branch Result ❌ v3-0-test https://github.com/apache/airflow/commit/c0c41ff134044c565516842bb089d30e827dccab";> You can attempt to backport this manually by running: ```bash cherry_picker c0c41ff v3-0-test ``` This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking the files that need manual conflict resolution. After you have resolved the conflicts, you can continue the backport process by running: ```bash cherry_picker --continue ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 merged PR #52929: URL: https://github.com/apache/airflow/pull/52929 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190723107 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -101,49 +101,64 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ +expected_setitem_calls = [] + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), ): -apps_value = "core,execution" -port = "9092" -host = "somehost" +# Mock the environment variable with initial value or None +mock_environ.get.return_value = original_env -# Parse the command line arguments -args = self.parser.parse_args( -["api-server", "--port", port, "--host", host, "--apps", apps_value, "--dev"] -) - -# Ensure AIRFLOW_API_APPS is not set initially -mock_environ.get.return_value = None +# Parse the command line arguments and call the api_server command +parsed_args = self.parser.parse_args(args) +api_server_command.api_server(parsed_args) -# Call the fastapi_api command -api_server_command.api_server(args) - -# Assert that AIRFLOW_API_APPS was set in the environment before subprocess -mock_environ.__setitem__.assert_called_with("AIRFLOW_API_APPS", apps_value) +# Verify the AIRFLOW_API_APPS was set correctly +if "--apps" in args: +expected_setitem_calls.append( +mock.call(api_server_command.AIRFLOW_API_APPS, parsed_args.apps) +) -# Simulate subprocess execution -Popen.assert_called_with( -[ -"fastapi", -"dev", -"airflow-core/src/airflow/api_fastapi/main.py", -"--port", -port, -"--host", -host, -], -close_fds=True, -) +# Verify AIRFLOW_API_APPS was cleaned up +if original_env is not None: + expected_setitem_calls.append(mock.call(api_server_command.AIRFLOW_API_APPS, original_env)) +else: + mock_environ.pop.assert_called_with(api_server_command.AIRFLOW_API_APPS, None) -# Assert that AIRFLOW_API_APPS was unset after subprocess -mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") +# Verify that the environment variable was set and cleaned up correctly +mock_environ.__setitem__.assert_has_calls(expected_setitem_calls) Review Comment: > and the final AIRFLOW_API_APPS value, that's what matters in the end. Nice catch! I miss the validation for setting up the `AIRFLOW_API_APPS` values in previous unit test, I use `assert_has_calls` to verify in the refactor 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190718306 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), ): -apps_value = "core,execution" -port = "9092" -host = "somehost" - # Parse the command line arguments -args = self.parser.parse_args( -["api-server", "--port", port, "--host", host, "--apps", apps_value, "--dev"] -) +parsed_args = self.parser.parse_args(args) # Ensure AIRFLOW_API_APPS is not set initially -mock_environ.get.return_value = None +mock_environ.get.return_value = original_env -# Call the fastapi_api command -api_server_command.api_server(args) - -# Assert that AIRFLOW_API_APPS was set in the environment before subprocess -mock_environ.__setitem__.assert_called_with("AIRFLOW_API_APPS", apps_value) - -# Simulate subprocess execution -Popen.assert_called_with( -[ -"fastapi", -"dev", -"airflow-core/src/airflow/api_fastapi/main.py", -"--port", -port, -"--host", -host, -], -close_fds=True, -) +# Call the api_server command +api_server_command.api_server(parsed_args) -# Assert that AIRFLOW_API_APPS was unset after subprocess -mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") +# Verify AIRFLOW_API_APPS was cleaned up +if original_env is not None: +# os.environ[AIRFLOW_API_APPS] = original_value + mock_environ.__setitem__.assert_any_call(api_server_command.AIRFLOW_API_APPS, original_env) +else: + mock_environ.pop.assert_called_with(api_server_command.AIRFLOW_API_APPS, None) Review Comment: Thanks @pierrejeambrun for the review! Just refactored the test and rebased to latest main. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190718306 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), ): -apps_value = "core,execution" -port = "9092" -host = "somehost" - # Parse the command line arguments -args = self.parser.parse_args( -["api-server", "--port", port, "--host", host, "--apps", apps_value, "--dev"] -) +parsed_args = self.parser.parse_args(args) # Ensure AIRFLOW_API_APPS is not set initially -mock_environ.get.return_value = None +mock_environ.get.return_value = original_env -# Call the fastapi_api command -api_server_command.api_server(args) - -# Assert that AIRFLOW_API_APPS was set in the environment before subprocess -mock_environ.__setitem__.assert_called_with("AIRFLOW_API_APPS", apps_value) - -# Simulate subprocess execution -Popen.assert_called_with( -[ -"fastapi", -"dev", -"airflow-core/src/airflow/api_fastapi/main.py", -"--port", -port, -"--host", -host, -], -close_fds=True, -) +# Call the api_server command +api_server_command.api_server(parsed_args) -# Assert that AIRFLOW_API_APPS was unset after subprocess -mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") +# Verify AIRFLOW_API_APPS was cleaned up +if original_env is not None: +# os.environ[AIRFLOW_API_APPS] = original_value + mock_environ.__setitem__.assert_any_call(api_server_command.AIRFLOW_API_APPS, original_env) +else: + mock_environ.pop.assert_called_with(api_server_command.AIRFLOW_API_APPS, None) Review Comment: Thanks @pierrejeambrun for the review! Just refactored the test and rebased to latest main. > and the final AIRFLOW_API_APPS value, that's what matters in the end. Nice catch! I miss the validation for setting up the `AIRFLOW_API_APPS` values in previous unit test, I use `assert_has_calls` to verify in the refactor now. ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("su
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190718306 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), ): -apps_value = "core,execution" -port = "9092" -host = "somehost" - # Parse the command line arguments -args = self.parser.parse_args( -["api-server", "--port", port, "--host", host, "--apps", apps_value, "--dev"] -) +parsed_args = self.parser.parse_args(args) # Ensure AIRFLOW_API_APPS is not set initially -mock_environ.get.return_value = None +mock_environ.get.return_value = original_env -# Call the fastapi_api command -api_server_command.api_server(args) - -# Assert that AIRFLOW_API_APPS was set in the environment before subprocess -mock_environ.__setitem__.assert_called_with("AIRFLOW_API_APPS", apps_value) - -# Simulate subprocess execution -Popen.assert_called_with( -[ -"fastapi", -"dev", -"airflow-core/src/airflow/api_fastapi/main.py", -"--port", -port, -"--host", -host, -], -close_fds=True, -) +# Call the api_server command +api_server_command.api_server(parsed_args) -# Assert that AIRFLOW_API_APPS was unset after subprocess -mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") +# Verify AIRFLOW_API_APPS was cleaned up +if original_env is not None: +# os.environ[AIRFLOW_API_APPS] = original_value + mock_environ.__setitem__.assert_any_call(api_server_command.AIRFLOW_API_APPS, original_env) +else: + mock_environ.pop.assert_called_with(api_server_command.AIRFLOW_API_APPS, None) Review Comment: Thanks @pierrejeambrun for the review! Just refactored the test and rebased to latest main. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
pierrejeambrun commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190338221 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), ): -apps_value = "core,execution" -port = "9092" -host = "somehost" - # Parse the command line arguments -args = self.parser.parse_args( -["api-server", "--port", port, "--host", host, "--apps", apps_value, "--dev"] -) +parsed_args = self.parser.parse_args(args) # Ensure AIRFLOW_API_APPS is not set initially -mock_environ.get.return_value = None +mock_environ.get.return_value = original_env -# Call the fastapi_api command -api_server_command.api_server(args) - -# Assert that AIRFLOW_API_APPS was set in the environment before subprocess -mock_environ.__setitem__.assert_called_with("AIRFLOW_API_APPS", apps_value) - -# Simulate subprocess execution -Popen.assert_called_with( -[ -"fastapi", -"dev", -"airflow-core/src/airflow/api_fastapi/main.py", -"--port", -port, -"--host", -host, -], -close_fds=True, -) +# Call the api_server command +api_server_command.api_server(parsed_args) -# Assert that AIRFLOW_API_APPS was unset after subprocess -mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") +# Verify AIRFLOW_API_APPS was cleaned up +if original_env is not None: +# os.environ[AIRFLOW_API_APPS] = original_value + mock_environ.__setitem__.assert_any_call(api_server_command.AIRFLOW_API_APPS, original_env) +else: + mock_environ.pop.assert_called_with(api_server_command.AIRFLOW_API_APPS, None) Review Comment: Maybe we should check the `"subprocess.Popen"` check directly, and the final `AIRFLOW_API_APPS` value, that's what matters in the end. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
pierrejeambrun commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190336206 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), ): -apps_value = "core,execution" -port = "9092" -host = "somehost" - # Parse the command line arguments -args = self.parser.parse_args( -["api-server", "--port", port, "--host", host, "--apps", apps_value, "--dev"] -) +parsed_args = self.parser.parse_args(args) # Ensure AIRFLOW_API_APPS is not set initially -mock_environ.get.return_value = None +mock_environ.get.return_value = original_env -# Call the fastapi_api command -api_server_command.api_server(args) - -# Assert that AIRFLOW_API_APPS was set in the environment before subprocess -mock_environ.__setitem__.assert_called_with("AIRFLOW_API_APPS", apps_value) - -# Simulate subprocess execution -Popen.assert_called_with( -[ -"fastapi", -"dev", -"airflow-core/src/airflow/api_fastapi/main.py", -"--port", -port, -"--host", -host, -], -close_fds=True, -) +# Call the api_server command +api_server_command.api_server(parsed_args) -# Assert that AIRFLOW_API_APPS was unset after subprocess -mock_environ.pop.assert_called_with("AIRFLOW_API_APPS") +# Verify AIRFLOW_API_APPS was cleaned up +if original_env is not None: +# os.environ[AIRFLOW_API_APPS] = original_value Review Comment: We still probably want to perform this. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190302582 ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), Review Comment: `--dev` flag needs to mock `subprocess.Popen`, resolve. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2190299257 ## airflow-core/src/airflow/cli/commands/api_server_command.py: ## @@ -112,7 +140,7 @@ def api_server(args): timeout_graceful_shutdown=worker_timeout, ssl_keyfile=ssl_key, ssl_certfile=ssl_cert, -access_log=access_logfile, +access_log=access_logfile, # type: ignore [arg-type] Review Comment: Will be fixed in #52860 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
jason810496 commented on PR #52929: URL: https://github.com/apache/airflow/pull/52929#issuecomment-3045450422 > I am not sure how much we want to configure apps in prod - I guess we might find it useful in some deployments. 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. > But should not we describe it in documentation why to do it and how to do it? > https://github.com/apache/airflow/issues/43103 . Feel free to take it over @jason810496 Thanks for bringing up the related issue! I will raise another for the documentation side. This PR is more like fixing the behavior for the `--apps` only. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
kaxil commented on PR #52929: URL: https://github.com/apache/airflow/pull/52929#issuecomment-3042387426 > 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? https://github.com/apache/airflow/issues/43103 . Feel free to take it over @jason810496 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Respect `apps` flags for api_server_command [airflow]
Copilot commented on code in PR #52929: URL: https://github.com/apache/airflow/pull/52929#discussion_r2188054419 ## airflow-core/src/airflow/cli/commands/api_server_command.py: ## @@ -112,7 +140,7 @@ def api_server(args): timeout_graceful_shutdown=worker_timeout, ssl_keyfile=ssl_key, ssl_certfile=ssl_cert, -access_log=access_logfile, +access_log=access_logfile, # type: ignore [arg-type] Review Comment: Rather than using a `# type: ignore`, consider casting `access_logfile` to the correct type (e.g., `str`) or adjusting its annotation so `access_log` receives a matching type. ```suggestion access_log=access_logfile, ``` ## airflow-core/tests/unit/cli/commands/test_api_server_command.py: ## @@ -75,49 +75,57 @@ def test_dev_arg(self, args, expected_command): close_fds=True, ) -def test_apps_env_var_set_unset(self): [email protected]( +"args", +[ +(["api-server"]), +(["api-server", "--apps", "all"]), +(["api-server", "--apps", "core,execution"]), +(["api-server", "--apps", "core"]), +(["api-server", "--apps", "execution"]), +], +ids=[ +"default_apps", +"all_apps_explicit", +"multiple_apps_explicit", +"single_app_core", +"single_app_execution", +], +) [email protected]("dev_mode", [True, False]) [email protected]( +"original_env", +[None, "some_value"], +) +def test_api_apps_env(self, args, dev_mode, original_env): """ Test that AIRFLOW_API_APPS is set and unset in the environment when calling the airflow api-server command """ + +if dev_mode: +args.append("--dev") + with ( -mock.patch("subprocess.Popen") as Popen, mock.patch("os.environ", autospec=True) as mock_environ, +mock.patch("uvicorn.run"), +mock.patch("subprocess.Popen"), Review Comment: There are two patches for `subprocess.Popen` in this `with` block—one named (`as Popen`) and one anonymous. Removing the duplicate anonymous patch will reduce confusion. ```suggestion ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] Respect `apps` flags for api_server_command [airflow]
jason810496 opened a new pull request, #52929: URL: https://github.com/apache/airflow/pull/52929 related: #52860 ## Why - While working on #52860, I found that current api-server only respect `--apps` flag with `--dev` mode. - We use `AIRFLOW_API_APPS` environment to specify which apps are going to initialize in `create_app` call. - Also the `AIRFLOW_API_APPS` env is no restored properly, we should set back to original value if it's set before the `airlow api-server` run. ## What - Add `with_api_apps_env` decorator with proper teardown - Refactor `test_api_apps_env` test with more scenario including all apps currently available, dev mode or not, and original env is set or not. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
