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

(ecr): add option to auto delete images upon ECR repository removal #12618

Closed
1 of 2 tasks
kirintwn opened this issue Jan 20, 2021 · 17 comments · Fixed by #24572
Closed
1 of 2 tasks

(ecr): add option to auto delete images upon ECR repository removal #12618

kirintwn opened this issue Jan 20, 2021 · 17 comments · Fixed by #24572
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@kirintwn
Copy link
Contributor

kirintwn commented Jan 20, 2021

Use Case

ECR repositories currently do not get delete if they contain images even the removalPolicy is set to DESTROY.

It was reported in #2765, and was thought to be a responsibility of CloudFormation team aws-cloudformation/cloudformation-coverage-roadmap#515.

Proposed Solution

I do think we should add a option to force delete the images using custom resource provider, which is just like #12090 that uses a custom resource provider to delete objects in s3.

The code might looks like this:

const bucket = new ecr.Repository(this, 'Repo', {
  repositoryName: 'delete-even-if-contains-images',
  removalPolicy: cdk.RemovalPolicy.DESTROY,
  autoDeleteImages: true,
});

The props autoDeleteImages can only to be true if removalPolicy is set to DESTROY.

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@kirintwn kirintwn added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jan 20, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry label Jan 20, 2021
@kirintwn
Copy link
Contributor Author

I have created a branch to show how it works. Note that the unit tests are not completed because I think we better wait until #12620 get merged.

mergify bot pushed a commit that referenced this issue May 6, 2021
* use native jest API like what #12596 did
* move `removal policy is "Retain" by default` & `'grant adds appropriate resource-*'` tests out of `events` scope in `repository.test.ts`, which I believe they were misplaced

I'm doing this partially because I would like to implement the feature proposed in #12618. It would be easier to mock `aws-sdk` function calls with jest.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
john-tipper pushed a commit to john-tipper/aws-cdk that referenced this issue May 10, 2021
* use native jest API like what aws#12596 did
* move `removal policy is "Retain" by default` & `'grant adds appropriate resource-*'` tests out of `events` scope in `repository.test.ts`, which I believe they were misplaced

I'm doing this partially because I would like to implement the feature proposed in aws#12618. It would be easier to mock `aws-sdk` function calls with jest.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@MrArnoldPalmer
Copy link
Contributor

hey @kirintwn, would you be interested in publishing this as a separate module? There definitely is a need and it would be a good way to get some users testing it before merging into the CDK. It may be possible to extend the existing ecr.Repository class and intercept this prop and then create the custom resource.

@MrArnoldPalmer MrArnoldPalmer added effort/small Small work item – less than a day of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels May 17, 2021
@MrArnoldPalmer MrArnoldPalmer removed their assignment Jun 21, 2021
@heiba
Copy link

heiba commented Aug 7, 2021

@kirintwn any chance we can publish your branch if it's fully working ? we need this feature terribly.

@heiba
Copy link

heiba commented Aug 7, 2021

@MrArnoldPalmer is there anyway the CDK team can help on publishing this feature ?

@kirintwn
Copy link
Contributor Author

kirintwn commented Aug 7, 2021

Hi @heiba , I have updated my branch, added more tests & opened a PR #15932.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
* use native jest API like what aws#12596 did
* move `removal policy is "Retain" by default` & `'grant adds appropriate resource-*'` tests out of `events` scope in `repository.test.ts`, which I believe they were misplaced

I'm doing this partially because I would like to implement the feature proposed in aws#12618. It would be easier to mock `aws-sdk` function calls with jest.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@rix0rrr rix0rrr added p1 and removed p2 labels Mar 16, 2022
@nemmison
Copy link

nemmison commented May 4, 2022

Any update or workaround for this?

@mikaeelkhalidnbs
Copy link

do we have any updates on this feature?

@JessieWadman
Copy link

This would be very nice. Constantly running into issues because of lack of this.

@MikeGongolidis
Copy link

We would really appreciate such an option!

@kushal-ti
Copy link

We really need this feature as well, would be a huge help

@ozgenbaris1
Copy link

please add this feature

@amirse
Copy link

amirse commented Nov 10, 2022

indeed, please add.

@TheRealAmazonKendra TheRealAmazonKendra added p2 and removed p1 labels Jan 24, 2023
@TheRealAmazonKendra TheRealAmazonKendra added effort/medium Medium work item – several days of effort and removed effort/small Small work item – less than a day of effort labels Jan 24, 2023
@madeline-k
Copy link
Contributor

To whoever ends up implementing this, be mindful of this issue: #16603 We don't want to end up with the same problem.

@github-actions github-actions bot added p1 and removed p2 labels Jan 29, 2023
@github-actions
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@mergify mergify bot closed this as completed in #24572 Mar 21, 2023
mergify bot pushed a commit that referenced this issue Mar 21, 2023
…al (#24572)

This request fixes the ECR Repository resource to allow setting a flag on the resource to auto delete the images in the repository. This is similar to the way S3 handles the autoDeleteObjects attribute. This code base starts from a stalled PR [#15932](#15932). This also takes into account the functionality added into S3 to create tag to not delete images if the flag is flipped from true to false. 

Closes [#12618](#12618)
References closed and not merged PR  [#15932](#15932)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@missourian55
Copy link

missourian55 commented Mar 26, 2023

Tested this feature this morning with cdk 2.70.0. I am still seeing the error

Resource handler returned message: "The repository with name 'test-app' in registry with id '1234567890' 
cannot be deleted because it still contains images (Service: Ecr, Status Code: 400,
Request ID: 842c370d-f12344XXXXX-ed47d07f0443)" 
(RequestToken: c64e0c80-9xxxxxxca5006c6c, HandlerErrorCode: GeneralServiceException)

homakk pushed a commit to homakk/aws-cdk that referenced this issue Mar 28, 2023
…al (aws#24572)

This request fixes the ECR Repository resource to allow setting a flag on the resource to auto delete the images in the repository. This is similar to the way S3 handles the autoDeleteObjects attribute. This code base starts from a stalled PR [aws#15932](aws#15932). This also takes into account the functionality added into S3 to create tag to not delete images if the flag is flipped from true to false. 

Closes [aws#12618](aws#12618)
References closed and not merged PR  [aws#15932](aws#15932)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@markmansur
Copy link
Contributor

This is now supported by CloudFormation
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecr-repository.html#cfn-ecr-repository-emptyondelete

Do we still need this custom resource?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.