cloudposse / terraform-aws-ecr

Terraform Module to manage Docker Container Registries on AWS ECR
https://cloudposse.com/accelerate
Apache License 2.0
185 stars 133 forks source link

Add support for time based rotation #132

Closed uhlajs closed 4 months ago

uhlajs commented 4 months ago

what

Add support for countType "sinceImagePushed" ECR Lifepolicy rule.

why

references

gberenice commented 4 months ago

/terratest

gberenice commented 4 months ago

@uhlajs the tests are failing:

Error: creating ECR Lifecycle Policy (eg-test-ecr-test-19516): operation error ECR: PutLifecyclePolicy, https response error StatusCode: 400, RequestID: 5ec05c35-e50c-4d13-b210-05f1e4c67bca, InvalidParameterException: Invalid parameter at 'LifecyclePolicyText' failed to satisfy constraint: 'Lifecycle policy validation failure: instance failed to match exactly one schema (matched 0 out of 2)

The issue arises from including null in the JSON structure, which AWS ECR lifecycle policy does not allow. We need to conditionally include or exclude the countUnit based on the var.time_based_rotation, like this:

      selection = merge(
        {
          tagStatus   = "any"
          countType   = (
            var.time_based_rotation ?
            "sinceImagePushed" :
            "imageCountMoreThan"
          )
          countNumber = var.max_image_count
        },
        var.time_based_rotation ? { countUnit = "days" } : {}
      )

I wonder if it makes sense to add an optional rule for time-based expiration instead of modifying an existing one. It can make sense to have both imageCountMoreThan and sinceImagePushed rules simultaneously, depending on the image management strategy.

Also, could you please add a fix to satisfy another failing check:

Running input-descriptions in /__w/terraform-aws-ecr/terraform-aws-ecr/examples/complete

Test: check if terraform inputs have descriptions
File: input-descriptions.bats
---------------------------------
region is missing a description
---------------------------------
uhlajs commented 4 months ago

@gberenice Thank you for quick response.

I wonder if it makes sense to add an optional rule for time-based expiration instead of modifying an existing one. It can make sense to have both imageCountMoreThan and sinceImagePushed rules simultaneously, depending on the image management strategy.

Agree, actually my colleague suggested same extension. I have modified the original PR so that both time_based_rotation and count_based_rotation are supported.

Also, could you please add a fix to satisfy another failing check:

Fixed.

Additionally, I have fixed linting.

uhlajs commented 4 months ago

It seems that AWS ECR doesn't support having multiple lifecycle policy validation:

Error: creating ECR Lifecycle Policy InvalidParameterException: Invalid parameter at 'LifecyclePolicyText' failed to satisfy constraint: 'Lifecycle policy validation failure: Only one rule can specify 'ANY' tags.'

gberenice commented 4 months ago

/terratest

uhlajs commented 4 months ago

Reverted to original design.

gberenice commented 4 months ago

/terratest

gberenice commented 4 months ago

@uhlajs thanks for trying out the second approach - good to know what options we have 👍 One small thing to fix before we're good to go, please run terraform fmt for examples/complete:

Running lint in /__w/terraform-aws-ecr/terraform-aws-ecr/examples/complete

Test: check if terraform code needs formatting
File: lint.bats
---------------------------------
variables.tf
---------------------------------

not ok 1 check if terraform code needs formatting
gberenice commented 4 months ago

/terratest

github-actions[bot] commented 4 months ago

These changes were released in v0.41.1.