Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-5560] Allow no confirmation on reset dags #6195

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

robinedwards
Copy link
Contributor

@robinedwards robinedwards commented Sep 26, 2019

Make sure you have checked all steps below.

Jira

Description

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

When using the backfill command with the --reset_dags option there is a confirmation prompt and no way to suppress it. I've wired in the --yes parameter to do this, note that using --no_confirm wasn't an option as thats aliased to '-c' which is used for --conf in the backfill sub command.

Tests

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

No tests for command line arguments, tested locally though.

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.

    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release
  • Done automatically via argparse

@robinedwards robinedwards changed the base branch from v1-10-test to v1-10-stable September 26, 2019 15:30
@feng-tao
Copy link
Member

is there a reason to suppress it as not everyone is familiar with this input arg? this will provide a safety guard to allow users know what will get clear during backfill.

@robinedwards
Copy link
Contributor Author

Yes when running the command from a BashOperator for instance I need a way of surpressing the input. The flag is documented.

@codecov-io
Copy link

Codecov Report

Merging #6195 into v1-10-stable will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           v1-10-stable    #6195      +/-   ##
================================================
- Coverage         76.53%   76.52%   -0.01%     
================================================
  Files               509      509              
  Lines             34352    34352              
================================================
- Hits              26291    26289       -2     
- Misses             8061     8063       +2
Impacted Files Coverage Δ
airflow/bin/cli.py 62.59% <ø> (ø) ⬆️
airflow/utils/dag_processing.py 56.97% <0%> (-0.35%) ⬇️

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 d06a956...d1df354. Read the comment docs.

@feng-tao
Copy link
Member

ok, but the flag name is too generic, yes to ? How about --suppress_prompt ?
cc @ashb

@robinedwards
Copy link
Contributor Author

I only chose --yes because it was already being used:

https://github.com/apache/airflow/blob/master/airflow/bin/cli.py#L1711

But I would be happy to change it to whatever you guys can agree on as long as its consistent

Note that the PR for master also updates clear to also use --yes #6197.

@ashb
Copy link
Member

ashb commented Sep 30, 2019

--yes feels more familiar to be, at least that's what apt-get install --yes uses.

@ashb
Copy link
Member

ashb commented Sep 30, 2019

Oh and --yes is what we already use for no-confirm on db reset/resetdb

@robinedwards
Copy link
Contributor Author

Any conclusion on this?

@ashb
Copy link
Member

ashb commented Oct 7, 2019

Taking silence as assent :)

@ashb ashb merged commit 8aa7a01 into apache:v1-10-stable Oct 7, 2019
abhishekbafna pushed a commit to abhishekbafna/airflow that referenced this pull request Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants