From 77a4c5aea73c9550e6ddadfd7ac1386cbeefb684 Mon Sep 17 00:00:00 2001 From: EphraimBuddy Date: Sun, 2 Aug 2020 10:34:57 +0100 Subject: [PATCH 1/5] show error for legacy cli commands --- airflow/cli/cli_parser.py | 2 + airflow/cli/commands/legacy_commands.py | 118 ++++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 airflow/cli/commands/legacy_commands.py diff --git a/airflow/cli/cli_parser.py b/airflow/cli/cli_parser.py index a20c0c9c7208f..2d70a45ce2994 100644 --- a/airflow/cli/cli_parser.py +++ b/airflow/cli/cli_parser.py @@ -28,6 +28,7 @@ from tabulate import tabulate_formats from airflow import settings +from airflow.cli.commands.legacy_commands import check_value from airflow.configuration import conf from airflow.exceptions import AirflowException from airflow.executors.executor_loader import ExecutorLoader @@ -65,6 +66,7 @@ def _check_value(self, action, value): if value == 'celery' and executor != ExecutorLoader.CELERY_EXECUTOR: message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}' raise ArgumentError(action, message) + check_value(action, value) super()._check_value(action, value) diff --git a/airflow/cli/commands/legacy_commands.py b/airflow/cli/commands/legacy_commands.py new file mode 100644 index 0000000000000..fca83280425b7 --- /dev/null +++ b/airflow/cli/commands/legacy_commands.py @@ -0,0 +1,118 @@ +# 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 argparse import ArgumentError + + +def check_value(action, value): # pylint: disable=too-many-branches + # pylint: disable=too-many-statements + """ Checks command value and raise error if value is in removed command""" + + # celery group + if value == "worker": + message = "worker command has been removed, please use `airflow celery worker`" + raise ArgumentError(action, message) + if value == "flower": + message = "flower command has been removed, please use `airflow celery flower`" + raise ArgumentError(action, message) + + # dags group + if value == "trigger_dag": + message = "trigger_dag command has been removed, please use `airflow dags trigger`" + raise ArgumentError(action, message) + if value == "delete_dag": + message = "delete_dag command has been removed, please use `airflow dags delete`" + raise ArgumentError(action, message) + if value == "show_dag": + message = "show_dag command has been removed, please use `airflow dags show`" + raise ArgumentError(action, message) + if value == "list_dag": + message = "list_dag command has been removed, please use `airflow dags list`" + raise ArgumentError(action, message) + if value == "dag_status": + message = "dag_status command has been removed, please use `airflow dags status`" + raise ArgumentError(action, message) + if value == "backfill": + message = "backfill command has been removed, please use `airflow dags backfill`" + raise ArgumentError(action, message) + if value == "list_dag_runs": + message = "list_dag_runs command has been removed, please use `airflow dags list_runs`" + raise ArgumentError(action, message) + if value == "pause": + message = "pause command has been removed, please use `airflow dags pause`" + raise ArgumentError(action, message) + if value == "unpause": + message = "unpause command has been removed, please use `airflow dags unpause`" + raise ArgumentError(action, message) + + # tasks group + if value == "test": + message = "test command has been removed, please use `airflow tasks test`" + raise ArgumentError(action, message) + if value == "clear": + message = "clear command has been removed, please use `airflow tasks clear`" + raise ArgumentError(action, message) + if value == "list_tasks": + message = "list_tasks command has been removed, please use `airflow tasks list`" + raise ArgumentError(action, message) + if value == "task_failed_deps": + message = "task_failed_deps command has been removed, please use `airflow tasks failed_deps`" + raise ArgumentError(action, message) + if value == "task_state": + message = "task_state command has been removed, please use `airflow tasks state`" + raise ArgumentError(action, message) + if value == "run": + message = "run command has been removed, please use `airflow tasks run`" + raise ArgumentError(action, message) + if value == "render": + message = "render command has been removed, please use `airflow tasks render`" + raise ArgumentError(action, message) + + # db group + if value == "initdb": + message = "initdb command has been removed, please use `airflow db init`" + raise ArgumentError(action, message) + if value == "resetdb": + message = "resetdb command has been removed, please use `airflow db reset`" + raise ArgumentError(action, message) + if value == "upgradedb": + message = "upgradedb command has been removed, please use `airflow db upgrade`" + raise ArgumentError(action, message) + if value == "checkdb": + message = "checkdb command has been removed, please use `airflow db check`" + raise ArgumentError(action, message) + if value == "shell": + message = "shell command has been removed, please use `airflow db shell`" + raise ArgumentError(action, message) + + # pools group + if value == "pool": + message = "pool command has been removed, please use `airflow pools`" + raise ArgumentError(action, message) + + # users group + if value == "list_users": + message = "list_users command has been removed, please use `airflow users list`" + raise ArgumentError(action, message) + + if value == "create_user": + message = "create_user command has been removed, please use `airflow users create`" + raise ArgumentError(action, message) + + if value == "delete_user": + message = "delete_user command has been removed, please use `airflow users delete`" + raise ArgumentError(action, message) From 7ef98f9af45147ab8a8365626f1df3c423089707 Mon Sep 17 00:00:00 2001 From: EphraimBuddy Date: Sun, 2 Aug 2020 11:22:56 +0100 Subject: [PATCH 2/5] apply review suggestions --- airflow/cli/commands/legacy_commands.py | 137 +++++++----------------- 1 file changed, 38 insertions(+), 99 deletions(-) diff --git a/airflow/cli/commands/legacy_commands.py b/airflow/cli/commands/legacy_commands.py index fca83280425b7..214ce3a150d78 100644 --- a/airflow/cli/commands/legacy_commands.py +++ b/airflow/cli/commands/legacy_commands.py @@ -17,102 +17,41 @@ from argparse import ArgumentError - -def check_value(action, value): # pylint: disable=too-many-branches - # pylint: disable=too-many-statements - """ Checks command value and raise error if value is in removed command""" - - # celery group - if value == "worker": - message = "worker command has been removed, please use `airflow celery worker`" - raise ArgumentError(action, message) - if value == "flower": - message = "flower command has been removed, please use `airflow celery flower`" - raise ArgumentError(action, message) - - # dags group - if value == "trigger_dag": - message = "trigger_dag command has been removed, please use `airflow dags trigger`" - raise ArgumentError(action, message) - if value == "delete_dag": - message = "delete_dag command has been removed, please use `airflow dags delete`" - raise ArgumentError(action, message) - if value == "show_dag": - message = "show_dag command has been removed, please use `airflow dags show`" - raise ArgumentError(action, message) - if value == "list_dag": - message = "list_dag command has been removed, please use `airflow dags list`" - raise ArgumentError(action, message) - if value == "dag_status": - message = "dag_status command has been removed, please use `airflow dags status`" - raise ArgumentError(action, message) - if value == "backfill": - message = "backfill command has been removed, please use `airflow dags backfill`" - raise ArgumentError(action, message) - if value == "list_dag_runs": - message = "list_dag_runs command has been removed, please use `airflow dags list_runs`" - raise ArgumentError(action, message) - if value == "pause": - message = "pause command has been removed, please use `airflow dags pause`" - raise ArgumentError(action, message) - if value == "unpause": - message = "unpause command has been removed, please use `airflow dags unpause`" - raise ArgumentError(action, message) - - # tasks group - if value == "test": - message = "test command has been removed, please use `airflow tasks test`" - raise ArgumentError(action, message) - if value == "clear": - message = "clear command has been removed, please use `airflow tasks clear`" - raise ArgumentError(action, message) - if value == "list_tasks": - message = "list_tasks command has been removed, please use `airflow tasks list`" - raise ArgumentError(action, message) - if value == "task_failed_deps": - message = "task_failed_deps command has been removed, please use `airflow tasks failed_deps`" - raise ArgumentError(action, message) - if value == "task_state": - message = "task_state command has been removed, please use `airflow tasks state`" - raise ArgumentError(action, message) - if value == "run": - message = "run command has been removed, please use `airflow tasks run`" - raise ArgumentError(action, message) - if value == "render": - message = "render command has been removed, please use `airflow tasks render`" - raise ArgumentError(action, message) - - # db group - if value == "initdb": - message = "initdb command has been removed, please use `airflow db init`" - raise ArgumentError(action, message) - if value == "resetdb": - message = "resetdb command has been removed, please use `airflow db reset`" - raise ArgumentError(action, message) - if value == "upgradedb": - message = "upgradedb command has been removed, please use `airflow db upgrade`" - raise ArgumentError(action, message) - if value == "checkdb": - message = "checkdb command has been removed, please use `airflow db check`" - raise ArgumentError(action, message) - if value == "shell": - message = "shell command has been removed, please use `airflow db shell`" - raise ArgumentError(action, message) - - # pools group - if value == "pool": - message = "pool command has been removed, please use `airflow pools`" - raise ArgumentError(action, message) - - # users group - if value == "list_users": - message = "list_users command has been removed, please use `airflow users list`" - raise ArgumentError(action, message) - - if value == "create_user": - message = "create_user command has been removed, please use `airflow users create`" - raise ArgumentError(action, message) - - if value == "delete_user": - message = "delete_user command has been removed, please use `airflow users delete`" - raise ArgumentError(action, message) +COMMAND_MAP = { + "worker": "celery worker", + "flower": "celery flower", + "trigger_dag": "dags trigger", + "delete_dag": "dags delete", + "show_dag": "dags show", + "list_dag": "dags list", + "dag_status": "dags status", + "backfill": "dags backfill", + "list_dag_runs": "dags list_runs", + "pause": "dags pause", + "unpause": "dags unpause", + "test": "tasks test", + "clear": "tasks clear", + "list_tasks": "tasks list", + "task_failed_deps": "tasks failed_deps", + "task_state": "tasks state", + "run": "tasks run", + "render": "tasks render", + "initdb": "db init", + "resetdb": "db reset", + "upgradedb": "db upgrade", + "checkdb": "db check", + "shell": "db shell", + "pool": "pools", + "list_users": "users list", + "create_user": "users create", + "delete_user": "users delete" +} + + +def check_value(action, value): + """ Checks command value and raise error if value is in removed command """ + try: + msg = "{} command, has been removed, please use `airflow {}`".format(value, COMMAND_MAP[value]) + raise ArgumentError(action, msg) + except KeyError: + pass From 20fa8797ed90ef692166b1be6841bfbd60c3ce37 Mon Sep 17 00:00:00 2001 From: EphraimBuddy Date: Sun, 2 Aug 2020 14:19:32 +0100 Subject: [PATCH 3/5] fixup! apply review suggestions and add tests --- airflow/cli/cli_parser.py | 3 +- airflow/cli/commands/legacy_commands.py | 4 +- tests/cli/commands/test_legacy_commands.py | 46 ++++++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 tests/cli/commands/test_legacy_commands.py diff --git a/airflow/cli/cli_parser.py b/airflow/cli/cli_parser.py index 2d70a45ce2994..b1a2d9e701b85 100644 --- a/airflow/cli/cli_parser.py +++ b/airflow/cli/cli_parser.py @@ -66,7 +66,8 @@ def _check_value(self, action, value): if value == 'celery' and executor != ExecutorLoader.CELERY_EXECUTOR: message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}' raise ArgumentError(action, message) - check_value(action, value) + if action.choices is not None and value not in action.choices: + check_value(action, value) super()._check_value(action, value) diff --git a/airflow/cli/commands/legacy_commands.py b/airflow/cli/commands/legacy_commands.py index 214ce3a150d78..80eb3fae26275 100644 --- a/airflow/cli/commands/legacy_commands.py +++ b/airflow/cli/commands/legacy_commands.py @@ -50,8 +50,6 @@ def check_value(action, value): """ Checks command value and raise error if value is in removed command """ - try: + if COMMAND_MAP.get(value) is not None: msg = "{} command, has been removed, please use `airflow {}`".format(value, COMMAND_MAP[value]) raise ArgumentError(action, msg) - except KeyError: - pass diff --git a/tests/cli/commands/test_legacy_commands.py b/tests/cli/commands/test_legacy_commands.py new file mode 100644 index 0000000000000..07b629a6b0dd4 --- /dev/null +++ b/tests/cli/commands/test_legacy_commands.py @@ -0,0 +1,46 @@ +# 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. + +import unittest +from argparse import ArgumentError +from unittest import mock +from unittest.mock import MagicMock + +from airflow.cli.commands.legacy_commands import COMMAND_MAP, check_value + +LEGACY_COMMANDS = ["worker", "flower", "trigger_dag", "delete_dag", "show_dag", "list_dag", + "dag_status", "backfill", "list_dag_runs", "pause", "unpause", "test", + "clear", "list_tasks", "task_failed_deps", "task_state", "run", + "render", "initdb", "resetdb", "upgradedb", "checkdb", "shell", "pool", + "list_users", "create_user", "delete_user"] + + +class TestLegacyCommandCheck(unittest.TestCase): + + def test_command_map(self): + for item in LEGACY_COMMANDS: + self.assertIsNotNone(COMMAND_MAP[item]) + + @mock.patch("airflow.cli.commands.legacy_commands.COMMAND_MAP") + def test_check_value(self, command_map): + action = MagicMock() + command_map.__getitem__.return_value = "users list" + with self.assertRaises(ArgumentError) as e: + check_value(action, 'list_user') + self.assertEqual( + str(e.exception), + "argument : list_user command, has been removed, please use `airflow users list`") From c5c8cfac0c8312379317c68e78b742cea887fbe7 Mon Sep 17 00:00:00 2001 From: EphraimBuddy Date: Mon, 3 Aug 2020 07:12:38 +0100 Subject: [PATCH 4/5] fixup! fixup! apply review suggestions and add tests --- airflow/cli/cli_parser.py | 4 +-- airflow/cli/commands/legacy_commands.py | 5 ++-- tests/cli/commands/test_legacy_commands.py | 30 +++++++++++++++++----- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/airflow/cli/cli_parser.py b/airflow/cli/cli_parser.py index b1a2d9e701b85..5c043ccff1d04 100644 --- a/airflow/cli/cli_parser.py +++ b/airflow/cli/cli_parser.py @@ -28,7 +28,7 @@ from tabulate import tabulate_formats from airflow import settings -from airflow.cli.commands.legacy_commands import check_value +from airflow.cli.commands.legacy_commands import check_legacy_command from airflow.configuration import conf from airflow.exceptions import AirflowException from airflow.executors.executor_loader import ExecutorLoader @@ -67,7 +67,7 @@ def _check_value(self, action, value): message = f'celery subcommand works only with CeleryExecutor, your current executor: {executor}' raise ArgumentError(action, message) if action.choices is not None and value not in action.choices: - check_value(action, value) + check_legacy_command(action, value) super()._check_value(action, value) diff --git a/airflow/cli/commands/legacy_commands.py b/airflow/cli/commands/legacy_commands.py index 80eb3fae26275..a0163a27f5bfe 100644 --- a/airflow/cli/commands/legacy_commands.py +++ b/airflow/cli/commands/legacy_commands.py @@ -48,8 +48,9 @@ } -def check_value(action, value): +def check_legacy_command(action, value): """ Checks command value and raise error if value is in removed command """ if COMMAND_MAP.get(value) is not None: - msg = "{} command, has been removed, please use `airflow {}`".format(value, COMMAND_MAP[value]) + new_command = COMMAND_MAP[value] + msg = f"`airflow {value}` command, has been removed, please use `airflow {new_command}`" raise ArgumentError(action, msg) diff --git a/tests/cli/commands/test_legacy_commands.py b/tests/cli/commands/test_legacy_commands.py index 07b629a6b0dd4..84010a8c415b8 100644 --- a/tests/cli/commands/test_legacy_commands.py +++ b/tests/cli/commands/test_legacy_commands.py @@ -14,13 +14,16 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. - +import contextlib +import io import unittest from argparse import ArgumentError from unittest import mock from unittest.mock import MagicMock -from airflow.cli.commands.legacy_commands import COMMAND_MAP, check_value +from airflow.cli import cli_parser +from airflow.cli.commands import config_command +from airflow.cli.commands.legacy_commands import COMMAND_MAP, check_legacy_command LEGACY_COMMANDS = ["worker", "flower", "trigger_dag", "delete_dag", "show_dag", "list_dag", "dag_status", "backfill", "list_dag_runs", "pause", "unpause", "test", @@ -29,18 +32,33 @@ "list_users", "create_user", "delete_user"] -class TestLegacyCommandCheck(unittest.TestCase): +class TestCliDeprecatedCommandsValue(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.parser = cli_parser.get_parser() + + def test_should_display_value(self): + with self.assertRaises(SystemExit) as cm_exception, \ + contextlib.redirect_stderr(io.StringIO()) as temp_stderr: + config_command.get_value(self.parser.parse_args(['worker'])) + + self.assertEqual(2, cm_exception.exception.code) + self.assertIn( + "`airflow worker` command, has been removed, " + "please use `airflow celery worker`, see help above.", + temp_stderr.getvalue().strip() + ) def test_command_map(self): for item in LEGACY_COMMANDS: self.assertIsNotNone(COMMAND_MAP[item]) @mock.patch("airflow.cli.commands.legacy_commands.COMMAND_MAP") - def test_check_value(self, command_map): + def test_check_legacy_command(self, command_map): action = MagicMock() command_map.__getitem__.return_value = "users list" with self.assertRaises(ArgumentError) as e: - check_value(action, 'list_user') + check_legacy_command(action, 'list_user') self.assertEqual( str(e.exception), - "argument : list_user command, has been removed, please use `airflow users list`") + "argument : `airflow list_user` command, has been removed, please use `airflow users list`") From 6026750d5793aee03d0d2d7b203686b041bab5b0 Mon Sep 17 00:00:00 2001 From: EphraimBuddy Date: Mon, 3 Aug 2020 11:24:35 +0100 Subject: [PATCH 5/5] fixup! fixup! fixup! apply review suggestions and add tests --- airflow/cli/commands/legacy_commands.py | 4 ++-- tests/cli/commands/test_legacy_commands.py | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/airflow/cli/commands/legacy_commands.py b/airflow/cli/commands/legacy_commands.py index a0163a27f5bfe..b2ca64ae08c27 100644 --- a/airflow/cli/commands/legacy_commands.py +++ b/airflow/cli/commands/legacy_commands.py @@ -50,7 +50,7 @@ def check_legacy_command(action, value): """ Checks command value and raise error if value is in removed command """ - if COMMAND_MAP.get(value) is not None: - new_command = COMMAND_MAP[value] + new_command = COMMAND_MAP.get(value) + if new_command is not None: msg = f"`airflow {value}` command, has been removed, please use `airflow {new_command}`" raise ArgumentError(action, msg) diff --git a/tests/cli/commands/test_legacy_commands.py b/tests/cli/commands/test_legacy_commands.py index 84010a8c415b8..42a04ff5bb008 100644 --- a/tests/cli/commands/test_legacy_commands.py +++ b/tests/cli/commands/test_legacy_commands.py @@ -18,7 +18,6 @@ import io import unittest from argparse import ArgumentError -from unittest import mock from unittest.mock import MagicMock from airflow.cli import cli_parser @@ -53,12 +52,10 @@ def test_command_map(self): for item in LEGACY_COMMANDS: self.assertIsNotNone(COMMAND_MAP[item]) - @mock.patch("airflow.cli.commands.legacy_commands.COMMAND_MAP") - def test_check_legacy_command(self, command_map): + def test_check_legacy_command(self): action = MagicMock() - command_map.__getitem__.return_value = "users list" with self.assertRaises(ArgumentError) as e: - check_legacy_command(action, 'list_user') + check_legacy_command(action, 'list_users') self.assertEqual( str(e.exception), - "argument : `airflow list_user` command, has been removed, please use `airflow users list`") + "argument : `airflow list_users` command, has been removed, please use `airflow users list`")