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

Jump to bottom

fix: revise blob.compose query parameters if_generation_match #454

Merged
merged 16 commits into from Jun 14, 2021
Merged

fix: revise blob.compose query parameters if_generation_match #454

merged 16 commits into from Jun 14, 2021

Conversation

cojenco
Copy link
Contributor

@cojenco cojenco commented Jun 5, 2021 •
edited

This changes if_generation_match, if_metageneration_match, if_source_generation_match for blob.compose() to fit API behavior

  • Add pending deprecation warning in cases where if_generation_match and if_metageneration_match is passed in as a list of long. Add conditional handling for backwards compatibility.
  • Update if_generation_match to be correctly included within the API request query parameters if it is type long. This makes the operation conditional on whether the composed object's current generation matches the given value.
  • Add if_source_generation_match as an optional parameter that serves as sourceObjects preconditions. This is sent with the API request body. When provided, the composition only performs if the generation of the source object that would be used matches this value.
  • Revise docstring, example and tests

Fixes #453🦕

@cojenco cojenco requested a review from a team June 5, 2021 01:19
@cojenco cojenco requested a review from a team as a code owner June 5, 2021 01:19
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Jun 5, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 5, 2021
tseaver
tseaver suggested changes Jun 7, 2021
View reviewed changes
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
google/cloud/storage/blob.py Show resolved Hide resolved
google/cloud/storage/blob.py Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
@cojenco cojenco added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 7, 2021
@tseaver
Copy link
Contributor

tseaver commented Jun 8, 2021

@cojenco I will merge / repair the tests broken by my PRs this week.

@google-cla
Copy link

google-cla bot commented Jun 8, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 8, 2021
@tseaver
Copy link
Contributor

tseaver commented Jun 8, 2021

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 8, 2021
@tseaver tseaver removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 8, 2021
@tseaver tseaver mentioned this pull request Jun 8, 2021
Closed
@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 8, 2021
@tseaver
Copy link
Contributor

tseaver commented Jun 8, 2021

@cojenco I put back the do not merge label to allow us to consider whether this might be a breaking change.

@cojenco
Copy link
Contributor Author

cojenco commented Jun 8, 2021

Got it, thanks @tseaver!

@cojenco cojenco requested a review from andrewsg June 8, 2021 19:42
@cojenco
Copy link
Contributor Author

cojenco commented Jun 8, 2021

This currently will introduce a breaking change. Revising PR for backwards compatibility.

@snippet-bot
Copy link

snippet-bot bot commented Jun 10, 2021

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@cojenco
Copy link
Contributor Author

cojenco commented Jun 10, 2021

Updated PR to handle backwards compatibility. Ready for review:)

tseaver
tseaver suggested changes Jun 11, 2021
View reviewed changes
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
@tseaver tseaver mentioned this pull request Jun 11, 2021
Merged
andrewsg
andrewsg requested changes Jun 11, 2021
View reviewed changes
google/cloud/storage/blob.py Outdated
Setting to 0 makes the operation succeed only if there are no live
versions of the object.

If a list of long is passed in, makes the operation conditional on whether the
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend phrasing this something like:

"Note: In a previous version, this argument worked identically to the if_source_generation_match argument. For backwards-compatibility reasons, if a list is passed in, this argument will behave like if_source_generation_match and also issue a DeprecationWarning."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I like this phrasing a lot, thanks!

google/cloud/storage/blob.py Show resolved Hide resolved
google/cloud/storage/blob.py Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
@cojenco cojenco removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 11, 2021
@cojenco cojenco requested a review from andrewsg June 14, 2021 16:10
andrewsg
andrewsg approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@andrewsg andrewsg left a comment

Choose a reason for hiding this comment

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

Thanks!

@cojenco cojenco merged commit 70d19e7 into master Jun 14, 2021
@cojenco cojenco deleted the obj-compose-retry branch June 14, 2021 22:31
cojenco added a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
@cojenco @tseaver @gcf-owl-bot
fix: revise blob.compose query parameters if_generation_match (goog…
af6fc2a 
…leapis#454)

* revise blob.compose logic to match API usage

* update tests

* update system test

* address comments

* 🦉 Updates from OwlBot

* revise logic for backwards compatibility

* add tests

* revise docstring

* fix test

* revise to DeprecationWarning

* address comments and revise docstrings

Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
cojenco added a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
@cojenco @tseaver @gcf-owl-bot
fix: revise blob.compose query parameters if_generation_match (goog…
b2b711e 
…leapis#454)

* revise blob.compose logic to match API usage

* update tests

* update system test

* address comments

* 🦉 Updates from OwlBot

* revise logic for backwards compatibility

* add tests

* revise docstring

* fix test

* revise to DeprecationWarning

* address comments and revise docstrings

Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
@release-please release-please bot mentioned this pull request Jan 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver approved these changes

@andrewsg andrewsg andrewsg approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change blob.compose to fit API behavior
3 participants
@cojenco @tseaver @andrewsg