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

fix(ecr): auto delete images on ECR repository containing manifest list #25789

Merged
merged 12 commits into from
Jun 6, 2023

Conversation

hkford
Copy link
Contributor

@hkford hkford commented May 31, 2023

Fixes #24822

As I commented on #24822 (comment), auto delete container images in ECR repository fails when it has container manifest list. I fix custom resource Lambda function to delete tagged images first.


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

@gitpod-io
Copy link

gitpod-io bot commented May 31, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team May 31, 2023 02:15
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels May 31, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@hkford
Copy link
Contributor Author

hkford commented May 31, 2023

Exemption Request - All integration tests remain unchanged so I think we do not need to update snapshots in this PR.

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels May 31, 2023
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

Can you add diffAssets: true to the integ test here? Since this integ test tests custom resources this should have been set to true and will show the diff.

@corymhall corymhall removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Jun 1, 2023
@hkford
Copy link
Contributor Author

hkford commented Jun 1, 2023

@corymhall
Added diffAssets: true here

@tmokmss
Copy link
Contributor

tmokmss commented Jun 2, 2023

@hkford
You should run the integration test locally in your AWS environment to update the snapshots and make sure it works. :)
c.f. https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#integration-tests

@aws-cdk-automation aws-cdk-automation dismissed their stale review June 2, 2023 03:04

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

@mergify mergify bot dismissed corymhall’s stale review June 2, 2023 07:24

Pull request has been modified.

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

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of minor comments.

@@ -40,14 +40,23 @@ async function emptyRepository(params: ECR.ListImagesRequest) {
const listedImages = await ecr.listImages(params).promise();

const imageIds = listedImages?.imageIds ?? [];
const imageIdsTagged = imageIds.filter((imageId) => 'imageTag' in imageId) ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you would be processing the tagged images twice. Can we instead create two lists that don't have overlap?

Maybe something like

const imageIds = [];
const imageIdsTagged = [];
(listedImages.imageIds ?? []).forEach(imageId => {
  if ('imageTag' in imageId) {
    imageIdsTagged.push(imageId);
  } else {
    imageIds.push(imageId);
  }
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test where some images are tagged and some are not (with mixed ordering) so that we can assert that it deletes the tagged images first?

@corymhall corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 5, 2023
@corymhall corymhall self-assigned this Jun 5, 2023
@mergify mergify bot dismissed corymhall’s stale review June 6, 2023 02:30

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 6, 2023
corymhall
corymhall previously approved these changes Jun 6, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2023

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 mergify bot dismissed corymhall’s stale review June 6, 2023 16:17

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2023

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 aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 6, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2023

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

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 830e6d3 into aws:main Jun 6, 2023
7 checks passed
@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2023

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).

@hkford hkford deleted the fix-ecr-delete branch June 13, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto delete images upon ECR repo removal - CDK v2.70.0 not working
4 participants