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

pass context to S3 and dynamoDB storage calls #27927

Merged

Conversation

bhowe34
Copy link
Contributor

@bhowe34 bhowe34 commented Jul 31, 2024

Description

What does this PR do?

Passes the request context to S3 and dynamoDB storage calls so they are cancelled when the request context is cancelled.

We have encountered instances where requests that have long been cancelled by the client are still processed by vault and result in increased storage requests. Passing the context will ensure these immediately return an error in this case so resources aren't wasted.

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

Hi there! I'm sorry this took some time to get eyes on. This looks good to me, and I'd be happy to merge it. The only thing that's missing to get this mergeable is a changelog for the PR.

This would be a file in the changelog directory called 27927.txt with a brief description of the change. It should be classified as an improvement and feel free to take inspiration from the examples within the folder.

@bhowe34
Copy link
Contributor Author

bhowe34 commented Sep 20, 2024

Added the changelog. I'm not sure if having multiple lines in the same block is allowed so let me know if the format needs changing.

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

I'm not quite sure how to suggest this via GitHub suggestion (sorry!) but could you split this up into two separate release-note:improvement blocks in the same file? As an example, look at 24667.txt. Once that's done, I'd be happy to merge this :)

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

I'm going to let the builds run and make sure everything looks good but unless there's any nasty surprises (I doubt it) then I expect to merge this Monday.

Thank you for the contribution!

@VioletHynes VioletHynes merged commit fc5ed22 into hashicorp:main Sep 23, 2024
64 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants