Aws v4 refactor #113

denissimonovski · 2022-07-04T12:31:30Z

what

Refactoring the module to use the S3 bucket module
Use an AWS provider > v4.9.x
Use terraform version > v1.0

why

So that we can continue to build newer features into this module

references

closes #104
TF AWS provider 4.X compatibility #104

…m-aws-codebuild into aws-v4-refactor

denissimonovski · 2022-07-07T15:49:33Z

@Nuru I saw your comment here, and I made the refactoring to use the s3 module as you mentioned. Can you review the PR, and help me with the bridgecrew issue?

denissimonovski · 2022-07-08T16:11:21Z

@nitrocode can you help me with this PR?

versions.tf

nitrocode · 2022-07-12T02:04:41Z

/test all

nitrocode · 2022-07-12T02:07:05Z

main.tf

@@ -2,53 +2,44 @@ data "aws_caller_identity" "default" {}

 data "aws_region" "default" {}

-resource "aws_s3_bucket" "cache_bucket" {
+module "cache_bucket" {


Usually when we create a breaking change like this, we release a "minor" release but we also include with it some kind of "migration" document which we create by first applying the current release, then running through terraform state moves and other API changes, then we can apply the new version (this version) and make sure it returns "no changes".

Those steps are documented inside of docs/ and added to the README.yaml so other people will know how to safely migrate from version 0.x to version 0.y.

I ran the tests below just to make sure you were on the correct path with the current proposed changes but I think a migration doc is the right way to go.

cc: @Nuru

I added a migration doc @nitrocode
Can you please review?

…ebuild into aws-v4-refactor

Nuru

Thank you for all your work.

I have requested several changes, hopefully self-explanatory. The main questions I have are

Is AWS provider version 4.22 really required? I think 4.9 should be good enough, and want to allow for older versions where possible.
Am I correct that the S3 bucket is only used for cache and should not contain data that cannot be easily replaced? If so, let's just recommend the easy upgrade path where the S3 bucket gets destroyed and recreated.

Nuru · 2022-07-14T23:30:49Z

docs/migration-v1-v2.md

@@ -0,0 +1,14 @@
+## Migration Notes for Dynamic Subnets v2.0


Nuru · 2022-07-14T23:31:10Z

docs/migration-v1-v2.md

@@ -0,0 +1,14 @@
+## Migration Notes for Dynamic Subnets v2.0
+
+### &nbsp; Breaking change: S3 cache bucket


remove   throughout

Nuru · 2022-07-14T23:32:44Z

examples/bitbucket/variables.tf

@@ -288,13 +288,13 @@ variable "logs_config" {
 }

 variable "extra_permissions" {
-  type        = list
+  type        = list(any)


Suggested change

type = list(any)

type = list(string)

Nuru · 2022-07-14T23:33:33Z

examples/complete/versions.tf


  required_providers {
    aws = {
      source  = "hashicorp/aws"
-      version = ">= 2.0"
+      version = ">= 4.22.0"


What was changed added after 4.9.0 that requires a later version?

Suggested change

version = ">= 4.22.0"

version = ">= 4.9.0"

Nuru · 2022-07-14T23:35:12Z

main.tf

+  count       = module.this.enabled && local.create_s3_cache_bucket ? 1 : 0
+  bucket_name = local.cache_bucket_name_normalised
+
+  acl                = true


Suggested change

acl = true

acl = "private"

Nuru · 2022-07-14T23:41:16Z

main.tf

+      enabled                                = true
+      id                                     = "codebuildcache"
+      abort_incomplete_multipart_upload_days = 1
+      prefix                                 = "/"


prefix is not a recognized element of lifecycle_configuration_rules. If needed, add to filter_and instead, but since prefix is / for the rule, no filter is needed.

Suggested change

prefix = "/"

Nuru · 2022-07-14T23:44:28Z

versions.tf


  required_providers {
    aws = {
      source  = "hashicorp/aws"
-      version = ">= 2.0"
+      version = ">= 4.22.0"


Suggested change

version = ">= 4.22.0"

version = ">= 4.9.0"

Nuru · 2022-07-14T23:47:39Z

docs/migration-v1-v2.md

+* Do not upgrade if using Terraform older than 1.0.
+* Do not upgrade if you are using AWS provider older than 4.22.


Why does this require AWS Terraform provider version later than 4.9?

Suggested change

* Do not upgrade if using Terraform older than 1.0.

* Do not upgrade if you are using AWS provider older than 4.22.

This module now requires Terraform version 1.0 or later and

AWS Terraform provider version 4.9 or later.

Nuru · 2022-07-14T23:53:24Z

docs/migration-v1-v2.md

+The upgrade is two faceted. 
+1. Update terraform to a version equal to or newer than version 1.0. You can follow the **[Upgrading to Terraform v1.0](https://www.terraform.io/language/upgrade-guides/1-0)** guide.
+2. Update the AWS provider to the latest version. You can follow the S3 bucket modules **[Upgrading to v2.0](https://github.com/cloudposse/terraform-aws-s3-bucket/wiki/Upgrading-to-v2.0)** guide.


Suggested change

The upgrade is two faceted.

1. Update terraform to a version equal to or newer than version 1.0. You can follow the **[Upgrading to Terraform v1.0](https://www.terraform.io/language/upgrade-guides/1-0)** guide.

2. Update the AWS provider to the latest version. You can follow the S3 bucket modules **[Upgrading to v2.0](https://github.com/cloudposse/terraform-aws-s3-bucket/wiki/Upgrading-to-v2.0)** guide.

If you are using Terraform v1.0 or later, upgrading to v2 of this module without taking any other steps will delete your existing S3 bucket and create a new one. Since the S3 bucket is only used for caching data, this may be acceptable, and is certainly the easiest path forward.

If you want to preserve the existing S3 bucket and its data, you will need to use `terraform state mv` to move the existing resources to their new "addresses" in the Terraform state. You can find the details of how to do this in the S3 bucket module's **[Upgrading to v2.0](https://github.com/cloudposse/terraform-aws-s3-bucket/wiki/Upgrading-to-v2.0)** guide.

Is this really necessary?
If the bucket is not empty, terraform will error out on bucket deletion, so no data will be lost. If it is empty the bucket will be replaced, but at that state it shouldn't be a problem.

@Nuru , can you take a look?

@denissimonovski I think it is necessary, because the bucket module and the resource this PR replaces it with had force_destroy set to true.

Nuru · 2022-07-15T00:00:21Z

/test all

…es in main

denissimonovski · 2022-07-21T08:31:13Z

@nitrocode can you please run the tests again?

nitrocode · 2022-07-22T13:07:23Z

examples/complete/variables.tf

@@ -1,6 +1,7 @@
 variable "region" {
  type        = string
-  description = "AWS region"
+  default     = ""


This isnt right.

Please see the s3 bucket

https://github.com/cloudposse/terraform-aws-s3-bucket/blob/caf2af9a03f947ae51348e675146f6475ca228a3/examples/complete/main.tf#L1-L9

The awsutils provider needs to be set with a region in examples/complete/main.tf just like above

And only the examples/complete/versions.tf file needs to be updated with this

https://github.com/cloudposse/terraform-aws-s3-bucket/blob/caf2af9a03f947ae51348e675146f6475ca228a3/examples/complete/versions.tf#L13

@nitrocode the var.region on the provider is already set as you can see here and the region is in the examples .tfvars. And the awsutils provider is not required in this module at the moment. If you say I should put it in the example, shouldn't I put it in the module as well? Given that the example only deploys this module and nothing else.

By using the latest s3 bucket module the awsutils provider is needed

denissimonovski · 2022-07-26T16:19:08Z

@nitrocode can you please re-run the tests now?

denissimonovski · 2022-08-02T20:04:49Z

@nitrocode can you please re-run the tests?

joe-niland · 2022-08-02T20:15:51Z

/test all

…m-aws-codebuild into aws-v4-refactor

denissimonovski · 2022-08-09T14:31:11Z

@joe-niland can you please re-run the tests?

joe-niland · 2022-08-09T18:50:54Z

/test all

denissimonovski · 2022-08-10T17:37:07Z

@joe-niland can you please re-run the tests again 🙏 ❔

joe-niland · 2022-08-11T00:23:47Z

/test all

nitrocode · 2022-08-11T02:59:23Z

Looks like the error on the list() function is due to an old module in examples/vpc/main.tf

terraform-aws-codebuild/examples/vpc/main.tf

Lines 13 to 15 in 6c27093

    
           module "subnets" { 
        
             source               = "cloudposse/dynamic-subnets/aws" 
        
             version              = "0.38.0"

You may need to bump the subnet and vpc modules. I would bump those to the latest and plan it locally to ensure you have the correct inputs set (It will be much faster doing that locally than bumping the version and requesting someone to rerun the tests).

denissimonovski · 2022-08-11T09:58:48Z

@nitrocode @joe-niland I made the changes and tested them locally this time, and the plan was successful:
Plan: 42 to add, 0 to change, 0 to destroy. Can you please re-run the tests now?

joe-niland · 2022-08-11T10:06:26Z

/test all

denissimonovski · 2022-08-11T12:57:08Z

@nitrocode, @joe-niland it passed all the tests now. I'll be waiting for feedback or for a merge.

denissimonovski · 2022-09-02T09:07:57Z

@nitrocode when can we expect a review/merge on this?

nitrocode

Looks good to me.

cc: @joe-niland @Nuru in case there is anything I missed. I'll leave the merging up to you folks.

denissimonovski · 2022-09-20T14:00:45Z

@joe-niland @Nuru when can we expect a review/merge on this?

denissimonovski · 2022-10-11T18:51:54Z

@nitrocode @joe-niland @Nuru when can we expect a review/merge on this?

joe-niland

@denissimonovski I've tried to address your last comment and also pointed out that we can remove the awsutil dependency now, as it has become optional in the s3-bucket module.

joe-niland · 2022-10-12T06:07:16Z

examples/vpc/versions.tf

+      source  = "hashicorp/aws"
+      version = ">= 4.9.0"
+    }
+    awsutils = {


@denissimonovski we can remove this (and the provider and other usage) now, as of this PR. Sorry for the run-around, but we may as well fix this now.

joe-niland · 2022-10-12T06:20:16Z

docs/migration-v1-v2.md

+The upgrade is two faceted. 
+1. Update terraform to a version equal to or newer than version 1.0. You can follow the **[Upgrading to Terraform v1.0](https://www.terraform.io/language/upgrade-guides/1-0)** guide.
+2. Update the AWS provider to the latest version. You can follow the S3 bucket modules **[Upgrading to v2.0](https://github.com/cloudposse/terraform-aws-s3-bucket/wiki/Upgrading-to-v2.0)** guide.


@denissimonovski I think it is necessary, because the bucket module and the resource this PR replaces it with had force_destroy set to true.

max-lobur · 2023-11-10T21:22:57Z

Superseded by AWS V5 Upgrade

nitrocode · 2023-11-11T14:24:27Z

Related pr #126

Thanks @denissimonovski @max-lobur and the reviewers for making it possible to migrate to v4 and v5 provider versions

Denis Simonovski added 2 commits July 4, 2022 14:25

refactor module to use aws provider 4.21.0

74f0073

remove comments

262efce

denissimonovski requested review from a team as code owners July 4, 2022 12:31

denissimonovski requested review from adamcrews and milldr and removed request for a team July 4, 2022 12:31

cloudpossebot and others added 3 commits July 4, 2022 12:32

Auto Format

5859338

terraform fmt

5fa3013

Merge branch 'aws-v4-refactor' of github.com:denissimonovski/terrafor…

0ca2673

…m-aws-codebuild into aws-v4-refactor

nitrocode reviewed Jul 10, 2022

View reviewed changes

versions.tf Outdated Show resolved Hide resolved

Denis Simonovski and others added 4 commits July 11, 2022 15:47

update terraform version to >= 1.0 and aws version in examples

d74b938

Auto Format

60b2bb2

update terraform version to >= 1.0 in examples

4755c25

update terraform version to >= 1.0 in examples

e690d54

denissimonovski requested a review from nitrocode July 11, 2022 14:05

nitrocode reviewed Jul 12, 2022

View reviewed changes

versions.tf Show resolved Hide resolved

nitrocode reviewed Jul 12, 2022

View reviewed changes

Nuru added the major Breaking changes (or first stable release) label Jul 13, 2022

Denis Simonovski added 2 commits July 13, 2022 22:42

add a migration doc

d98ff72

Merge branch 'master' of github.com:denissimonovski/terraform-aws-cod…

4fc2d1f

…ebuild into aws-v4-refactor

denissimonovski requested a review from nitrocode July 13, 2022 20:54

bateller previously approved these changes Jul 14, 2022

View reviewed changes

Nuru previously requested changes Jul 14, 2022

View reviewed changes

update readme, examples, migration docs, and remove some default valu…

6e7ef98

…es in main

nitrocode reviewed Jul 22, 2022

View reviewed changes

add awsutils to examples

274ced3

Denis Simonovski and others added 4 commits August 2, 2022 22:24

fix bucket name reference

6b09b65

Auto Format

270de7e

fix bucket name reference

44a51f3

mErge branch 'aws-v4-refactor' of github.com:denissimonovski/terrafor…

76a2067

…m-aws-codebuild into aws-v4-refactor

fix logging target bucket

e50293e

fix vpc example

78522f1

joe-niland requested a review from nitrocode August 11, 2022 22:58

nitrocode approved these changes Sep 9, 2022

View reviewed changes

joe-niland reviewed Oct 12, 2022

View reviewed changes

adamantike mentioned this pull request May 27, 2023

Update Terraform cloudposse/ecs-codepipeline/aws to v0.33.0 cloudposse/terraform-aws-ecs-web-app#225

Merged

max-lobur closed this Nov 10, 2023

		@@ -0,0 +1,14 @@
		## Migration Notes for Dynamic Subnets v2.0

		###   Breaking change: S3 cache bucket

		* Do not upgrade if using Terraform older than 1.0.
		* Do not upgrade if you are using AWS provider older than 4.22.

Aws v4 refactor #113

Aws v4 refactor #113

Conversation

denissimonovski commented Jul 4, 2022 • edited Loading

what

why

references

denissimonovski commented Jul 7, 2022

denissimonovski commented Jul 8, 2022

nitrocode commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nuru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nuru commented Jul 15, 2022

denissimonovski commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denissimonovski commented Jul 26, 2022

denissimonovski commented Aug 2, 2022

joe-niland commented Aug 2, 2022

denissimonovski commented Aug 9, 2022

joe-niland commented Aug 9, 2022

denissimonovski commented Aug 10, 2022

joe-niland commented Aug 11, 2022

nitrocode commented Aug 11, 2022

denissimonovski commented Aug 11, 2022

joe-niland commented Aug 11, 2022

denissimonovski commented Aug 11, 2022

denissimonovski commented Sep 2, 2022

nitrocode left a comment

Choose a reason for hiding this comment

denissimonovski commented Sep 20, 2022

denissimonovski commented Oct 11, 2022

joe-niland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-lobur commented Nov 10, 2023

nitrocode commented Nov 11, 2023

denissimonovski commented Jul 4, 2022 •

edited

Loading