Add Legacy command displaying new CLI counterparts #10115

ephraimbuddy · 2020-08-02T09:41:43Z

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

turbaszek · 2020-08-02T09:50:10Z

airflow/cli/commands/legacy_commands.py

How about

def check_value(action, value): cmd_map = { "worker": "celery worker", # old: new ... } try: msg = "{} command, has been removed, please use `airflow {}`.format(value, cmd_map[value]) raise ArgumentError(action, msg) except KeyError: pass

We can consider making this map a global constant as it may be useful in the future. WDYT?

I like this!

How do you suggest to write a test for this? @turbaszek

I would mock COMMAND_MAP with {"old_cmd": "new_cmd"} and then:

action = MagicMock() with self.assertrises(ArgumentError): check_value(action, "old_cmd")

plus some regex check that the error msg is valid one.

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() )

I propose to test this by testing the CLI parser. We overwrite private methods, so I wouldn't trust unit tests only.

kaxil

Thanks @ephraimbuddy for this PR.

How much effort might be required to make it backwards-compatible with deprecation warning, I definitely not saying we should do it --- but would like to hear your opinions @ephraimbuddy @turbaszek @potiuk .

kaxil · 2020-08-02T10:15:57Z

CI is failing:


self = DefaultHelpParser(prog='airflow celery', usage=None, description='Start celery components. Works only when using Celer...xecutor/celery.html', formatter_class=<class 'argparse.RawTextHelpFormatter'>, conflict_handler='error', add_help=True)
action = _SubParsersAction(option_strings=[], dest='subcommand', nargs='A...', const=None, default=None, type=None, choices={'f...class=<class 'argparse.RawTextHelpFormatter'>, conflict_handler='error', add_help=True)}, help=None, metavar='COMMAND')
value = 'worker'

    def _check_value(self, action, value):
        """Override _check_value and check conditionally added command"""
        executor = conf.get('core', 'EXECUTOR')
        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)

airflow/cli/cli_parser.py:69:

potiuk · 2020-08-02T10:23:16Z

I

How much effort might be required to make it backwards-compatible with deprecation warning, I definitely not saying we should do it --- but would like to hear your opinions @ephraimbuddy @turbaszek @potiuk .

I think it's totally not worth it. There are very few issues where people use the CLI to automate stuff for Airflow and for most of them migration to 2.0 will be quite some change anyway (at least running upgrade check). I think we are talking about a handful of users here - far less than people who are writing DAGs and they use CLI either very rarely (db init, db reset etc) so they don't remember the commands by heart, or very frequently use one command ("backfill") but then it will be very easy to adapt to that single command.

kaxil · 2020-08-02T10:23:42Z

I

How much effort might be required to make it backwards-compatible with deprecation warning, I definitely not saying we should do it --- but would like to hear your opinions @ephraimbuddy @turbaszek @potiuk .

I think it's totally not worth it. There are very few issues where people use the CLI to automate stuff for Airflow and for most of them migration to 2.0 will be quite some change (at least running upgrade check). I think we are talking about a handful of users here - far less than people who are writing DAGs and they use CLI either very rarely (db init, db reset etc) so they don't remember the commands by heart, or very frequently use one command ("backfill") but then it will be very easy to adapt to that single command.

Agree 👍

ephraimbuddy · 2020-08-02T10:31:25Z

CI is failing:


self = DefaultHelpParser(prog='airflow celery', usage=None, description='Start celery components. Works only when using Celer...xecutor/celery.html', formatter_class=<class 'argparse.RawTextHelpFormatter'>, conflict_handler='error', add_help=True)
action = _SubParsersAction(option_strings=[], dest='subcommand', nargs='A...', const=None, default=None, type=None, choices={'f...class=<class 'argparse.RawTextHelpFormatter'>, conflict_handler='error', add_help=True)}, help=None, metavar='COMMAND')
value = 'worker'

    def _check_value(self, action, value):
        """Override _check_value and check conditionally added command"""
        executor = conf.get('core', 'EXECUTOR')
        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)

airflow/cli/cli_parser.py:69:

It is like the worker command is still in use in the CI instead of celery worker?

turbaszek · 2020-08-02T10:58:41Z

It is like the worker command is still in use in the CI instead of celery worker?

Probably the problem is related to subparsers. The command celery worker is first parsed as celery then as worker with respectible parser.

ephraimbuddy · 2020-08-02T11:00:38Z

It is like the worker command is still in use in the CI instead of celery worker?

Probably the problem is related to subparsers. The command celery worker is first parsed as celery then as worker with respectible parser.

Seen, It also affects backfill, I understand now.

airflow/cli/commands/legacy_commands.py

mik-laj · 2020-08-02T16:39:58Z

airflow/cli/commands/legacy_commands.py

Suggested change

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}`"

mik-laj · 2020-08-02T16:42:57Z

airflow/cli/commands/legacy_commands.py

Can you give a more descriptive name e.g. check_legacy_commands?

turbaszek · 2020-08-03T08:48:03Z

airflow/cli/commands/legacy_commands.py

Suggested change

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:

turbaszek · 2020-08-03T08:49:39Z

tests/cli/commands/test_legacy_commands.py

If we are using existing command there's no reason to mock I think

boring-cyborg bot added the area:CLI label Aug 2, 2020

turbaszek reviewed Aug 2, 2020

View reviewed changes

kaxil reviewed Aug 2, 2020

View reviewed changes

kaxil approved these changes Aug 2, 2020

View reviewed changes

potiuk requested changes Aug 2, 2020

View reviewed changes

airflow/cli/commands/legacy_commands.py Outdated Show resolved Hide resolved

airflow/cli/commands/legacy_commands.py Outdated Show resolved Hide resolved

mik-laj reviewed Aug 2, 2020

View reviewed changes

turbaszek reviewed Aug 3, 2020

View reviewed changes

ephraimbuddy added 5 commits August 3, 2020 11:27

show error for legacy cli commands

77a4c5a

apply review suggestions

7ef98f9

fixup! apply review suggestions and add tests

20fa879

fixup! fixup! apply review suggestions and add tests

c5c8cfa

fixup! fixup! fixup! apply review suggestions and add tests

6026750

mik-laj approved these changes Aug 3, 2020

View reviewed changes

potiuk approved these changes Aug 3, 2020

View reviewed changes

potiuk merged commit 201823b into apache:master Aug 3, 2020

ephraimbuddy deleted the legacy-commands branch August 3, 2020 18:33

	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}`"

-    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:

Add Legacy command displaying new CLI counterparts #10115

Add Legacy command displaying new CLI counterparts #10115

Uh oh!

Conversation

ephraimbuddy commented Aug 2, 2020

Uh oh!

turbaszek Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ephraimbuddy Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

kaxil commented Aug 2, 2020

Uh oh!

potiuk commented Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil commented Aug 2, 2020 • edited by potiuk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ephraimbuddy commented Aug 2, 2020

Uh oh!

turbaszek commented Aug 2, 2020

Uh oh!

ephraimbuddy commented Aug 2, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

turbaszek Aug 2, 2020 •

edited

Loading

ephraimbuddy Aug 2, 2020 •

edited

Loading

potiuk commented Aug 2, 2020 •

edited

Loading

kaxil commented Aug 2, 2020 •

edited by potiuk

Loading