jedcunningham commented on code in PR #45255:
URL: https://github.com/apache/airflow/pull/45255#discussion_r1898731554
##########
providers/src/airflow/providers/celery/executors/celery_executor.py:
##########
@@ -163,8 +162,12 @@ def __getattr__(name):
CELERY_CLI_COMMAND_PATH = (
"airflow.providers.celery.cli.celery_command"
- if Version(airflow_version) >= Version("2.8.0")
- else "airflow.cli.commands.local_commands.celery_command"
+ if AIRFLOW_V_2_8_PLUS
+ else (
+ "airflow.cli.commands.local_commands.celery_command"
+ if AIRFLOW_V_3_0_PLUS
Review Comment:
It might be worth switching this to flat if/elses. Much easier to determine
what's happening vs nested ternary.
Case in point, won't "3.0" also be true for 2_8_PLUS, so we will never fall
into this else for the 3.0 command?
##########
providers/tests/celery/cli/test_celery_command.py:
##########
@@ -270,96 +270,196 @@ def test_run_command(self, mock_celery_app):
]
)
+ @mock.patch("airflow.cli.commands.daemon_utils.TimeoutPIDLockFile")
+ @mock.patch("airflow.cli.commands.daemon_utils.setup_locations")
+ @mock.patch("airflow.cli.commands.daemon_utils.daemon")
+ @mock.patch("airflow.providers.celery.executors.celery_executor.app")
+ def test_run_command_daemon_v_3_below(
+ self, mock_celery_app, mock_daemon, mock_setup_locations, mock_pid_file
+ ):
+ if not AIRFLOW_V_3_0_PLUS:
Review Comment:
Don't put the whole body of the test in a conditional - use [pytest
skipif](https://docs.pytest.org/en/stable/how-to/skipping.html#id1) instead.
##########
providers/src/airflow/providers/fab/auth_manager/cli_commands/db_command.py:
##########
@@ -17,12 +17,26 @@
from __future__ import annotations
from airflow import settings
-from airflow.cli.commands.local_commands.db_command import
run_db_downgrade_command, run_db_migrate_command
from airflow.providers.fab.auth_manager.models.db import _REVISION_HEADS_MAP,
FABDBManager
+from airflow.providers.fab.version_compat import AIRFLOW_V_3_0_PLUS
from airflow.utils import cli as cli_utils
from airflow.utils.providers_configuration_loader import
providers_configuration_loaded
+def get_db_command():
+ try:
+ if AIRFLOW_V_3_0_PLUS:
+ import airflow.cli.commands.local_commands.db_command as db_command
+ else:
+ import airflow.cli.commands.db_command as db_command
+ except ImportError:
+ from airflow.exceptions import AirflowOptionalProviderFeatureException
+
+ raise AirflowOptionalProviderFeatureException("Failed to import
db_command from Airflow CLI.")
Review Comment:
This doesn't feel like an appropriate use for
`AirflowOptionalProviderFeatureException` - if we have this cli command, this
import should work, there isn't really anything that is optional that would
allow it to work. Right?
--
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]
