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(storage): improve v4 signature query parameters encoding #48

Merged
merged 18 commits into from Mar 7, 2020

Conversation

@IlyaFaer
Copy link
Member

@IlyaFaer IlyaFaer commented Feb 7, 2020

Closes #9

google/cloud/storage/_signing.py Show resolved Hide resolved
google/cloud/storage/_signing.py Show resolved Hide resolved
tests/unit/test__signing.py Show resolved Hide resolved
google/cloud/storage/_signing.py Show resolved Hide resolved
@IlyaFaer IlyaFaer requested review from crwilcox and frankyn Feb 10, 2020
@IlyaFaer IlyaFaer marked this pull request as ready for review Feb 10, 2020
@frankyn
Copy link
Member

@frankyn frankyn commented Feb 11, 2020

@IlyaFaer this LGTM, but please update relevant conformance tests before merging this code to confirm that it's working as expected.

I updated conformance tests in https://github.com/googleapis/conformance-tests/tree/master/storage/v1

If you have questions PLMK. Specifically for this change you should rely on the following two conformance tests:

  • Query Parameter Encoding
  • Query Parameter Ordering

google/cloud/storage/_signing.py Show resolved Hide resolved
tests/unit/test__signing.py Show resolved Hide resolved
google/cloud/storage/_signing.py Outdated Show resolved Hide resolved
@tseaver
Copy link
Contributor

@tseaver tseaver commented Feb 12, 2020

This PR also addresses part of the change for #12.

@tseaver
Copy link
Contributor

@tseaver tseaver commented Feb 12, 2020

@frankyn We don't currently do any of the cross-language conformance tests inside python-storage. I just made #57 to track that.

@frankyn
Copy link
Member

@frankyn frankyn commented Feb 13, 2020

@IlyaFaer please respond to @tseaver's comments before merging

google/cloud/storage/_signing.py Outdated Show resolved Hide resolved
google/cloud/storage/_signing.py Show resolved Hide resolved
@frankyn
Copy link
Member

@frankyn frankyn commented Feb 20, 2020

Friendly ping and pending Kokoro passing.

@frankyn frankyn requested a review from tseaver Feb 26, 2020
@frankyn
Copy link
Member

@frankyn frankyn commented Feb 26, 2020

Merging when green.

@frankyn
Copy link
Member

@frankyn frankyn commented Feb 26, 2020

Pending approval from @tseaver as there is on-going discussion.

@frankyn
Copy link
Member

@frankyn frankyn commented Mar 6, 2020

@tseaver friendly ping

@crwilcox crwilcox dismissed tseaver’s stale review Mar 6, 2020

Reviewed, attempted to remove coercion, but due to truthiness of bytes and strs in python 2 I am going to approve this.

@crwilcox crwilcox removed the request for review from tseaver Mar 7, 2020
@crwilcox crwilcox merged commit 8df0b55 into googleapis:master Mar 7, 2020
3 checks passed
@IlyaFaer IlyaFaer deleted the v4_sign_query_params branch Apr 1, 2020
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
…apis#48)

* feat(storage): improve v4 signature query parameters encoding

* add URL encoding test

* add query_parameters arg into conformance tests

* add tests for _quote_param() function

* declare test file encoding

* fix the param type

* add test with bytes

* Update _signing.py

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
…apis#48)

* feat(storage): improve v4 signature query parameters encoding

* add URL encoding test

* add query_parameters arg into conformance tests

* add tests for _quote_param() function

* declare test file encoding

* fix the param type

* add test with bytes

* Update _signing.py

Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.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.

6 participants