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

(aws-ecr): Default RemovalPolicy for ECR is RETAIN #8684

Closed
eickler opened this issue Jun 22, 2020 · 4 comments
Closed

(aws-ecr): Default RemovalPolicy for ECR is RETAIN #8684

eickler opened this issue Jun 22, 2020 · 4 comments
Assignees
Labels
@aws-cdk/aws-ecr Related to Amazon Elastic Container Registry bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p1

Comments

@eickler
Copy link

eickler commented Jun 22, 2020

Reference: https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_core.RemovalPolicy.html

The reference states that the default removal policy is DESTROY, but an ECR Repository is actually retained during the destroy phase. I created a repository using

new ecr.Repository(this, 'repository', { imageScanOnPush: true, repositoryName: ... } })

and the generated template is

repository9F1A3F0B:
  Type: AWS::ECR::Repository
    Properties:
      RepositoryName: td000-repo
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: td000/repository/Resource

There should be probably a statement that RETAIN is the default policy for services XYZ and DESTROY for services ABC but I am not sure how to properly phrase this and what actually the defaults are outside of ECR.


This is a 📕 documentation issue

@eickler eickler added documentation This is a problem with documentation. feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jun 22, 2020
@github-actions github-actions bot added the @aws-cdk/aws-ecr Related to Amazon Elastic Container Registry label Jun 22, 2020
@MrArnoldPalmer MrArnoldPalmer added p1 and removed feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jun 24, 2020
@SomayaB SomayaB added the bug This issue is a bug. label Jul 9, 2020
@MrArnoldPalmer MrArnoldPalmer added the effort/small Small work item – less than a day of effort label Aug 17, 2020
@davivcgarcia
Copy link

Do we have any update on this issue?

@MrArnoldPalmer
Copy link
Contributor

@davivcgarcia I haven't looked into this yet. I'm not quite sure what to do because basically in this spot, its not incorrect, just confusing. Since this doc is on the RemovalPolicy enum type, what it means is if no removal policy is specified in CFN then the default usually is "DESTROY". However as the issue author points out, when creating an ECR repository using the L2 the default is "RETAIN", which is documented here.

The RemovalPolicy type itself has no knowledge of the resources basically and the L2's defaults are documented alongside the L2s. I'm not quite sure what the solution is but if you're interested in investigating and giving some suggestions and possible submitting a PR that would be great.

@MrArnoldPalmer MrArnoldPalmer removed their assignment Jun 21, 2021
@nohack
Copy link
Contributor

nohack commented Jul 17, 2021

@MrArnoldPalmer @davivcgarcia

I have tested some of these assumptions and have the below points

  • In cloudformation, the default deletion policy is DESTROY.
    Through CF if create a stack with an s3 bucket in it and delete the stack
    a) Empty Bucket: The stack is deleted with associated s3 bucket.(Assume we did not explicitly specify DeletionPolicy)
    b) Non Empty Bucket: If we don't specify the deletion or even if specify the DeletionPolicy as delete, cdk destroy will
    fail with DELETE_FAILED status.We have to empty the bucket for the stack deletion to be successful.

    The same with ecr.

  • From cdk docs

    Many stateful resources in the AWS Construct Library will accept a removalPolicy as a property, typically defaulting it to
    RETAIN.

    So L2 constructs for S3, ECR and RDS can have removal policy as RETAIN as they are stateful.

There are some related issues which I pointed at the end.
One thing that is confusing from the docs here and as pointed out is that the RemovalPolicy in itself cannot have a default value as as its just a standalone enum.The resource with which a RemovalPolicy is associated can have a default removal policy.
So I think its better to remove that statement from the docs.Can create a pr if that is fine.

Refs:
aws/aws-cdk-rfcs#23

#3297

@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jul 17, 2022
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 bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

No branches or pull requests

7 participants