provider/aws: Add aws_codedeploy_app

[ctiwald]

This is (preliminary) support for CodeDeploy. It's sort of useless by itself but is a complete resource in its own right. I'd rather submit these for review and merge one at a time than dropping a giant patch that represents 3-4 distinct resources, even if they're most useful when used together.

I also haven't written the rest yet.

• Docs
• Create / Update / Delete an application all supported
• Acceptance tests

I saw @radeksimko post a test plan over in #2636, so in the interest of submitting a complete PR:

Test plan

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSCodeDeployApp' 2>/dev/null
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSCodeDeployApp -timeout 90m
=== RUN TestAccAWSCodeDeployApp_basic
--- PASS: TestAccAWSCodeDeployApp_basic (1.56s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    1.563s

[apparentlymart]

After a first skim this certainly looks plausible to me. At first I doubted that this object could really be this simple, but confirmed in the API documentation! :)

Just one concern: AWS has several different, competing products that each have a separate "application" concept: CodeDeploy, Elastic Beanstalk, and OpsWorks. Thus to avoid confusion we should perhaps call this resource aws_codedeploy_app. I'm (slowly) working through OpsWorks over in #1892 and have been using an aws_opsworks_ prefix, planning to call the application object aws_opsworks_app.

[ctiwald]

@apparentlymart -- That's a simple fix. I'll take care of it today. Sorry about that. Didn't realize there was a namespace collision.

[ctiwald]

@apparentlymart -- Updated the resource name to avoid a namespace collision. Here's the new test plan:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSCodeDeployApp' 2>/dev/null
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSCodeDeployApp -timeout 90m
=== RUN TestAccAWSCodeDeployApp_basic
--- PASS: TestAccAWSCodeDeployApp_basic (1.56s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    1.563s

[radeksimko]

@ctiwald Thanks for the test plan, it's just a convention I use to make reviewer's life easier.

👍 for the name change.

On the first sight it looks ok. I don't have much experience with CodeDeploy, but reviewing this PR may be a good chance to learn more. 😸 I should have some time by the end of this week, so I may have a look at it (unless someone else jumps in).

Sometimes it's ok to send a PR (yet multiple commits) which covers multiple resources. If you want to add some other resources that would make the CodeDeploy integration more useful, feel free.
It will then make reviewing & testing much easier, because we can bring up something that actually works purely via Terraform, debate about the cross-resource interaction too and not worry about backward compatibility.

[ctiwald]

@radeksimko -- sure. I'm about halfway done with the deployment group code and I can push it to this branch. At this point I think I can commit to delivering CodeDeploy in its entirety over the next week or so. It's simple, only a couple of resources, and really quite mundane as new resources go.

[ctiwald]

I'm still committed to finishing this off. Work's been a bit of a beast lately but I've got the CodeDeploy DeploymentGroup logic almost done, and after that there's very little left to make this a full-fledged, ready-to-go resource set. Just wanted to post an update. I've not forgotten about this.

[ctiwald]

Just an update on this. Code is done. I've been having trouble testing but it's related to CodeDeploy's somewhat irrational IAM requirement. Basically I just can't get the IAM permission right. As soon as I do I should have a complete PR here. Apologies for the long delay.

[nwalke]

@ctiwald Do you need help with IAM permissions stuff for this? I've set up CodeDeploy on a few accounts now and have a pretty good understanding about what's required.

[ctiwald]

@nwalke -- actually that'd be great. Sorry this has languished so long. Work's gotten in the way.

I've got the code for the deployment group and application resources written. I just need a valid service_role IAM policy for my integration test. From there I can basically submit, quite quickly, those two resources,

[ctiwald]

And that's basically the whole thing. DeploymentConfig should be trivial to create as a resource.

[ctiwald]

Actually, @nwalke, I got it sorted here. I've got a minor test failure I need to correct but I'll see if I can get this PR up for real within the next two days.

[nwalke]

@ctiwald Awesome. Would be nice to have this.

[ctiwald]

Alright @nwalke, this is all but done. I've got one minor issue to fix in the update of deployment groups but otherwise it's functional for both app resources and deployment groups. I am, however, having an irritating integration test problem.

@radeksimko -- I'm hoping you can point me at some prior art or other advice. The problem is this: You can't create a deployment group without an IAM role ARN. It's a required API field. But there's a small, couple second propagation time when creating IAM roles. The API call will error if the role isn't in place. Is there any way for me to create the roll as part of a setup stage in the integration test, and tear it down at the end? I just need a couple seconds of delay to ensure the role propagates and CodeDeploy can use it.

[nwalke]

@ctiwald Good work, that was quick...

I'm not sure if this is much help, but I've definitely seen that same problem reading through the changelog. Here's an example, not sure if it helps you or not: https://github.com/hashicorp/terraform/pull/3061/files

[radeksimko]

@ctiwald

But there's a small, couple second propagation time when creating IAM roles. The API call will error if the role isn't in place.

This is a problem to be solved for users too, not only for tests. I remember solving this for ECS service:
MeredithCorpOSS@fad019e

Since you're only using role by itself, that should solve the problem for you.

---

In case you'd be using instance profile, which uses that policy though, then you have to define the relationship between the policy & the resource using the instance profile, see MeredithCorpOSS@9c2a3e7

FYI: This is a general problem with IAM policies, which I have raised with AWS, but there's no ETA nor guarantee they will ever solve this.

[ctiwald]

Thanks, @nwalke -- this was basically 95% done so it wasn't much to get it to 99%.

@radeksimko -- that'll work. I'm incorporating it now.

[ctiwald]

Just an update here: Got the retry working just fine. Grappling with one little hiccup in the updating logic. Will try to crank it out tonight and no later than tomorrow.

[ctiwald]

@radeksimko -- I'm struggling a bit with the update logic here. Hoping you might know some similar resource. The problem is the unique identifier provided by CodeDeploy isn't used by any subsequent API call. To update a deployment group, you need to provide the app name and the deployment group name... but you can update the deployment group name.

This means that, for example, we have to update the ID with every update (if it includes those two dimensions), or use the unique ID and ... somehow... avoid terraform plan updating the state's deployment_group_name, which I don't think is possible.

I do have a workaround but I want to run it by you before I submit it: I could just force new if the deployment_group_name updates. The API technically supports changing a deployment group name, but as far as I can tell, there's absolutely no harm whatsoever in simply recreating the resource rather than updating it. The ID passed back by the API is meaningless. You can't do anything with it. So I don't think recreating the resource would break any other custom tools written by terraform users.

Thoughts? Let me know if this doesn't make sense. It's a rather obtuse problem.

[ctiwald]

I'm just going to roll forward with the above suggestion. If it proves unsuitable it can always be patched.

[ctiwald]

@radeksimko -- I think this is ready for your review. We're missing the deployment configuration resource but this, as written, gets ASG users 99% of the way there. I'll try to add it later this week but there's really no reason at this point not to test this and get it merged.

[radeksimko]

Thanks @ctiwald ,
I'm not quite sure I understand fully the problem with IDs, but it may just be because I was not looking into the code and still need to learn a bit more about how CodeDeploy works.

I truly appreciate clean git log and used commit messages 👍 😃
I am on holiday till the end of this week, but I may have a look at it during the weekend or the next one.

[apparentlymart]

Sorry this is a nit I should've spotted earlier, but since we renamed the resource to be app this argument should probably now be called app_name for consistency.

[apparentlymart]

@ctiwald rather than sending you on another change/review iteration, I just made a new commit to address my application_name nit and then merged it along with your changes. Thanks for working on this!

[apparentlymart]

Looks like there was a bug I didn't catch in the doc pages here that causes the pages to be mis-formatted. I'm looking at that now...

[apparentlymart]

Doc pages fixed up in 932f0dd.

@phinze the doc pages for these resources are currently broken on the live Terraform site. Is it possible to re-render the docs based on this fixup commit, without doing another release?

[phinze]

@apparentlymart yep - we maintain release/vX.Y.Z branches for this purpose. I'll cherry pick that commit over and repush the site.

[apparentlymart]

@phinze cool thanks. Sorry for the miss on that one... reviewed the content, but didn't test the rendering. Will be more diligent next time!

[ghost]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.