Skip to content

[AIRFLOW-1247] Fix ignore all dependencies argument ignored - #2327

Closed
ghost wants to merge 1 commit into
apache:masterfrom
mremes:patch-1
Closed

[AIRFLOW-1247] Fix ignore all dependencies argument ignored#2327
ghost wants to merge 1 commit into
apache:masterfrom
mremes:patch-1

Conversation

@ghost

@ghost ghost commented May 25, 2017

Copy link
Copy Markdown

Cause: Comma missing so the store_true argument goes to help string.
Fix: Add missing comma

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

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

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

@mention-bot

Copy link
Copy Markdown

@mremes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mistercrunch, @bolkedebruin and @aoen to be potential reviewers.

@aoen

aoen commented May 25, 2017

Copy link
Copy Markdown
Contributor

LGTM, thanks!

@codecov-io

codecov-io commented May 25, 2017

Copy link
Copy Markdown

Codecov Report

Merging #2327 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2327      +/-   ##
==========================================
+ Coverage      71%   71.08%   +0.07%     
==========================================
  Files         150      150              
  Lines       11740    11740              
==========================================
+ Hits         8336     8345       +9     
+ Misses       3404     3395       -9
Impacted Files Coverage Δ
airflow/bin/cli.py 53.45% <ø> (+1.41%) ⬆️

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 6e52070...256336f. Read the comment docs.

@bolkedebruin

Copy link
Copy Markdown
Contributor

Can you please add tests and update the commit message?

@ghost
ghost force-pushed the patch-1 branch 2 times, most recently from 84c9faa to 7785184 Compare May 26, 2017 05:51
@bolkedebruin

Copy link
Copy Markdown
Contributor

@mremes please don't forget the tests: it is the reason why it was not caught in the first place.

@ghost
ghost force-pushed the patch-1 branch 2 times, most recently from a70f7ae to 44a6baa Compare May 29, 2017 18:35
@ghost

ghost commented May 29, 2017

Copy link
Copy Markdown
Author

@bolkedebruin added tests for upstream and depends_on_past dependencies not met. Run the scenario locally as I don't have the dev environment setup for running the tests yet... Will probably need to as there's more to contribute

@ghost
ghost force-pushed the patch-1 branch 3 times, most recently from f459d5e to cb381db Compare May 29, 2017 19:18
@bolkedebruin

Copy link
Copy Markdown
Contributor

can you make the tests pass?

@ghost

ghost commented Jun 14, 2017

Copy link
Copy Markdown
Author

@bolkedebruin It passes all well

Comment thread airflow/bin/cli.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to change this to action="store_true", then this issue would have been avoided. Bonus points for fixing the othoer cases.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saguziel probably a refactoring PR would be the right place to do that.

@aoen

aoen commented Jul 13, 2017

Copy link
Copy Markdown
Contributor

@ashb

ashb commented Sep 8, 2017

Copy link
Copy Markdown
Member

@mremes One of the causes of the test failure is a missing license in dags/test_cli_triggered_dags.py -- could you add it please?

@criccomini

Copy link
Copy Markdown
Contributor

Checking on status. If you want this in 1.9.0, it needs to get merged shortly.

@ghost

ghost commented Sep 20, 2017

Copy link
Copy Markdown
Author

@criccomini done, fixed tests by adding the license to the head of the test file.

The code change is the same as in the #2441, except this adds the tests for the functionality.

@criccomini

Copy link
Copy Markdown
Contributor

Merging!

@asfgit asfgit closed this in 5ee0209 Sep 20, 2017
mrares pushed a commit to mrares/incubator-airflow that referenced this pull request Oct 7, 2017
mrares pushed a commit to mrares/incubator-airflow that referenced this pull request Dec 5, 2017
mrares pushed a commit to mrares/incubator-airflow that referenced this pull request Apr 23, 2018
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.

8 participants