r/aws_ebs_volume: Termination Snapshot

[johananl]

This PR addresses issue #2109 by @catsby (termination snapshot for EBS volumes).

[jen20]

Hi @johananl! I can't speak for the Terraform team with regards to the first question (I'm not aware of much additional functionality outside of the AWS API implemented like this), but I can provide some guidance on the second:

Testing in this case consists of a bunch of steps - create an instance with a volume, destroy it, and ensure that the termination snapshot is created. Also ensure that no snapshot is created if the option is not enabled (an existing test can likely be modified for this). You'll then want to tidy up the created snapshot so it doesn't sit around indefinitely. A good example might be the tests that attach a volume to an instance - you'll likely need multiple test steps.

[catsby]

Thanks for answering those, @jen20 !

An example pattern you could follow is with aws_db_instance and how it handles checking for the snapshot:

• https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_db_instance_test.go#L294

For now, I would suggest this PR/effort to focus on only ebs_volume. Nested volumes on aws_instance have their own peculiarities to work out that may just hinder getting this released

[johananl]

Hey @catsby. Just wanted to make sure you guys have noticed I removed the [WIP] from this PR :-)
I'm sorry it took me a while to implement - this is my first Terraform feature contribution and I wanted to do this right.

Any feedback is welcome!

[DanielMarquard]

Can this PR still be merged? I have a use case for this that will cut my costs substantially.

Thank you for your contribution, @johananl. 😃

[jen20]

Hi @DanielMarquard and @johananl! I've had a look through the code here and it looks good to me. However, it needs further input from someone at HashiCorp about whether they wish to implement things which don't map 1:1 to an available API function, and I don't want to merge this until that happens. Thanks for contributing!

[DanielMarquard]

It may not be a 1:1 API function, but I'm pretty sure it's supported in CloudFormation, so to me it would make sense to be included as part of Terraform.

Hope it gets approved. 😃

[johananl]

Thanks for the feedback @jen20 @DanielMarquard.

However, it needs further input from someone at HashiCorp about whether they wish to implement things which don't map 1:1 to an available API function, and I don't want to merge this until that happens.

Please see #2109 which was opened by @catsby from HashiCorp. I actually raised the same question before implementing this and the answer was - yes, we should implement it.

However, I agree it's better if someone from HashiCorp takes a look before merging.

[DanielMarquard]

Any input from Hashicorp on this merge request? We really need this at the Department of Justice, among other departments my company contracts to.

[johananl]

@catsby @bill-rich ?

[tareks]

Any updates on this? Looking at following this approach (similar to RDS) for certain EBS volumes. If it's not happening, then may need to consider another option.

[johananl]

@catsby @bill-rich - would love to get your feedback on this.

There is also a similar issue which can probably be resolved in a similar way. I'll go ahead and implement it as soon as this one is done.

If there are changes required I'd love to quickly push fixes. Waiting eagerly for your feedback :-)

[johananl]

Rebased over current master as things broke in the meantime.

CI is green. make testacc passes on my testing.

@catsby @bill-rich @aeschright I'm not losing hope! ;-)

[zhelding]

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

[ewbankkit]

Superseded by #11118.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.