feat(ecr): Add emptyOnDelete CloudFormation property to Repository L2 construct #28233

markmansur · 2023-12-02T03:01:42Z

Added emptyOnDelete prop to the ecr Repository construct. emptyOndelete is supported by CloudFormation
See here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecr-repository.html#cfn-ecr-repository-emptyondelete

I've also deprecated the autoDeleteImages prop that deployed a custom resource. According to #24572 this was added before CloudFormation added the EmptyOnDelete property here aws-cloudformation/cloudformation-coverage-roadmap#515

Closes #28196

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Thanks 👍
I left you some comments for some minor adjustments.

lpizzinidev · 2023-12-03T14:26:44Z

packages/aws-cdk-lib/aws-ecr/lib/repository.ts

@@ -706,6 +714,7 @@ export class Repository extends RepositoryBase {
      imageScanningConfiguration: props.imageScanOnPush !== undefined ? { scanOnPush: props.imageScanOnPush } : undefined,
      imageTagMutability: props.imageTagMutability || undefined,
      encryptionConfiguration: this.parseEncryption(props),
+      emptyOnDelete: props.emptyOnDelete,


We should ignore autoDeleteImages if emptyOnDelete is set here:

if (props.emptyOnDelete === undefined && props.autoDeleteImages) {

(I think that setting emptyOnDelete = false should override autoDeleteImages = true, but feel free to comment on this).

Also:

This will need some extra unit tests

Does emptyOnDelete = true requires removalPolicy === RemovalPolicy.DESTROY? I wasn't able to find documentation on this

I'm fine with only using autoDeleteImages if emptyOnDelete is undefined.

emptyOnDelete = true does not require removalPolicy === RemovalPolicy.DESTROY

Made the change and added tests, please review 🙂

markmansur · 2023-12-03T21:28:01Z

packages/aws-cdk-lib/aws-ecr/README.md


 ```ts
 const repository = new ecr.Repository(this, 'MyTempRepo', {
  removalPolicy: RemovalPolicy.DESTROY,
-  autoDeleteImages: true,
+  emptyOnDelete: true,
 });
 ```



Don't you think it's better to remove autoDeleteImages from the readme entirely? I've already marked it as deprecated in the JSDoc.

packages/@aws-cdk/app-staging-synthesizer-alpha/README.md

markmansur · 2023-12-03T21:28:01Z

packages/aws-cdk-lib/aws-ecr/README.md


 ```ts
 const repository = new ecr.Repository(this, 'MyTempRepo', {
  removalPolicy: RemovalPolicy.DESTROY,
-  autoDeleteImages: true,
+  emptyOnDelete: true,
 });
 ```



Don't you think it's better to remove autoDeleteImages from the readme entirely? I've already marked it as deprecated in the JSDoc.

lpizzinidev

Can you please provide documentation for

emptyOnDelete = true does not require removalPolicy === RemovalPolicy.DESTROY

?

lpizzinidev · 2023-12-04T13:25:50Z

packages/aws-cdk-lib/aws-ecr/README.md

@@ -176,12 +176,12 @@ policy is set to `RemovalPolicy.DESTROY`, the repository will be deleted as long
 as it does not contain any images.

 To override this and force all images to get deleted during repository deletion,
-enable the`autoDeleteImages` option.
+enable the`emptyOnDelete` option.

 ```ts
 const repository = new ecr.Repository(this, 'MyTempRepo', {
  removalPolicy: RemovalPolicy.DESTROY,


Suggested change

removalPolicy: RemovalPolicy.DESTROY,

Unnecessary.

markmansur · 2023-12-07T05:53:17Z

Can you please provide documentation for

emptyOnDelete = true does not require removalPolicy === RemovalPolicy.DESTROY

?

Actually I misspoke. I was testing this out with JSON CloudFormation when I should have been testing it with CDK.
By default CDK sets the RemovalPolicy to RETAIN which will override the emptyOnDelete setting. I was wrong, RemovalPolicy.DESTROY is required when using emptyOnDelete. I've updated the readme, sorry for the confusion 🙂

lpizzinidev

Thanks!
I think we're just missing one validation and we'll be good to go.

lpizzinidev · 2023-12-07T07:32:57Z

packages/aws-cdk-lib/aws-ecr/lib/repository.ts

@@ -723,7 +732,7 @@ export class Repository extends RepositoryBase {
      resourceName: this.physicalName,
    });

-    if (props.autoDeleteImages) {
+    if (props.emptyOnDelete === undefined && props.autoDeleteImages) {


Given #28233 (comment).
We should validate for emptyOnDelete as well:

if (props.emptyOnDelete) { if (props.removalPolicy !== RemovalPolicy.DESTROY) { throw new Error('Cannot use \'emptyOnDelete\' property on a repository without setting removal policy to \'DESTROY\'.'); } } if (props.emptyOnDelete === undefined && props.autoDeleteImages) { ... }

With a related unit test for coverage.

lpizzinidev

Thanks 👍

mergify · 2023-12-19T14:23:10Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-12-19T14:26:52Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-12-19T16:22:14Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-12-19T16:50:01Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: aa469ae
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-12-19T16:50:28Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Dec 2, 2023

aws-cdk-automation requested a review from a team December 2, 2023 03:01

aws-cdk-automation previously requested changes Dec 2, 2023

View reviewed changes

markmansur requested a review from aws-cdk-automation December 2, 2023 04:00

markmansur added 2 commits December 1, 2023 23:09

feat(ecr): Add emptyOnDelete CloudFormation property to L2 construct

b55035e

deprecate autoDeleteImages

dc5ac6d

markmansur force-pushed the feat/ecr-empty-on-delete branch from efa45be to 161767a Compare December 2, 2023 04:09

Update ecr readme with emptyOnDelete

5c3ee91

markmansur force-pushed the feat/ecr-empty-on-delete branch from 161767a to 5c3ee91 Compare December 2, 2023 05:14

fix app-staging-synthesizer-alpha integ tests

04296e6

markmansur force-pushed the feat/ecr-empty-on-delete branch from 91a08b1 to 04296e6 Compare December 2, 2023 17:04

integ tests

1dce578

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 2, 2023

lpizzinidev suggested changes Dec 3, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 3, 2023

markmansur commented Dec 3, 2023

View reviewed changes

markmansur added 2 commits December 3, 2023 16:28

fix readme typo

66cc90c

when emptyOnDelete is set, ignore autoDeleteImages

4eb9f74

lpizzinidev suggested changes Dec 4, 2023

View reviewed changes

markmansur added 5 commits December 6, 2023 23:19

Update README

74a0ffd

Update README

4fb8f12

Update integ tests

206161b

Update

2dc5034

Update readme

c3b8188

Update integ tests

c3165cb

lpizzinidev suggested changes Dec 7, 2023

View reviewed changes

Add check and test for removal policy when using emptyOnDelete

ecf6a46

markmansur requested a review from lpizzinidev December 9, 2023 05:14

lpizzinidev approved these changes Dec 9, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 9, 2023

sumupitchayan self-assigned this Dec 18, 2023

sumupitchayan previously approved these changes Dec 19, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023

Merge branch 'main' into feat/ecr-empty-on-delete

6aa73fa

kaizencc approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'main' into feat/ecr-empty-on-delete

3cbab65

Merge branch 'main' into feat/ecr-empty-on-delete

aa469ae

mergify bot merged commit a175da8 into aws:main Dec 19, 2023
10 checks passed

markmansur deleted the feat/ecr-empty-on-delete branch December 22, 2023 18:52

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecr): Add emptyOnDelete CloudFormation property to Repository L2 construct #28233

feat(ecr): Add emptyOnDelete CloudFormation property to Repository L2 construct #28233

markmansur commented Dec 2, 2023 •

edited

Loading

aws-cdk-automation left a comment

lpizzinidev left a comment

lpizzinidev Dec 3, 2023

markmansur Dec 3, 2023

markmansur Dec 3, 2023

markmansur Dec 3, 2023

lpizzinidev left a comment

lpizzinidev Dec 4, 2023

markmansur commented Dec 7, 2023

lpizzinidev left a comment

lpizzinidev Dec 7, 2023 •

edited

Loading

lpizzinidev left a comment

mergify bot commented Dec 19, 2023

mergify bot commented Dec 19, 2023

mergify bot commented Dec 19, 2023

aws-cdk-automation commented Dec 19, 2023

mergify bot commented Dec 19, 2023

feat(ecr): Add emptyOnDelete CloudFormation property to Repository L2 construct #28233

feat(ecr): Add emptyOnDelete CloudFormation property to Repository L2 construct #28233

Conversation

markmansur commented Dec 2, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Dec 3, 2023

Choose a reason for hiding this comment

markmansur Dec 3, 2023

Choose a reason for hiding this comment

markmansur Dec 3, 2023

Choose a reason for hiding this comment

markmansur Dec 3, 2023

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Dec 4, 2023

Choose a reason for hiding this comment

markmansur commented Dec 7, 2023

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 19, 2023

mergify bot commented Dec 19, 2023

mergify bot commented Dec 19, 2023

aws-cdk-automation commented Dec 19, 2023

AWS CodeBuild CI Report

mergify bot commented Dec 19, 2023

markmansur commented Dec 2, 2023 •

edited

Loading

lpizzinidev Dec 7, 2023 •

edited

Loading