-
Notifications
You must be signed in to change notification settings - Fork 151
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
feature: V4 Post policies #87
Conversation
This reverts commit f56440b.
@frankyn, I've pushed a real PR. Only system tests are failing with Everything except this seems to be fine |
Thanks @IlyaFaer, I'll review. I'll update group in V4 Sig Hangouts Chat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience @IlyaFaer, added a few more comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more nit, and will ping @crwilcox for his Python expertise.
@IlyaFaer yes, keep it in the same file to reduce complexity of conformance tests. |
@IlyaFaer could you update conformance tests to follow the exact format of source conformance tests: https://github.com/googleapis/conformance-tests/blob/master/storage/v1/v4_signatures.json It can be a subsequent PR to reduce complexity for this one. I'm trying to prevent snowflakes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed a few nits in yesterday's review. Thanks for consolidating conformance tests @IlyaFaer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pending KokoroCI passing and approval from @crwilcox
@frankyn, you mean, use these @crwilcox, @frankyn, I think, I saw random conformance tests are flakely failing in Python 2.7 and 3.5 while it all was in progress. I'll take a closer look. |
Hi @IlyaFaer,
Yes, that's correct, could you please open a tracking issue for it as well? Thank you.
Conformance tests should not have flakes, please keep us posted. |
It seems there are a few kokoro failures. @IlyaFaer are these expected or is this work to do yet? |
@frankyn, @crwilcox, I've found out what was causing flaky: So I've added I've fixed system tests as well (they were failing with I hope nothing is forgotten. All checks are green now 🎉 |
Awesome. Yep @IlyaFaer ordered dicts weren't in 2.7 at all or even older versions of 3. |
* feat: add POST policies building method * add comments, ignoring x-ignore fields and required fields validation * fix docs style, add virtual hosted style URLs * add bucket_bound_hostname support * cosmetic changes * add unit tests * Revert "add unit tests" This reverts commit f56440b. * add few lines from the old implementation for consistency * add some system tests * move system tests into separate class * fix credentials scope URL mistake * fix unit tests * fix algorithm name * add an example * add access token support * add credentials as an argument * rename method * add conformance tests into client unit tests * align conformance tests with test data * add an ability to set expiration as integer * update conformance tests to avoid problems with json spaces and timestamp Z-symbol violation * update implementation to avoid Z symbol isoformat violation and json whitespaces encoding * fix error with bounded hostnames * fix problem with bounded hostnames in implementation * fix conformance tests * fix problems: ascii encoding of signature and fields order * change asserts order * fix conformance tests * fix encoding issues * cosmetic changes and adding conformance tests * fix russion "C" letter in comment * add conformance tests data * cosmetic changes * cosmetic changes * add fields sorting * fix system tests Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
* feat: add POST policies building method * add comments, ignoring x-ignore fields and required fields validation * fix docs style, add virtual hosted style URLs * add bucket_bound_hostname support * cosmetic changes * add unit tests * Revert "add unit tests" This reverts commit f56440b. * add few lines from the old implementation for consistency * add some system tests * move system tests into separate class * fix credentials scope URL mistake * fix unit tests * fix algorithm name * add an example * add access token support * add credentials as an argument * rename method * add conformance tests into client unit tests * align conformance tests with test data * add an ability to set expiration as integer * update conformance tests to avoid problems with json spaces and timestamp Z-symbol violation * update implementation to avoid Z symbol isoformat violation and json whitespaces encoding * fix error with bounded hostnames * fix problem with bounded hostnames in implementation * fix conformance tests * fix problems: ascii encoding of signature and fields order * change asserts order * fix conformance tests * fix encoding issues * cosmetic changes and adding conformance tests * fix russion "C" letter in comment * add conformance tests data * cosmetic changes * cosmetic changes * add fields sorting * fix system tests Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Towards #8