Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add support for time based rotation #132

Merged
merged 11 commits into from
Jul 30, 2024

Conversation

uhlajs
Copy link
Contributor

@uhlajs uhlajs commented Jul 29, 2024

what

Add support for countType "sinceImagePushed" ECR Lifepolicy rule.

why

  • Increase flexibility and usefulness of this module.

references

@mergify mergify bot added the triage Needs triage label Jul 29, 2024
Honza Uhlík added 2 commits July 29, 2024 10:36
@uhlajs uhlajs marked this pull request as ready for review July 29, 2024 08:48
@uhlajs uhlajs requested review from a team as code owners July 29, 2024 08:48
@uhlajs uhlajs requested review from Gowiem and gberenice July 29, 2024 08:48
@gberenice
Copy link

/terratest

@gberenice
Copy link

@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
Copy link
Contributor Author

uhlajs commented Jul 30, 2024

@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
Copy link
Contributor Author

uhlajs commented Jul 30, 2024

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
Copy link

/terratest

@uhlajs
Copy link
Contributor Author

uhlajs commented Jul 30, 2024

Reverted to original design.

@gberenice
Copy link

/terratest

@gberenice
Copy link

@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
Copy link

/terratest

Copy link

@gberenice gberenice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uhlajs LGTM 👍

@gberenice gberenice merged commit 2a4b139 into cloudposse:main Jul 30, 2024
18 checks passed
@mergify mergify bot removed the triage Needs triage label Jul 30, 2024
Copy link
Contributor

These changes were released in v0.41.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants