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

feat: add if*generation*Match support, pt1 #123

Merged
merged 21 commits into from May 15, 2020

Conversation

IlyaFaer
Copy link

@IlyaFaer IlyaFaer commented May 1, 2020

Add if_generation_match, if_generation_not_match, if_metageneration_match and if_metageneration_not_match arguments into methods:

Client(): lookup_bucket(), get_bucket()

Bucket(): get_blob(), delete_blob(), delete(), exists(), patch(), reload(), update()

Blob(): delete(), download_to_file(), download_to_filename(), download_as_string(), rewrite(), update_storage_class(), exists(), patch(), reload(), update(), reload()

Towards #127

@IlyaFaer IlyaFaer added api: storage Issues related to the googleapis/python-storage API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels May 1, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 1, 2020
@IlyaFaer IlyaFaer requested a review from frankyn May 1, 2020 10:36
@IlyaFaer IlyaFaer marked this pull request as ready for review May 1, 2020 10:36
@IlyaFaer IlyaFaer changed the title feat: add ifMetageneration*Match support, pt1 feat: add if*generation*Match support, pt1 May 1, 2020
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Client-side checks for if_*generation checks should not be added for this case.

Please remove use of _raise_for_more_than_one_none and let the service handle those errors.

google/cloud/storage/_helpers.py Outdated Show resolved Hide resolved
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.

Looks like the system tests are in part 2. So that we don't merge an incomplete change, please go ahead and merge part 2 into this PR and close that one. It will be a large PR but that's better than risking merging one and having the other delayed.

google/cloud/storage/_helpers.py Show resolved Hide resolved
google/cloud/storage/_helpers.py Show resolved Hide resolved
Copy link
Author

@IlyaFaer IlyaFaer left a comment

Choose a reason for hiding this comment

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

In fact, parts 1 and 2 are completely separated from each other, I've just tried to make PRs easier to read. Anyway after erasing the checks changes become much more shorter, so I suppose we can merge part 2 into this one.

google/cloud/storage/_helpers.py Show resolved Hide resolved
google/cloud/storage/_helpers.py Show resolved Hide resolved
@IlyaFaer
Copy link
Author

IlyaFaer commented May 12, 2020

Lint session failed, but the problem is not related to this PR:

Running session lint
Creating virtual environment (virtualenv) using python3.8 in .nox/lint
pip install flake8 black==19.10b0
black --check docs google tests noxfile.py setup.py
All done! ✨ 🍰 ✨
35 files would be left unchanged.
flake8 google tests
google/cloud/storage/_signing.py:156:13: F523 '...'.format(...) has unused arguments at position(s): 0
Command flake8 google tests failed with exit code 1
Session lint failed.

@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2020
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 Ilya. This looks great, only thing to note is that it looks like the metageneration match could use a system test. I don't think there's utility in making a system test for each modified function, but adding a system test for the metageneration match in addition to the generation might be a sweet spot for effort/reward. Should be good to go after that's in.

@IlyaFaer
Copy link
Author

I've added more system tests, and re-reviewed it all once again - fixed a couple of nits.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise LGTM. Pending @andrewsg's comments.

google/cloud/storage/blob.py Outdated Show resolved Hide resolved
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

LGTM, pending @andrewsg's approval.

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!

@frankyn frankyn merged commit 0944442 into googleapis:master May 15, 2020
@IlyaFaer IlyaFaer deleted the metageneration_match_pt1 branch May 15, 2020 07:49
IlyaFaer pushed a commit to MaxxleLLC/python-storage that referenced this pull request May 20, 2020
* feat: add ifMetageneration*Match support, pt1

* fix unit tests, add test for helper

* fix unit tests

* add generation match args into more methods

* feat: add if*generation*Match support, pt2

* Lint fix.

* delete "more than one set "checks

* del excess import

* delete "more than one set" checks

* rename the helper; add error raising in case of wront parameters type

* add more system tests

* system tests fixes

* cleanup system test

* fix comments

* delete excess checks

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* feat: add ifMetageneration*Match support, pt1

* fix unit tests, add test for helper

* fix unit tests

* add generation match args into more methods

* feat: add if*generation*Match support, pt2

* Lint fix.

* delete "more than one set "checks

* del excess import

* delete "more than one set" checks

* rename the helper; add error raising in case of wront parameters type

* add more system tests

* system tests fixes

* cleanup system test

* fix comments

* delete excess checks

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* feat: add ifMetageneration*Match support, pt1

* fix unit tests, add test for helper

* fix unit tests

* add generation match args into more methods

* feat: add if*generation*Match support, pt2

* Lint fix.

* delete "more than one set "checks

* del excess import

* delete "more than one set" checks

* rename the helper; add error raising in case of wront parameters type

* add more system tests

* system tests fixes

* cleanup system test

* fix comments

* delete excess checks

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants