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
Member

@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 requested a review from frankyn May 1, 2020
@IlyaFaer IlyaFaer marked this pull request as ready for review May 1, 2020
@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

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

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
Member Author

@IlyaFaer IlyaFaer left a comment

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
Member Author

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

Copy link
Contributor

@andrewsg andrewsg left a comment

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
Member Author

@IlyaFaer IlyaFaer commented May 14, 2020

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

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

LGTM, pending @andrewsg's approval.

Copy link
Contributor

@andrewsg andrewsg left a comment

Thanks!

@frankyn frankyn merged commit 0944442 into googleapis:master May 15, 2020
3 checks passed
@IlyaFaer IlyaFaer deleted the metageneration_match_pt1 branch May 15, 2020
IlyaFaer added a commit to q-logic/python-storage that referenced this issue 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 added a commit to cojenco/python-storage that referenced this issue 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 added a commit to cojenco/python-storage that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants