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] Fix handling of "fallback" for int/boolean config option #4674

Merged
merged 1 commit into from
Feb 11, 2019

Conversation

ttanay
Copy link
Contributor

@ttanay ttanay commented Feb 8, 2019

Support for a fallback kwarg was added in AirflowConfigParser.get
in [AIRFLOW-3742], but, AirflowConfigParser.getboolean also needs
to support it.

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"

Description

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

On the latest master, when I ran airflow worker, I got the following error:

[2019-02-09 00:12:25,983] {__init__.py:51} INFO - Using executor SequentialExecutor
Traceback (most recent call last):
File "/home/tanay/Coding/airflow/.env/bin/airflow", line 6, in <module>
exec(compile(open(__file__).read(), __file__, 'exec'))
File "/home/tanay/Coding/airflow/airflow/bin/airflow", line 32, in <module>
args.func(args)
File "/home/tanay/Coding/airflow/airflow/utils/cli.py", line 74, in wrapper
return f(*args, **kwargs)
File "/home/tanay/Coding/airflow/airflow/bin/cli.py", line 1058, in worker
if not settings.validate_session():
File "/home/tanay/Coding/airflow/airflow/settings.py", line 226, in validate_session
worker_precheck = conf.getboolean('core', 'worker_precheck', fallback=False)
TypeError: getboolean() got an unexpected keyword argument 'fallback'

Support for a fallback kwarg in AirflowConfigParser.get was added by @ashb in #4567.
Support for it also needs to be in AirflowConfigParser.getboolean.

Tests

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

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
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #4674 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4674      +/-   ##
==========================================
- Coverage   74.35%   74.33%   -0.02%     
==========================================
  Files         430      429       -1     
  Lines       27962    27950      -12     
==========================================
- Hits        20790    20778      -12     
  Misses       7172     7172
Impacted Files Coverage Δ
airflow/configuration.py 92.08% <100%> (ø) ⬆️
airflow/operators/latest_only_operator.py 90% <0%> (-0.48%) ⬇️
airflow/operators/python_operator.py 95.12% <0%> (-0.15%) ⬇️
airflow/sensors/base_sensor_operator.py 97.91% <0%> (-0.05%) ⬇️
airflow/models/skipmixin.py
airflow/models/__init__.py 92.81% <0%> (+0.06%) ⬆️

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 59d2615...5549642. Read the comment docs.

@feng-tao
Copy link
Member

feng-tao commented Feb 9, 2019

there are getint, getfloat etc in the configuration as well. Do we need to update those as well? cc @ashb

@ttanay
Copy link
Contributor Author

ttanay commented Feb 9, 2019

Yes, we need to update getint() and getfloat() as well. Basically, any function that internally calls get().
Making the changes.

@ttanay ttanay changed the title [AIRFLOW-3852] Add support for fallback in AirflowConfigParser.getboolean [AIRFLOW-3852] Add support for fallback kwarg in configuration Feb 9, 2019
@ttanay
Copy link
Contributor Author

ttanay commented Feb 9, 2019

Made the changes.

@ashb ashb changed the title [AIRFLOW-3852] Add support for fallback kwarg in configuration [AIRFLOW-3742] Fix handling of "fallback" for int/boolean config option Feb 11, 2019
…tint/boolean

We added (and used) fallback as an argument on `getboolean` but didn't
add it to the method, or add tests covering those "casting" accessors,
so they broke.

This fixes those methods, and adds tests covering them
@ashb
Copy link
Member

ashb commented Feb 11, 2019

Thanks @ttanay - I have extended your PR (on your fork) slightly by adding tests :)

@ashb ashb merged commit 8917ce6 into apache:master Feb 11, 2019
ashb pushed a commit that referenced this pull request Feb 11, 2019
…tint/boolean (#4674)

We added (and used) fallback as an argument on `getboolean` but didn't
add it to the method, or add tests covering those "casting" accessors,
so they broke.

This fixes those methods, and adds tests covering them
@ttanay
Copy link
Contributor Author

ttanay commented Feb 11, 2019

Thanks @ashb!

@feng-tao
Copy link
Member

lgtm

astahlman added a commit to lyft/airflow that referenced this pull request Feb 13, 2019
Merging to pull in this bug fix: apache#4674
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
…tint/boolean (apache#4674)

We added (and used) fallback as an argument on `getboolean` but didn't
add it to the method, or add tests covering those "casting" accessors,
so they broke.

This fixes those methods, and adds tests covering them
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