[AIRFLOW-3998] Use nested commands in cli. #4821

jmcarp · 2019-03-02T20:56:37Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3998
- AIP-16).

Description

Here are some details about my PR, including screenshots of any UI changes:

This is a proof of concept for https://issues.apache.org/jira/browse/AIRFLOW-3998 / https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-16%3A+Use+nested+commands+in+CLI.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

Passes flake8

codecov-io · 2019-03-03T02:42:54Z

Codecov Report

Merging #4821 into master will decrease coverage by 0.26%.
The diff coverage is 82.92%.

@@            Coverage Diff             @@
##           master    #4821      +/-   ##
==========================================
- Coverage    79.1%   78.83%   -0.27%     
==========================================
  Files         489      489              
  Lines       30704    30675      -29     
==========================================
- Hits        24288    24184     -104     
- Misses       6416     6491      +75

Impacted Files	Coverage Δ
...ow/contrib/example_dags/example_qubole_operator.py	`0% <ø> (ø)`	⬆️
...flow/contrib/example_dags/example_qubole_sensor.py	`0% <ø> (ø)`	⬆️
...le_dags/example_passing_params_via_test_command.py	`100% <ø> (ø)`	⬆️
airflow/models/crypto.py	`78.78% <ø> (ø)`	⬆️
...trib/example_dags/example_azure_cosmosdb_sensor.py	`0% <ø> (ø)`	⬆️
airflow/models/taskinstance.py	`93.02% <100%> (-0.17%)`	⬇️
airflow/bin/cli.py	`66.05% <82.84%> (-0.41%)`	⬇️
airflow/operators/mysql_operator.py	`0% <0%> (-100%)`	⬇️
airflow/operators/mysql_to_hive.py	`0% <0%> (-100%)`	⬇️
airflow/utils/sqlalchemy.py	`74.41% <0%> (-4.66%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65eef1c...9eec98a. Read the comment docs.

zhongjiajie · 2019-03-07T06:41:17Z

👍 like to see this change.

airflow/bin/cli.py

jmcarp · 2019-04-14T20:50:33Z

This is still incomplete but ready for more review. I grouped commands related to dags into the airflow dags subcommand, tasks into airflow tasks, users into airflow users, etc. I don't have strong preferences about how we group some of these commands: for example, I left airflow initdb and airflow resetdb as top-level commands, but would be happy to move them to airflow db init and airflow db reset`.

@ashb @feng-tao @XD-DENG @Fokko: what do you think about the proposed changes? Happy to make any revisions, and I'll update documentation if/when we decide the final command structure.

ashb · 2019-04-15T16:24:53Z

No strong preference to same commit/PR or others but maybe a slight lean towards another PR as this is already quite big.

Have you checked what the rendered CLI docs look like with this?

ashb

Largely good I think, I only skimmed over the code changes (assuming they are some-what mechanical/rote in nature).

The following tasks are still at a top level:

task_failed_deps -> should be under tasks group?
initdb/upgradedb/resetdb - maybe a db group is called for?
sync_perm -- it cuts across users, roles and dags, so this one probably should stay at the top level?
run -- this somehow exists at the top level and under tasks.

Just running airflow tasks etc or any other "group" command fails with :AttributeError: 'Namespace' object has no attribute 'func' - that should print the help for the group ideally.

airflow/bin/cli.py

ashb · 2019-04-15T16:26:47Z

airflow/bin/cli.py

+
+@cli_utils.action_logging
+def variables_export(args):
+    export_helper(args.file)


Suggested change

export_helper(args.file)

variable_export_helper(args.file)

(and whatever is needed to make that function exist too)

Made this change.

ashb · 2019-04-15T16:35:49Z

airflow/bin/cli.py

-            'func': trigger_dag,
-            'help': "Trigger a DAG run",
-            'args': ('dag_id', 'subdir', 'run_id', 'conf', 'exec_date'),
+            'help': 'DAGs',


Could we expand this help text a little bit? Even something like "List and manage DAGs" would be better

jmcarp · 2019-04-19T18:54:57Z

@ashb: looks like sphinx-argparse handles nested commands correctly, and I updated so that missing subcommands print usage instructions instead of throwing an error. I also shuffled commands as suggested.

jmcarp · 2019-04-24T04:46:13Z

Updated documentation to match the new CLI structure.

What's the next step here? A vote on the AIP?

ashb · 2019-04-29T10:19:26Z

@jmcarp Yeah sounds right. Do you have time to write the vote email summarizing the changes or would you like me to pick this up?

feng-tao · 2019-04-29T17:59:07Z

the change LGTM, once we get the pass from AIP. I think it is good to go.

jmcarp · 2019-05-07T03:00:44Z

I'm traveling for work this week, so if you have time, feel free to write the vote email @ashb. Otherwise, I'll pick this up again next week.

jmcarp · 2019-06-06T03:03:23Z

@ashb: now that I have time to get back to this, I'm not sure where to find the procedure for holding the vote. How does that work?

ashb · 2019-06-06T14:29:19Z

It's not written down anywhere yet as we haven't fully formalized it. Here's an example of a recent vote on adding Pylint: https://lists.apache.org/thread.html/f4940d36e98ded96a2473bb2ccdfa4cc648faa2c1334b2aa901c0bba@%3Cdev.airflow.apache.org%3E

ashb · 2019-06-11T11:21:44Z

Started vote https://lists.apache.org/thread.html/cb2b854389ddc36d46e55d51625a90a54ef8d438299c5342bbd135bf@%3Cdev.airflow.apache.org%3E

jmcarp · 2019-07-01T04:33:06Z

Updated, tests passing.

jmcarp · 2019-07-07T03:37:33Z

Conflicts resolved. Ready for review @ashb @mik-laj or anybody else interested.

ashb · 2019-07-15T11:34:09Z

Sorry @jmcarp, I was in inbox hell and didn't see the ping, and it's conflicted again. I'm happy with this change now and the AIP is passed so once you resolve the conflicts again you can merge yourself(?)

jmcarp · 2019-07-19T04:42:56Z

Conflicts resolved @ashb. I don't think I ever got commit access to the repo--I must have missed a step. Any idea what I missed?

mik-laj · 2019-07-19T04:45:53Z

@jmcarp https://gitbox.apache.org/setup/ ;-)

mik-laj · 2019-07-19T04:46:49Z

I'm waiting for Travis to finish work.

jmcarp force-pushed the cli-nested-commands branch 4 times, most recently from 7513be6 to 0981878 Compare March 3, 2019 02:05

jmcarp force-pushed the cli-nested-commands branch 7 times, most recently from 62fb38d to 3656e9e Compare March 31, 2019 19:23

mik-laj reviewed Apr 1, 2019

View reviewed changes

airflow/bin/cli.py Show resolved Hide resolved

jmcarp force-pushed the cli-nested-commands branch 8 times, most recently from f5d7718 to b611410 Compare April 3, 2019 19:29

jmcarp force-pushed the cli-nested-commands branch from b611410 to 7edf4f1 Compare April 14, 2019 00:05

ashb requested changes Apr 15, 2019

View reviewed changes

jmcarp force-pushed the cli-nested-commands branch 4 times, most recently from 6ece318 to 5d75b7e Compare April 18, 2019 13:47

jmcarp force-pushed the cli-nested-commands branch 3 times, most recently from 2927a79 to 916af83 Compare April 18, 2019 19:06

ashb approved these changes Apr 23, 2019

View reviewed changes

jmcarp force-pushed the cli-nested-commands branch 3 times, most recently from 4a46363 to 5b67478 Compare April 24, 2019 03:22

jmcarp force-pushed the cli-nested-commands branch 3 times, most recently from 3a460cb to 9eec98a Compare July 1, 2019 03:55

jmcarp force-pushed the cli-nested-commands branch from 9eec98a to 8086520 Compare July 7, 2019 03:36

[AIRFLOW-3998] Use nested commands in cli.

d30237f

jmcarp force-pushed the cli-nested-commands branch from 8086520 to d30237f Compare July 19, 2019 04:39

ashb merged commit 30defe1 into apache:master Jul 19, 2019

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-3998] Use nested commands in cli. (apache#4821)

de9e1b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3998] Use nested commands in cli. #4821

[AIRFLOW-3998] Use nested commands in cli. #4821

jmcarp commented Mar 2, 2019 •

edited by ashb

Loading

codecov-io commented Mar 3, 2019 •

edited

Loading

zhongjiajie commented Mar 7, 2019

jmcarp commented Apr 14, 2019

ashb commented Apr 15, 2019

ashb left a comment

ashb Apr 15, 2019

jmcarp Apr 18, 2019

ashb Apr 15, 2019 •

edited

Loading

jmcarp Apr 18, 2019

jmcarp commented Apr 19, 2019

jmcarp commented Apr 24, 2019

ashb commented Apr 29, 2019

feng-tao commented Apr 29, 2019

jmcarp commented May 7, 2019

jmcarp commented Jun 6, 2019

ashb commented Jun 6, 2019

ashb commented Jun 11, 2019

jmcarp commented Jul 1, 2019

jmcarp commented Jul 7, 2019

ashb commented Jul 15, 2019

jmcarp commented Jul 19, 2019

mik-laj commented Jul 19, 2019

mik-laj commented Jul 19, 2019

[AIRFLOW-3998] Use nested commands in cli. #4821

[AIRFLOW-3998] Use nested commands in cli. #4821

Conversation

jmcarp commented Mar 2, 2019 • edited by ashb Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

codecov-io commented Mar 3, 2019 • edited Loading

Codecov Report

zhongjiajie commented Mar 7, 2019

jmcarp commented Apr 14, 2019

ashb commented Apr 15, 2019

ashb left a comment

Choose a reason for hiding this comment

ashb Apr 15, 2019

Choose a reason for hiding this comment

jmcarp Apr 18, 2019

Choose a reason for hiding this comment

ashb Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

jmcarp Apr 18, 2019

Choose a reason for hiding this comment

jmcarp commented Apr 19, 2019

jmcarp commented Apr 24, 2019

ashb commented Apr 29, 2019

feng-tao commented Apr 29, 2019

jmcarp commented May 7, 2019

jmcarp commented Jun 6, 2019

ashb commented Jun 6, 2019

ashb commented Jun 11, 2019

jmcarp commented Jul 1, 2019

jmcarp commented Jul 7, 2019

ashb commented Jul 15, 2019

jmcarp commented Jul 19, 2019

mik-laj commented Jul 19, 2019

mik-laj commented Jul 19, 2019

jmcarp commented Mar 2, 2019 •

edited by ashb

Loading

codecov-io commented Mar 3, 2019 •

edited

Loading

ashb Apr 15, 2019 •

edited

Loading