Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add wrapper support #246

Closed

Conversation

stephgosling
Copy link

@stephgosling stephgosling commented Oct 27, 2021

Description

This PR enables wrapper support such that the module can be used to create multiple resources without needing for_each. It's a blatant copy of the same in terraform-aws-s3-bucket

Motivation and Context

I'm a terragrunt user and I had need to create a handful of (very) stateful instances in EC2 so I couldn't use an ASG. instance_count went away with v3.x and terragrunt doesn't support for_each so here we are.

Breaking Changes

No breaking changes

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects

At the moment I am still testing this locally but it's looking promising. this terragrunt.hcl produces this plan

I have two questions:

  • when you merged 92 in terraform-aws-s3-bucket a pre-commit hook was added, I don't have the same here (yet). Is it required?
  • seems like the wrapper maintenance is going to be quite onerous, particularly if a module is changing frequently :(

@stephgosling stephgosling changed the title feature: add wrapper and README [WIP] Add wrapper support Oct 27, 2021
@antonbabenko
Copy link
Member

Hi Steph,

Good to see that someone found the wrapper concept useful for exactly same reasons as I had with S3 buckets and terragrunt!

There is already an open issue related to this - antonbabenko/pre-commit-terraform#202

Once there is a pre-commit hook that handles updates of the wrappers, it won't require manual changes from the individual developers.

PS: I will try to review this PR a bit later this week or during the next one. Please make it "ready for review" if you think you are done with it.

@stephgosling
Copy link
Author

OK cool I'll have a look at that hook then, if I pull it in and run it and the output is the same (certainly for the terraform if not the readme then I'll be confident this PR is ok. fwiw I span up two nodes and extended it to four on my fork but better to have the script confirm!

@stephgosling
Copy link
Author

Hi @antonbabenko sorry for the delay. I've copied the the pre-commit hook script and run it (it picked up two problems which I've fixed and pushed). I have one last query and that's regarding the text output. At the moment generate-terraform-wrappers.sh has readme output hardcoded as for the s3 bucket module which makes sense, as it was the first. In this PR I manually edited the readme for this module so the script complains. How do you want to handle that? fwiw the diff is:

diff --git a/wrappers/README.md b/wrappers/README.md
index 0c0d8d3..ac0d206 100644
--- a/wrappers/README.md
+++ b/wrappers/README.md
@@ -46,26 +46,25 @@ module "wrapper" {
 }
 ```
 
-## Example: Manage multiple EC2 instances in one Terragrunt layer
+## Example: Manage multiple S3 buckets in one Terragrunt layer
 
-`eu-west-1/ec2-instances/terragrunt.hcl`:
+`eu-west-1/s3-buckets/terragrunt.hcl`:
 
 ```hcl
 terraform {
-  source = "git::git@github.com:terraform-aws-modules/terraform-aws-ec2-instance.git?ref=master//wrappers"
+  source = "git::git@github.com:terraform-aws-modules/terraform-aws-s3-bucket.git?ref=master//wrappers"
 }
 
 inputs = {
   items = {
-    instance1 = {
-      name = "my-random-instance-1"
-      ami  = "ami-0123456789abcdef0"
+    bucket1 = {
+      bucket        = "my-random-bucket-1"
+      force_destroy = true
     }
-    instance2 = {
-      name = "my-random-instance-2"
-      ami  = "ami-fedcba9876543210f"
+    bucket2 = {
+      bucket        = "my-random-bucket-2"
+      force_destroy = true
     }
   }
 }
 ```
-

I can see editing that is going to be pretty gnarly as a shell script... the quickest way I can think is importing it all as template file or somesuch so the source for this text is independent of the repo?

@antonbabenko
Copy link
Member

You are right, the shell script is hardcoded for just the S3 bucket module. I didn't have the intention to make this script very flexible and generate examples for any modules because it would require several more input arguments.

Maybe we can just keep examples for S3 buckets and add a note in the script that source and inputs should be updated for the module accordingly?

@stephgosling stephgosling marked this pull request as ready for review November 22, 2021 16:21
@stephgosling stephgosling changed the title [WIP] Add wrapper support Add wrapper support Nov 22, 2021
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Jan 11, 2022
@github-actions
Copy link

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Jan 22, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants