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.

Sign up for GitHub

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

Jump to bottom

fix(storage): add content type, disposition and encoding in v4 storage post policy #2769

Merged
merged 9 commits into from Sep 17, 2020
Merged

fix(storage): add content type, disposition and encoding in v4 storage post policy #2769

merged 9 commits into from Sep 17, 2020

Conversation

pierreis
Copy link
Contributor

This merge request adds content type, disposition and encoding in storage post policy. These conditions were present in the PolicyV4Fields, but ignored.

Fixes #2767.

@pierreis pierreis requested a review from tritone as a code owner August 24, 2020 10:42
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2020
tritone
tritone reviewed Aug 24, 2020
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

@pierreis thanks for your contribution!

@frankyn can you confirm that these fields are supposed to be present in the post policy? If so, are we missing conformance test cases for these?

@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2020
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Aug 25, 2020
@frankyn
Copy link
Member

frankyn commented Sep 3, 2020

Hi @pierreis,

Thanks for finding this gap!

I will need to update our conformance tests to include these fields (https://github.com/googleapis/conformance-tests/tree/master/storage/v1) before we can merge this in.

I'll keep you posted.

After the conformance tests are updated then you'll need to update the following file:

@tritone
Copy link
Contributor

tritone commented Sep 3, 2020

Hi @pierreis,

Thanks for finding this gap!

I will need to update our conformance tests to include these fields (https://github.com/googleapis/conformance-tests/tree/master/storage/v1) before we can merge this in.

I'll keep you posted.

After the conformance tests are updated then you'll need to update the following file:

Thanks @frankyn . I'm happy to manage getting the conformance test in when you have it ready.

tritone added a commit to tritone/google-cloud-go that referenced this pull request Sep 15, 2020
@tritone
test(storage): update post policy v4 conformance
11a0dfe 
This was recently added to conformance-tests to catch the case
that was noticed in googleapis#2767. We can remove the skip in the open
PR meant to fix that issue (googleapis#2769).
@tritone tritone mentioned this pull request Sep 15, 2020
Merged
tritone added a commit that referenced this pull request Sep 15, 2020
@tritone
test(storage): update post policy v4 conformance (#2860)
0dc7cea 
This was recently added to conformance-tests to catch the case
that was noticed in #2767. We can remove the skip in the open
PR meant to fix that issue (#2769).
@tritone
Copy link
Contributor

tritone commented Sep 15, 2020

I added in the required conformance test. @pierreis could you update this PR to remove the skip here: https://github.com/googleapis/google-cloud-go/blob/master/storage/conformance_test.go#L45 ? Then we can verify that this change makes the conformance test pass.

@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2020
@pierreis
Copy link
Contributor Author

@tritone Thank you for the updates! I see that you have already pushed the changes 🙂

@tritone
Copy link
Contributor

tritone commented Sep 16, 2020

@tritone Thank you for the updates! I see that you have already pushed the changes 🙂

Yes, I want to get this released soon so figured I'd push it along. :) Thanks again for noticing the issue and sending a PR!

The conformance test is failing currently due to how the conditions are sorted. Following up to see whether the test or the code needs to change here. I should have a fix soon.

tritone added a commit to tritone/google-cloud-go that referenced this pull request Sep 16, 2020
@tritone
test(storage): copy latest conformance test fixes
02be820 
Pick up fixes to conformance tests that cover encoding edge
cases and condition ordering for V4 signature and post policy.
This will unblock googleapis#2769.
@tritone tritone mentioned this pull request Sep 16, 2020
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Sep 16, 2020
@tritone
test(storage): copy latest conformance test fixes (#2873)
8728cdf 
Pick up fixes to conformance tests that cover encoding edge
cases and condition ordering for V4 signature and post policy.
This will unblock #2769.
@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2020
@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2020
codyoss
codyoss approved these changes Sep 17, 2020
View reviewed changes
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

LGTM

@tritone tritone added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Sep 17, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit ca3a6e2 into googleapis:master Sep 17, 2020
tritone added a commit to tritone/google-cloud-go that referenced this pull request Sep 18, 2020
@tritone
fix(storage): complete fix for PostPolicy fields
7359eba 
In googleapis#2769 we neglected to add these conditions to the returned
policyFields that the end user uses to populate the request. This
resulted in an integration test failing. This PR fixes the issue.

Fixes googleapis#2882
@tritone tritone mentioned this pull request Sep 18, 2020
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Sep 18, 2020
@tritone
fix(storage): complete fix for PostPolicy fields (#2887)
19d25d0 
In #2769 we neglected to add these conditions to the returned
policyFields that the end user uses to populate the request. This
resulted in an integration test failing. This PR fixes the issue.

Fixes #2882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankyn frankyn frankyn approved these changes

@tritone tritone tritone approved these changes

@codyoss codyoss codyoss approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

storage: content-type condition does not appear in generated V4 post policy
5 participants
@pierreis @frankyn @tritone @codyoss @kokoro-team