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

feat: add storage_type parameter #79

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Conversation

adubeniuk
Copy link
Contributor

what

Amazon has announced IO-optimized storage type for DocumentDB. Support for it has been added since HashiCorp AWS provider version 5.29.0

why

Keep standard as default but also add ability to create IO-optimized DocumentDB clusters.

references

https://aws.amazon.com/about-aws/whats-new/2023/11/amazon-documentdb-i-o-optimized/
https://registry.terraform.io/providers/hashicorp/aws/5.29.0/docs/resources/docdb_cluster#storage_type

@adubeniuk adubeniuk requested review from a team as code owners December 8, 2023 10:36
@vsenk0as
Copy link

@Gowiem @florian0410
Hi!
Could you please reconsider this request, as the changes are minimal, but this feature is quite important for cost optimization.
Thanks!

@cbonilla-liquid
Copy link

Hi! Any estimation when this will be ready? Thanks

@awb-i4o
Copy link

awb-i4o commented Jan 25, 2024

We urgently need this PR to be merged and really would appreciate if progress can be made.

Thank you.

@Gowiem
Copy link
Member

Gowiem commented Jan 25, 2024

/terratest

@Gowiem
Copy link
Member

Gowiem commented Jan 26, 2024

Hey folks, quick update: We're looking into why our CI didn't run and then we'll get this approved and merged. Apologies for the delay and thanks for the pings. Going forward if you do want some functionality like this merge, feel free to ping us in the SweetOps slack via the #pr-reviews channel and someone will be faster to get to you.

Side note -- @adubeniuk, since this is being asked for by a few folks I just want to move it along, but I would normally request that you add a validation block to your new variable since it has an explicit set of values. See here for an example: https://github.com/cloudposse/terraform-aws-documentdb-cluster/blob/main/context.tf#L86-L89. If you can add that in the near term before we get the CI check figured out... that may kick the CI check when you do a push which would be a win-win

@adubeniuk
Copy link
Contributor Author

hi @Gowiem
added the necessary validation
thanks

@Gowiem
Copy link
Member

Gowiem commented Jan 26, 2024

/terratest

Gowiem
Gowiem previously approved these changes Jan 26, 2024
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

LGTM

@Gowiem
Copy link
Member

Gowiem commented Jan 26, 2024

Hey @adubeniuk, unfortunately, we need to run some automation to pass our tests. Mind doing the following locally, committing the result, and pushing to your branch?

make init
make readme

Thanks!

@adubeniuk
Copy link
Contributor Author

hi @Gowiem, updated Readme, thanks

@Gowiem
Copy link
Member

Gowiem commented Jan 26, 2024

/terratest

@Gowiem Gowiem enabled auto-merge (squash) January 26, 2024 17:45
@Gowiem Gowiem merged commit 88a68fd into cloudposse:main Jan 26, 2024
10 checks passed
@Gowiem
Copy link
Member

Gowiem commented Jan 26, 2024

@adubeniuk thanks for the hard work! This has been released as https://github.com/cloudposse/terraform-aws-documentdb-cluster/releases/tag/0.25.0 👍

cc @awb-i4o @cbonilla-liquid @vsenk0as

@vsenk0as
Copy link

@Gowiem
thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants