You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/02 09:41:50 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #10115: Add Legacy command displaying new CLI counterparts
ephraimbuddy opened a new pull request #10115:
URL: https://github.com/apache/airflow/pull/10115
Closes: #10109
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667659271
> > 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464068871
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,57 @@
+# 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
+
+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 """
Review comment:
What you need to add here is this if (from _check_value):
```
if action.choices is not None and value not in action.choices:
```
The problem is that the Default Help parser is run for both - the main "parser" and "celery" subcommand parser. And when it is run for the subcommand parser it should be accepted.
If you add the 'if' before, the check for old parameters will only be done if 'worker' is not valid choice.
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,57 @@
+# 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
+
+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])
Review comment:
I'd prefer an explicit
```
if not COMMAND_MAP.get(value):
raise ArgumentError(action, "...".format)
```
here). It's not the best practice to base your logic on exceptions, and particularly bad on "lack" of exception as you did here :) (so raising an exception if no exception was raised is bad ).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464060856
##########
File path: 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)
Review comment:
How do you suggest to write a test for this? @turbaszek
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667655772
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464070550
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,57 @@
+# 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
+
+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 """
Review comment:
Thanks! That solved it!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667655772
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 that single command.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464098100
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,55 @@
+# 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
+
+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 """
+ if COMMAND_MAP.get(value) is not None:
+ msg = "{} command, has been removed, please use `airflow {}`".format(value, COMMAND_MAP[value])
Review comment:
```suggestion
new_command = COMMAND_MAP[value]
msg = f"`airflow {value}` command, has been removed, please use `airflow {new_command}`"
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464057634
##########
File path: 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)
Review comment:
I like 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667655772
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 that single command.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464278038
##########
File path: tests/cli/commands/test_legacy_commands.py
##########
@@ -0,0 +1,64 @@
+# 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 contextlib
+import io
+import unittest
+from argparse import ArgumentError
+from unittest import mock
+from unittest.mock import MagicMock
+
+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",
+ "clear", "list_tasks", "task_failed_deps", "task_state", "run",
+ "render", "initdb", "resetdb", "upgradedb", "checkdb", "shell", "pool",
+ "list_users", "create_user", "delete_user"]
+
+
+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_legacy_command(self, command_map):
+ action = MagicMock()
+ command_map.__getitem__.return_value = "users list"
+ with self.assertRaises(ArgumentError) as e:
+ check_legacy_command(action, 'list_user')
Review comment:
If we are using existing command there's no reason to mock I think
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464057031
##########
File path: 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)
Review comment:
How about
```python
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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667659113
> 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464062455
##########
File path: 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)
Review comment:
I would mock `COMMAND_MAP` with `{"old_cmd": "new_cmd"}` and then:
```python
action = MagicMock()
with self.assertrises(ArgumentError):
check_value(action, "old_cmd")
```
plus some regex check that the error msg is valid one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464098386
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,55 @@
+# 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
+
+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):
Review comment:
Can you give a more descriptive name e.g. check_legacy_commands?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667656582
> 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`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464058148
##########
File path: 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)
Review comment:
Yeah!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464057031
##########
File path: 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)
Review comment:
How about
```python
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)
excep KeyError:
raise ArgumentError(action, "Command {} not found".format(value))
```
We can consider making this map a global constant as it may be useful in the future. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464057031
##########
File path: 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)
Review comment:
How about
```python
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:
raise ArgumentError(action, "Command {} not found".format(value))
```
We can consider making this map a global constant as it may be useful in the future. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464057031
##########
File path: 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)
Review comment:
How about
```python
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)
excep KeyError:
raise ArgumentError(action, "Command {} not found".format(value))
```
We can consider making this map a global constant as it may be useful in the future. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464069464
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,57 @@
+# 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
+
+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])
Review comment:
I totally agree
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667655807
> 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 that single command.
Agree 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464068871
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,57 @@
+# 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
+
+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 """
Review comment:
What you need to add here is this if (from _check_value):
```
if action.choices is not None and value not in action.choices:
```
The problem is that the Default Help parser is run for both - the main "parser" and "celery" subcommand parser. And when it is run for the subcommand parser it should be accepted.
If you add the 'if' before, the check for old parameters will only be done if they are not a valid choice.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464057031
##########
File path: 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)
Review comment:
How about
```python
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:
raise ArgumentError(action, "Command {} not found".format(value))
```
We can consider making this map a global constant as it may be useful in the future. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464099108
##########
File path: 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)
Review comment:
```python
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667655105
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:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464277205
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,56 @@
+# 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
+
+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_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]
Review comment:
```suggestion
new_command = COMMAND_MAP.get(value)
if new_command is not None:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#issuecomment-667655807
> 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 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10115:
URL: https://github.com/apache/airflow/pull/10115
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464060856
##########
File path: 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)
Review comment:
How do you suggest to write a test for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464069163
##########
File path: airflow/cli/commands/legacy_commands.py
##########
@@ -0,0 +1,57 @@
+# 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
+
+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])
Review comment:
I'd prefer an explicit
```
if COMMAND_MAP.get(value) is None:
raise ArgumentError(action, "...".format)
```
here). It's not the best practice to base your logic on exceptions, and particularly bad on "lack" of exception as you did here :) (so raising an exception if no exception was raised is bad ).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10115: Add Legacy command displaying new CLI counterparts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10115:
URL: https://github.com/apache/airflow/pull/10115#discussion_r464304945
##########
File path: tests/cli/commands/test_legacy_commands.py
##########
@@ -0,0 +1,64 @@
+# 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 contextlib
+import io
+import unittest
+from argparse import ArgumentError
+from unittest import mock
+from unittest.mock import MagicMock
+
+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",
+ "clear", "list_tasks", "task_failed_deps", "task_state", "run",
+ "render", "initdb", "resetdb", "upgradedb", "checkdb", "shell", "pool",
+ "list_users", "create_user", "delete_user"]
+
+
+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_legacy_command(self, command_map):
+ action = MagicMock()
+ command_map.__getitem__.return_value = "users list"
+ with self.assertRaises(ArgumentError) as e:
+ check_legacy_command(action, 'list_user')
Review comment:
Agree!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org