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-4731] Broken get_bucket kwarg in GCS hook #5368

Merged
merged 2 commits into from
Jun 10, 2019
Merged

[AIRFLOW-4731] Broken get_bucket kwarg in GCS hook #5368

merged 2 commits into from
Jun 10, 2019

Conversation

chronitis
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Description

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

google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.

Upstream change at googleapis/google-cloud-python#7856

Tests

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

Updated one test in test_gcs_hook

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

@codecov-io
Copy link

Codecov Report

Merging #5368 into master will not change coverage.
The diff coverage is 88.88%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5368   +/-   ##
=======================================
  Coverage   79.03%   79.03%           
=======================================
  Files         480      480           
  Lines       30129    30129           
=======================================
  Hits        23813    23813           
  Misses       6316     6316
Impacted Files Coverage Δ
airflow/contrib/hooks/gcs_hook.py 70.89% <88.88%> (ø) ⬆️

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 d1626d8...911f489. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jun 4, 2019

Codecov Report

Merging #5368 into master will increase coverage by 0.03%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5368      +/-   ##
==========================================
+ Coverage   79.02%   79.05%   +0.03%     
==========================================
  Files         481      481              
  Lines       30191    30218      +27     
==========================================
+ Hits        23858    23889      +31     
+ Misses       6333     6329       -4
Impacted Files Coverage Δ
airflow/contrib/hooks/gcs_hook.py 70.89% <88.88%> (ø) ⬆️
airflow/models/taskinstance.py 92.61% <0%> (+0.17%) ⬆️
airflow/contrib/operators/ssh_operator.py 83.54% <0%> (+1.26%) ⬆️
airflow/operators/docker_operator.py 99.12% <0%> (+2.57%) ⬆️

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 0da976a...488dc80. Read the comment docs.

@ashb
Copy link
Member

ashb commented Jun 5, 2019

Ugh thanks Google.

Could you include a note about this change in our UPDATING.md please (some people may be using the package directly.)

Gordon Ball added 2 commits June 5, 2019 13:20
google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.
@ashb
Copy link
Member

ashb commented Jun 5, 2019

Oh - could we safely make the calls positional (as you've done) but leave the version constraint as it is? Then we would reduce the chance of this breaking other code.

Or do you think it's better to force the update of the dep?

@chronitis
Copy link
Contributor Author

The dependency version doesn't need to be forced, if you'd prefer it not be.

The tradeoff is tricky - the existing ~= 1.14 will mean new installs will use 1.16 or later, so upgrading the requirement forces this upgrade at the same time as we're notifying of this bug, rather than it being encountered at some arbitrary later time. But it does force an upgrade which is not technically necessary - the changes will quite happily work with 1.14.

@potiuk
Copy link
Member

potiuk commented Jun 9, 2019

I think it's quite OK with bumping up the dependency. I don't see any downside of it, and this is indeed valid point with ~ installing 1.16.

@ashb ashb merged commit 201e671 into apache:master Jun 10, 2019
@chronitis chronitis deleted the airflow-4731 branch June 10, 2019 10:56
ashb pushed a commit that referenced this pull request Jun 10, 2019
google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.

(cherry picked from commit 201e671)
@chronitis chronitis restored the airflow-4731 branch June 11, 2019 11:38
@chronitis chronitis deleted the airflow-4731 branch June 26, 2019 07:28
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
)

google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
)

google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 8, 2019
)

google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.

(cherry picked from commit 201e671)
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 14, 2019
)

google-storage-client 1.16 introduced a breaking change where the
signature of client.get_bucket changed from (bucket_name) to
(bucket_or_name). Calls with named arguments to this method now fail.
This commit makes all calls positional to work around this.

(cherry picked from commit 201e671)
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