FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feature: V4 Post policies by IlyaFaer · Pull Request #87 · googleapis/python-storage · GitHub

This repository was archived by the owner on Mar 31, 2026. It is now read-only.
/ python-storage Public archive

feature: V4 Post policies - #87

Merged
crwilcox merged 43 commits into
googleapis:masterfrom
MaxxleLLC:v4_post_policies
Apr 1, 2020
Merged

feature: V4 Post policies#87
crwilcox merged 43 commits into
googleapis:masterfrom
MaxxleLLC:v4_post_policies

Conversation

Copy link
Copy Markdown

Towards #8

IlyaFaer added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Mar 16, 2020
googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 16, 2020
Comment thread google/cloud/storage/client.py Outdated
Comment thread tests/unit/test_client.py
IlyaFaer requested a review from frankyn March 16, 2020 15:07
IlyaFaer marked this pull request as ready for review March 16, 2020 15:07

IlyaFaer commented Mar 16, 2020
edited
Loading

Copy link
Copy Markdown
Author

@frankyn, I've pushed a real PR. Only system tests are failing with Anonymous caller does not have storage.objects.create access to <bucket-name>/<file-name> (the same Jonathan Lui got)

Everything except this seems to be fine

frankyn commented Mar 16, 2020

Copy link
Copy Markdown
Contributor

Thanks @IlyaFaer, I'll review. I'll update group in V4 Sig Hangouts Chat.

frankyn requested a review from crwilcox March 17, 2020 06:01

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Small nits

Comment thread google/cloud/storage/_signing.py Outdated
Comment thread google/cloud/storage/client.py Outdated

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for your patience @IlyaFaer, added a few more comments.

Comment thread google/cloud/storage/_signing.py Outdated
Comment thread google/cloud/storage/client.py Outdated

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One more nit, and will ping @crwilcox for his Python expertise.

IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 23, 2020
Comment thread google/cloud/storage/client.py Outdated

Copy link
Copy Markdown
Author

@frankyn, @crwilcox, I've moved conformance tests into this PR. I assume you'd like me to add the conformance tests data as well!? Locally I've added policy conformance tests data into url_signer_v4_test_data.json, and used it while running the tests. Is it a correct place?

frankyn commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

@IlyaFaer yes, keep it in the same file to reduce complexity of conformance tests.

frankyn commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

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

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I missed a few nits in yesterday's review. Thanks for consolidating conformance tests @IlyaFaer

Comment thread google/cloud/storage/client.py Outdated
IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2020
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2020

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Few more nits.

Comment thread tests/unit/test_client.py Outdated
Comment thread google/cloud/storage/client.py Outdated

frankyn left a comment
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, pending KokoroCI passing and approval from @crwilcox

IlyaFaer commented Mar 31, 2020
edited
Loading

Copy link
Copy Markdown
Author

follow the exact format of source conformance tests

@frankyn, you mean, use these signingV4Tests and postPolicyV4Tests fields on a first level of the json? That's not a problem, though I assume some code should be changed in test__signing.py as well, so yes, that probably should be done in another PR.

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

frankyn commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

Hi @IlyaFaer,

@frankyn, you mean, use these signingV4Tests and postPolicyV4Tests fields on a first level of the json? That's not a problem, though I assume some code should be changed in test__signing.py as well, so yes, that probably should be done in another PR.

Yes, that's correct, could you please open a tracking issue for it as well? Thank you.

I think, I saw random conformance tests are flakely failing in Python 2.7 and 3.5. Will take a closer look.

Conformance tests should not have flakes, please keep us posted.

Copy link
Copy Markdown
Contributor

It seems there are a few kokoro failures. @IlyaFaer are these expected or is this work to do yet?

frankyn commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

@crwilcox, @IlyaFaer raised that are some flakes. It looks like system tests and not conformance tests, but I might be incorrect. Pending follow-up.

Thanks for reviewing this PR @crwilcox!

IlyaFaer commented Apr 1, 2020
edited
Loading

Copy link
Copy Markdown
Author

@frankyn, @crwilcox, I've found out what was causing flaky: dict.items() returns result in different order sometimes:


So I've added sorted() for fields - didn't detect any new failing on 2.7/3.5 so far.

I've fixed system tests as well (they were failing with Anonymous requests are not allowed for some time, but now they're okay, so I've fixed a couple of my bads).

I hope nothing is forgotten. All checks are green now 🎉

crwilcox commented Apr 1, 2020

Copy link
Copy Markdown
Contributor

Awesome. Yep @IlyaFaer ordered dicts weren't in 2.7 at all or even older versions of 3.

crwilcox merged commit b451e2d into googleapis:master Apr 1, 2020
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* 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>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

5 participants


Back | FazBrowse Home | New Git URL