Update Terraform cloudposse/ecs-codepipeline/aws to v0.33.0

[adamantike]

trafficstars

Breaking changes

This release introduces a breaking change that affects projects using the github_webhooks_token variable.

Since Terraform 1.2.0, modules to be used with the for_each, count, and depends_on arguments must not contain their own provider configurations. The github_webhooks_token variable was being used to instantiate a github provider, which must now be instantiated and provided separately.

If your project is affected by this change, the following steps will allow you to upgrade to this version:

1. Add the GitHub provider to your project:

   terraform {
     required_providers {
       github = {
         source  = "integrations/github"
         version = ">= 4.2.0"
       }
     }
   }
   

2. Instantiate the provider (feel free to use any other supported Authentication mechanism):

   provider "github" {
     owner = var.repo_owner
     token = "{your_github_token}"
   }
   

3. Stop providing the github_webhooks_token variable to this module.

what

• Removed variables no longer available in the cloudposse/ecs-codepipeline/aws module.
• Supersedes #202
• Fixes #227

why

• Version 0.32.0 upgrades the cloudposse/repository-webhooks/github module, which removed the hardcoded github provider, so some variables have been removed in favor of instantiating the provider separately.

references

• Upstream issue: https://github.com/cloudposse/terraform-github-repository-webhooks/issues/37
• Upstream pull request: https://github.com/cloudposse/terraform-github-repository-webhooks/pull/40

[getglad]

@Gowiem @korenyoni - is it possible to get this reviewed or landed? Would be helpful to have this in main

[Gowiem]

Pulling in @joe-niland since he's an expert on this module. Joe, figured you may have an opinion here.

[Gowiem]

/test all

[Gowiem]

@adamantike @getglad -- This looks like a solid contribution, thanks! I'm checking with the rest of the maintainer team on if this should be a major version rev or not. I believe it should, but semver is followed differently everywhere and it's we've just started rev'ing past v0.x.y, so it's good to check. Will follow up when we can move this forward 👍

[joe-niland]

Pulling in @joe-niland since he's an expert on this module. Joe, figured you may have an opinion here.

Thanks for the ping @Gowiem. I don't use this module that much anymore, but I did test this branch and it was fine after I made the adjustments below.

Since this introduces a breaking change, I think we need to include information about how to change a consuming module in the PR description (example). As far as I can see, it's just:

• unsetting the github_webhooks_token variable
• adding a github provider block
• updating the github provider source and version

[Gowiem]

@joe-niland good call on the breaking changes info and including that in the PR description. @adamantike do you mind providing that info into your description and then we'll get this merged and cut a release? Here is a great PR example of what that should look like: https://github.com/cloudposse/terraform-aws-s3-bucket/pull/158

[mihaiplesa]

Looking forward to get this in which would fix the below on Terraform 1.4.x on arm64 macOS.

│ Error: Incompatible provider version
│ 
│ Provider registry.terraform.io/hashicorp/github v3.0.0 does not have a
│ package available for your current platform, darwin_arm64.

[getglad]

@max-lobur @Gowiem @joe-niland friendly re-bump on this.

It sounds like this was down to documentation on the PR description, but now seeing some failures around a pinning to https://github.com/cloudposse/terraform-null-label.git?ref=tags/0.19.2 in a subnet module.

Any ideas on how we can get it unblocked and merged?

[Gowiem]

@getglad we (@max-lobur specifically) just did a sweeping change to our automation to add tflint, so things like that are sadly a necessary evil that we need to deal with on open PRs. We'll need @adamantike to provide the PR description updates that are discussed above and fix the tflint issues you mentioned.

@adamantike friendly ping on this. Mind updating the above?

Alternatively if you're looking to get this merged @getglad, feel free to fork this PR's branch, make the necessary updates, put up another PR, and request me as a reviewer. We'll get this moved along quickly in that case.

[adamantike]

Sorry for not tackling this before. I have rebased my changes, and updated the PR description to include the information about the breaking change and how to upgrade to the new minor version!

[Gowiem]

/terratest

[Gowiem]

@adamantike sorry to continue to drag this out, but we've got some lint issues and terraform failures due to the moved block somewhere in the chain. The moved block means we need to update the required terraform version in either the module itself or in the examples directories. Mind checking into those and we'll see if we can get this PR green and shipped?

[adamantike]

It's probably the same issue I needed to fix in the complete example, because of very old versions of dynamic-subnets and vpc modules being used. Let me try to fix those ASAP.

[Gowiem]

/terratest

[Gowiem]

@adamantike can you bump this line to v1.3+? https://github.com/cloudposse/terraform-aws-ecs-web-app/blob/main/examples/complete/versions.tf#L2

[adamantike]

Just updated required_version in the examples to use at least 1.1.0, as that's the minimum required version in the dynamic-subnets module (reference)

[Gowiem]

/terratest

[Gowiem]

The fun never ends:

│ Error: Unsupported block type
│ 
│   on .terraform/modules/ecs_web_app.ecs_codepipeline.codebuild/main.tf line 414, in resource "aws_codebuild_project" "default":
│  414:     dynamic "auth" {
│ 
│ Blocks of type "auth" are not expected here.

Mind looking into that @adamantike?

[adamantike]

That error will require more coordination with other modules. The issue is that cloudposse/terraform-aws-codebuild uses source.auth dynamic blocks (reference), which were removed from the AWS provider version 5 (notes from the upgrade guide).

I'm not familiar with how well maintained that module is, but consider that it has an open issue and two open PRs (1, 2) from 2022, regarding better support for AWS provider version 4.

A temporary fix would be to limit the AWS provider in this module, to: version = ">= 3.34, < 5.0". That will unfortunately affect users that don't even use CodePipeline. Is there any better option to apply in the meantime?

[adamantike]

I have created https://github.com/cloudposse/terraform-aws-codebuild/pull/123, in case we want to go with the upstream fix. It will require releasing a new version for cloudposse/codebuild/aws and cloudposse/ecs-codepipeline/aws, to then refresh this PR.

[Gowiem]

@adamantike I think going with the option of limiting to > 5.0. AWS Provider is legit in this circumstance. At least until we get your other PR upstream in and then we can circle back around to this module, if that is of interest.

Want to make that update within versions.tf and we'll see how the tests do?

[Gowiem]

/terratest

[Gowiem]

@adamantike sorry to do it, mind running make init && make readme + commit that?

[Gowiem]

/terratest

[Gowiem]

/terratest

[Gowiem]

@adamantike haha this is hitting every bump in the road, I'm sorry. This is why large child modules like this aren't the best... but they do have their place so here we are.

Anyway, try bumping the ALB module to the latest tag (1.7.0): https://github.com/cloudposse/terraform-aws-ecs-web-app/blob/main/examples/complete/main.tf#L29C1-L30

[adamantike]

Yeah, it seems these examples haven't been tested by the CI in a while. But we will figure it out!

The PR is already using the latest version for the ALB module, so I will need to investigate if this is related to the S3 ACL defaults being changed by AWS last month.

[adamantike]

After debugging the current issue, I have found that we need to avoid the example from trying to create ACLs for the ALB's access logs bucket.

There are two options here, from simpler to more complex:

1. Change the example to set access_logs_enabled = false when instantiating the ALB module.
2. Send s3_object_ownership = "BucketOwnerEnforced" to the underlying cloudposse/s3-bucket/aws module, which disables the ACL creation (reference). This requires: a. Exposing that variable in cloudposse/lb-s3-bucket/aws and releasing a new version. I have already created a PR for this: https://github.com/cloudposse/terraform-aws-lb-s3-bucket/pull/67 b. Exposing the same variable, and bumping the cloudposse/lb-s3-bucket/aws module, in cloudposse-aws-alb. c. Bumping cloudposse-aws-alb in this module's examples, and setting s3_object_ownership = "BucketOwnerEnforced".

[Gowiem]

@adamantike option #2 sounds like good fixes and would be great contributions. I think if you're up for it, then continue down that path.

But in the mean time, since that use-case shouldn't matter for our example, let's go with option #1.

[mschfh]

That error will require more coordination with other modules. The issue is that cloudposse/terraform-aws-codebuild uses source.auth dynamic blocks (reference), which were removed from the AWS provider version 5 (notes from the upgrade guide).

The AWS provider v5 PR was merged and released as v2.0.0.