[AIRFLOW-4858] Deprecate "Historical convenience functions" in conf #5495

haoliang7 · 2019-06-27T09:36:39Z

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-4858
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

Here are some details about my PR, including screenshots of any UI changes:
The first argument of zope.deprecation.deprecated should be string or string list to deprecate top level function in a module.

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

Code Quality

Passes flake8

haoliang7 · 2019-06-27T09:39:54Z

In conf we want to deprecate "historical convenience functions" get, getint, getboolean, etc. However the current code doesn't issue a deprecation warning if any of these function is called.

The first argument of zope.deprecation.deprecated should be string or string list to deprecate top level functions in a module.

codecov-io · 2019-06-27T11:11:05Z

Codecov Report

Merging #5495 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5495      +/-   ##
==========================================
+ Coverage   79.12%   79.12%   +<.01%     
==========================================
  Files         489      489              
  Lines       30688    30688              
==========================================
+ Hits        24281    24282       +1     
+ Misses       6407     6406       -1

Impacted Files	Coverage Δ
airflow/configuration.py	`91.94% <ø> (ø)`	⬆️
airflow/models/taskinstance.py	`93.18% <0%> (+0.16%)`	⬆️

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 f6ae172...aaccd36. Read the comment docs.

ashb

Can you please also fix up the existing uses in side airflow that this now correctly warns about?

For example a non-exhaustive list:

airflow/config_templates/airflow_local_settings.py contains many
airflow/lineage/init.py
airflow/logging_config.py

ashb · 2019-06-28T13:47:51Z

~~Please re-target against AIRFLOW-4723.~~ Nm - related but not the same issue.

haoliang7 · 2019-07-02T06:48:00Z

Can you please also fix up the existing uses in side airflow that this now correctly warns about?

For example a non-exhaustive list:

airflow/config_templates/airflow_local_settings.py contains many

airflow/lineage/init.py

airflow/logging_config.py

Sure, working on it.

haoliang7 · 2019-08-14T07:05:48Z

@ashb The old config methods are removed. Please review my last commit.

ashb · 2019-08-16T16:51:23Z

airflow/configuration.py

@@ -38,7 +38,7 @@

 # show Airflow's deprecation warnings
 warnings.filterwarnings(
-    action='default', category=DeprecationWarning, module='airflow')
+    action='always', category=DeprecationWarning, module='airflow')


Why this change? And what effect does this change have?

This was for testing and accidentally added to the commit. I have changed it back. Sorry for the confusion.

ashb · 2019-08-16T16:54:51Z

The change was a little bit bigger than needed (configuration.conf.getboolean was fine) but making it all consistent is no bad thing.

One small comment (on the warnings filter) then this looks good to me, thanks.

(Sorry, it's conflicted again, I didn't see the ping because my inbox is currently 1k deep in unread airflow messages 😉 )

haoliang7 · 2019-08-25T15:03:53Z

The change was a little bit bigger than needed (configuration.conf.getboolean was fine) but making it all consistent is no bad thing.

One small comment (on the warnings filter) then this looks good to me, thanks.

(Sorry, it's conflicted again, I didn't see the ping because my inbox is currently 1k deep in unread airflow messages 😉 )

@ashb The conflicts have been resolved. The warnings filter was a mistake that has been corrected. Please kindly review again. Thanks.

1. Issue old conf method deprecation warnings properly and remove current old conf method usages. 2. Unify the way to use conf as from airflow.configuration import conf 3. Fix improper test mock in tests/jobs/test_scheduler_job.py tests/test_logging_config.py If we remove the old conf method, they will mock the conf method and call the same method inside the mock object which will incur maximum recursion error.

…w.configuration These places were missed in the original PR (apache#5495)

…w.configuration (#6144) These places were missed in the original PR (#5495)

…w.configuration (#5495) 1. Issue old conf method deprecation warnings properly and remove current old conf method usages. 2. Unify the way to use conf as `from airflow.configuration import conf` (cherry picked from commit f497d1d)

…w.configuration (#6144) These places were missed in the original PR (#5495) (cherry picked from commit b2e06d0)

…w.configuration (apache#5495) 1. Issue old conf method deprecation warnings properly and remove current old conf method usages. 2. Unify the way to use conf as `from airflow.configuration import conf` (cherry picked from commit f497d1d)

…w.configuration (apache#6144) These places were missed in the original PR (apache#5495) (cherry picked from commit b2e06d0)

ashb requested changes Jun 28, 2019

View reviewed changes

ashb reviewed Aug 16, 2019

View reviewed changes

Hao Liang and others added 2 commits August 27, 2019 10:30

Deprecate "Historical convenience functions" in conf

b6215f2

ashb approved these changes Sep 3, 2019

View reviewed changes

ashb merged commit f497d1d into apache:master Sep 3, 2019

ashb mentioned this pull request Sep 18, 2019

[AIRFLOW-4858] Deprecate "Historical convenience functions" in airflow.configuration #6144

Merged

2 tasks

ashb added a commit to ashb/airflow that referenced this pull request Sep 18, 2019

[AIRFLOW-4858] Deprecate "Historical convenience functions" in airflo…

7a8e083

…w.configuration These places were missed in the original PR (apache#5495)

ashb added a commit that referenced this pull request Sep 18, 2019

[AIRFLOW-4858] Deprecate "Historical convenience functions" in airflo…

b2e06d0

…w.configuration (#6144) These places were missed in the original PR (#5495)

ashb added a commit that referenced this pull request Sep 24, 2019

[AIRFLOW-4858] Deprecate "Historical convenience functions" in airflo…

0495ea4

…w.configuration (#6144) These places were missed in the original PR (#5495) (cherry picked from commit b2e06d0)

ashb added a commit to ashb/airflow that referenced this pull request Sep 25, 2019

[AIRFLOW-4858] Deprecate "Historical convenience functions" in airflo…

d28cf63

…w.configuration (apache#6144) These places were missed in the original PR (apache#5495) (cherry picked from commit b2e06d0)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-4858] Deprecate "Historical convenience functions" in conf #5495

[AIRFLOW-4858] Deprecate "Historical convenience functions" in conf #5495

haoliang7 commented Jun 27, 2019 •

edited

Loading

haoliang7 commented Jun 27, 2019

codecov-io commented Jun 27, 2019 •

edited

Loading

ashb left a comment

ashb commented Jun 28, 2019 •

edited

Loading

haoliang7 commented Jul 2, 2019

haoliang7 commented Aug 14, 2019

ashb Aug 16, 2019

haoliang7 Aug 24, 2019

ashb commented Aug 16, 2019

haoliang7 commented Aug 25, 2019

[AIRFLOW-4858] Deprecate "Historical convenience functions" in conf #5495

[AIRFLOW-4858] Deprecate "Historical convenience functions" in conf #5495

Conversation

haoliang7 commented Jun 27, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

haoliang7 commented Jun 27, 2019

codecov-io commented Jun 27, 2019 • edited Loading

Codecov Report

ashb left a comment

Choose a reason for hiding this comment

ashb commented Jun 28, 2019 • edited Loading

haoliang7 commented Jul 2, 2019

haoliang7 commented Aug 14, 2019

ashb Aug 16, 2019

Choose a reason for hiding this comment

haoliang7 Aug 24, 2019

Choose a reason for hiding this comment

ashb commented Aug 16, 2019

haoliang7 commented Aug 25, 2019

haoliang7 commented Jun 27, 2019 •

edited

Loading

codecov-io commented Jun 27, 2019 •

edited

Loading

ashb commented Jun 28, 2019 •

edited

Loading