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.

Sign up for GitHub

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

Jump to bottom

[AIRFLOW-4255] Make GCS Hook Backwards compatible #5089

Merged
merged 5 commits into from
Apr 14, 2019
Merged

[AIRFLOW-4255] Make GCS Hook Backwards compatible #5089

merged 5 commits into from
Apr 14, 2019

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Apr 12, 2019 •
edited
Loading

Make sure you have checked all steps below.

Jira

Description

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

@kaxil kaxil requested a review from ashb April 12, 2019 13:31
ashb
ashb reviewed Apr 12, 2019
View reviewed changes
airflow/contrib/hooks/gcs_hook.py Outdated
"""

warnings.warn("'multipart' and 'num_retries' parameters have been deprecated."
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest passing multipart=None and then an if multipart is not None -- otherwise there is no way for the user to "fix" this warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Updated

@codecov-io
Copy link

codecov-io commented Apr 12, 2019 •
edited
Loading

Codecov Report

Merging #5089 into master will increase coverage by 0.15%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5089      +/-   ##
==========================================
+ Coverage   76.81%   76.97%   +0.15%     
==========================================
  Files         463      463              
  Lines       29757    29810      +53     
==========================================
+ Hits        22859    22946      +87     
+ Misses       6898     6864      -34
Impacted Files Coverage Δ
airflow/contrib/hooks/gcs_hook.py 70.44% <78.57%> (+16.79%) ⬆️
airflow/models/baseoperator.py 94.03% <0%> (-0.11%) ⬇️
airflow/bin/cli.py 66.66% <0%> (+0.13%) ⬆️
airflow/models/taskinstance.py 92.59% <0%> (+0.17%) ⬆️
airflow/www/views.py 76.38% <0%> (+0.35%) ⬆️
airflow/contrib/operators/qubole_operator.py 86.79% <0%> (+1.07%) ⬆️
airflow/contrib/hooks/qubole_hook.py 55% <0%> (+3.06%) ⬆️
airflow/hooks/dbapi_hook.py 88.79% <0%> (+3.19%) ⬆️

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 86422e7...f4d2db0. Read the comment docs.

@kaxil kaxil requested a review from Fokko April 13, 2019 21:14
Fokko
Fokko approved these changes Apr 14, 2019
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

LGTM

@Fokko Fokko merged commit c6efd01 into apache:master Apr 14, 2019
@Fokko
Copy link
Contributor

Fokko commented Apr 14, 2019

@kaxil Do you feel like cherry-picking this onto v1-10-test? There are lot of merge conflicts, and I find it hard to do since I didn't write the code.

@kaxil kaxil deleted the backport-gcs branch April 14, 2019 23:16
@kaxil
Copy link
Member Author

kaxil commented Apr 14, 2019 •
edited
Loading

Thanks @Fokko , yes will do it. Currently, v1-10-test and v1-10-stable look out of sync. Will talk with @ashb and cherry-pick those.

@ashb
Copy link
Member

ashb commented Apr 15, 2019 •
edited
Loading

Yeah - I hadn't been using/updating v1-10-test for the .3 release.

I've been using my own fork and making have use of push --force-with-lease. Give me a few hours and this should be easier to cherry-pick as I've got all the models re-org cherry-picked. Just need to fix a few failures from the old www tree.

ashb
ashb reviewed Apr 15, 2019
View reviewed changes
airflow/contrib/hooks/gcs_hook.py
@@ -265,6 +275,10 @@ def delete(self, bucket, object):
:param object: name of the object to delete
:type object: str
"""

if generation is not None:
warnings.warn("'generation' parameter is no longer supported", DeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

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

We should add a stacklevel=2 to all these warn calls - that way the warning will be reported with the filename/line number of the caller, rather than gcs_hook;py:280: generation' parameter is no longer supported

cthenderson pushed a commit to cthenderson/apache-airflow that referenced this pull request Apr 16, 2019
@kaxil
[AIRFLOW-4255] Make GCS Hook Backwards compatible (apache#5089)
05ea5bf 
* [AIRFLOW-4255] Make GCS Hook Backwards compatible

* Update UPDATING.md

* Add option to stop warnings

* Update test_gcs_hook.py

* Add tests
kaxil added a commit that referenced this pull request Apr 16, 2019
@kaxil
[AIRFLOW-4255] Make GCS Hook Backwards compatible (#5089)
f6a0faf 
* [AIRFLOW-4255] Make GCS Hook Backwards compatible

* Update UPDATING.md

* Add option to stop warnings

* Update test_gcs_hook.py

* Add tests
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
@kaxil @andriisoldatenko
[AIRFLOW-4255] Make GCS Hook Backwards compatible (apache#5089)
d93a0b7 
* [AIRFLOW-4255] Make GCS Hook Backwards compatible

* Update UPDATING.md

* Add option to stop warnings

* Update test_gcs_hook.py

* Add tests
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
@kaxil
[AIRFLOW-4255] Make GCS Hook Backwards compatible (apache#5089)
626cc84 
* [AIRFLOW-4255] Make GCS Hook Backwards compatible

* Update UPDATING.md

* Add option to stop warnings

* Update test_gcs_hook.py

* Add tests
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 8, 2019
@kaxil @dharamsk
[AIRFLOW-4255] Make GCS Hook Backwards compatible (apache#5089)
49a97de 
* [AIRFLOW-4255] Make GCS Hook Backwards compatible

* Update UPDATING.md

* Add option to stop warnings

* Update test_gcs_hook.py

* Add tests
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 14, 2019
@kaxil @dharamsk
[AIRFLOW-4255] Make GCS Hook Backwards compatible (apache#5089)
56a113e 
* [AIRFLOW-4255] Make GCS Hook Backwards compatible

* Update UPDATING.md

* Add option to stop warnings

* Update test_gcs_hook.py

* Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashb ashb ashb left review comments

@Fokko Fokko Fokko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kaxil @codecov-io @Fokko @ashb