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-3742] Respect the fallback arg in airflow.configuration.get #4567

Merged
merged 1 commit into from
Jan 29, 2019

Conversation

ashb
Copy link
Member

@ashb ashb commented Jan 21, 2019

Make sure you have checked all steps below.

Jira

Description

  • This fallback argument is part of the API from our parent class, but we didn't
    support it because of the various steps we perform in get() - this
    makes it behave more like the parent class, and can simplify a few
    instances in our code (I've only included one that I found here)

Tests

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

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

@ashb ashb requested review from Fokko and kaxil January 21, 2019 16:10
This argument is part of the API from our parent class, but we didn't
support it because of the various steps we perform in `get()` - this
makes it behave more like the parent class, and can simplify a few
instances in our code (I've only included one that I found here)
@ashb ashb force-pushed the config-parser-fallback-get branch from 1636852 to 983dca2 Compare January 21, 2019 16:59
@codecov-io
Copy link

Codecov Report

Merging #4567 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4567      +/-   ##
==========================================
+ Coverage   74.11%   74.12%   +0.01%     
==========================================
  Files         421      421              
  Lines       27681    27675       -6     
==========================================
- Hits        20515    20514       -1     
+ Misses       7166     7161       -5
Impacted Files Coverage Δ
airflow/settings.py 80.88% <100%> (+0.6%) ⬆️
airflow/configuration.py 92.08% <100%> (ø) ⬆️
airflow/jobs.py 77.65% <0%> (+0.27%) ⬆️

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 39dab6f...983dca2. Read the comment docs.

@ashb
Copy link
Member Author

ashb commented Jan 27, 2019

Ping @Fokko @feng-tao @kaxil Anyone got time to review this? I've got some fixes I'd like to do around RBAC webserver config/AIRFLOW_HOME that I'd like to use this for

@kaxil
Copy link
Member

kaxil commented Jan 27, 2019

LGTM @ashb

@feng-tao
Copy link
Member

LGTM

@feng-tao feng-tao merged commit 0d64fd8 into master Jan 29, 2019
@ashb ashb deleted the config-parser-fallback-get branch January 30, 2019 13:07
@ashb
Copy link
Member Author

ashb commented Feb 11, 2019

Fixed up by #4674

ashb added a commit that referenced this pull request Feb 11, 2019
#4567)

This argument is part of the API from our parent class, but we didn't
support it because of the various steps we perform in `get()` - this
makes it behave more like the parent class, and can simplify a few
instances in our code (I've only included one that I found here)
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
apache#4567)

This argument is part of the API from our parent class, but we didn't
support it because of the various steps we perform in `get()` - this
makes it behave more like the parent class, and can simplify a few
instances in our code (I've only included one that I found here)
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