-
Notifications
You must be signed in to change notification settings - Fork 81
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: Support generating presigned url for UploadPart operation #1809
Conversation
After 9e3244e the generated URL's are correct and uploading works :). Will look into writing tests for this now. |
...n/software/amazon/smithy/aws/swift/codegen/customization/UploadPartPresignedURLMiddleware.kt
Outdated
Show resolved
Hide resolved
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.
Thank you for submitting this PR. I added couple pointers & comments.
Feel free to lmk if you'd rather have me take up the PR. I'm happy to do either (review your PR and have you drive it through to merge, or take up the work from you going forward).
...n/software/amazon/smithy/aws/swift/codegen/customization/UploadPartPresignedURLMiddleware.kt
Outdated
Show resolved
Hide resolved
...tware/amazon/smithy/aws/swift/codegen/customization/presignable/PresignableUrlIntegration.kt
Outdated
Show resolved
Hide resolved
...tware/amazon/smithy/aws/swift/codegen/customization/presignable/PresignableUrlIntegration.kt
Outdated
Show resolved
Hide resolved
...tware/amazon/smithy/aws/swift/codegen/customization/presignable/PresignableUrlIntegration.kt
Outdated
Show resolved
Hide resolved
...are/amazon/smithy/aws/swift/codegen/middleware/UploadPartPresignedURLMiddlewareRenderable.kt
Outdated
Show resolved
Hide resolved
Thank you for your feedback, I'm planning to look into it today. |
I hope I have addressed all your comments correctly. Once everything looks correct to you, I'd start looking into getting some test coverage on these changes. I have integrated and tested these changes in my Vapor project, and the generated presigned url's are still working correctly. |
Everything looks good! I'll take a second look when integration test is added for the presigned URL for uploadPart 👍 Just as a pointer, here's where you could add a test case for uploadPart: S3PresigndRequestTests. Note that because the |
Decided to add a completely new file for the integration test, hope that makes sense. Also covered the other method that didn't have a test case yet ( |
I triggered the CI run for the PR and it seems because this comes from a fork (either that or bc PR is created by someone outside of team, or perhaps both), it can't read the secrets tied to repo that's used to run integration tests in CI (see here). I might have to put up a new PR with your changes to run CI & merge. |
Thank you! You're right - a fork cannot access the secrets of the upstream repository for security reasons. |
Thank you for your help and guidance! You can close this PR at any time in favour of the other one you're going to open. |
Closing PR; work for it is tracked in #1835. |
The PR clone (#1835) was merged. It should go out with today's release 👍 |
It adds methods to generate a presigned url for the
UploadPart
operation.Issue
Fixes #1808, #723
Description of changes
It generates two new methods:
UploadPartInput.presignURL(config:expiration:)
S3Client.presignedURLForUploadPart(input:expiration:)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.