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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Jun 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8dcf1f2
fix(aws-ecr): auto delete images on ECR repository containing manifes…
hkford May 2, 2023
ea57af2
fix two test cases: deletes all objects when the name changes on upda…
hkford May 2, 2023
9ead2af
fix test case: delete event where repo has many images does recurse a…
hkford May 2, 2023
dfa043c
Merge branch 'main' into fix-ecr-delete
hkford May 25, 2023
5c5e8c4
Merge branch 'main' into fix-ecr-delete
hkford May 31, 2023
df5da37
add diffAssets: true to the integ test
hkford Jun 1, 2023
18dc534
update snapshot
hkford Jun 2, 2023
d1e481e
update snapshot
hkford Jun 2, 2023
b1cd781
revise after review
hkford Jun 6, 2023
a16b92e
fix snapshot
hkford Jun 6, 2023
5c34dfe
Merge branch 'main' into fix-ecr-delete
corymhall Jun 6, 2023
e1d8afa
Merge branch 'main' into fix-ecr-delete
mergify[bot] Jun 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/aws-cdk-lib/aws-ecr/lib/auto-delete-images-handler/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
  }
});


const nextToken = listedImages.nextToken ?? null;
if (imageIds.length === 0) {
return;
}

if (imageIdsTagged.length !== 0) {
await ecr.batchDeleteImage({
repositoryName: params.repositoryName,
imageIds: imageIdsTagged,
}).promise();
}

await ecr.batchDeleteImage({
repositoryName: params.repositoryName,
imageIds,
imageIds: imageIds,
}).promise();

if (nextToken) {
Expand Down
59 changes: 53 additions & 6 deletions packages/aws-cdk-lib/aws-ecr/test/auto-delete-images-handler.test.ts
View file
Open in desktop
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?

Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,15 @@ test('deletes all objects when the name changes on update event', async () => {
// THEN
expect(mockECRClient.listImages).toHaveBeenCalledTimes(1);
expect(mockECRClient.listImages).toHaveBeenCalledWith({ repositoryName: 'MyRepo' });
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(1);
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledWith({
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(2);
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(1, {
repositoryName: 'MyRepo',
imageIds: [
{ imageDigest: 'ImageDigest1', imageTag: 'ImageTag1' },
{ imageDigest: 'ImageDigest2', imageTag: 'ImageTag2' },
],
});
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(2, {
repositoryName: 'MyRepo',
imageIds: [
{ imageDigest: 'ImageDigest1', imageTag: 'ImageTag1' },
Expand Down Expand Up @@ -218,8 +225,21 @@ test('deletes all images on delete event', async () => {
// THEN
expect(mockECRClient.listImages).toHaveBeenCalledTimes(1);
expect(mockECRClient.listImages).toHaveBeenCalledWith({ repositoryName: 'MyRepo' });
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(1);
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledWith({
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(2);
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(1, {
repositoryName: 'MyRepo',
imageIds: [
{
imageTag: 'tag1',
imageDigest: 'sha256-1',
},
{
imageTag: 'tag2',
imageDigest: 'sha256-2',
},
],
});
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(2, {
repositoryName: 'MyRepo',
imageIds: [
{
Expand Down Expand Up @@ -286,7 +306,8 @@ test('delete event where repo has many images does recurse appropriately', async
],
nextToken: 'token1',
})
.mockResolvedValueOnce(undefined) // batchDeleteImage() call
.mockResolvedValueOnce(undefined) // batchDeleteImage() call for tagged images
.mockResolvedValueOnce(undefined) // batchDeleteImage() call for all images
.mockResolvedValueOnce({ // listedImages() call
imageIds: [
{
Expand Down Expand Up @@ -314,7 +335,7 @@ test('delete event where repo has many images does recurse appropriately', async
expect(mockECRClient.describeRepositories).toHaveBeenCalledTimes(1);
expect(mockECRClient.listImages).toHaveBeenCalledTimes(2);
expect(mockECRClient.listImages).toHaveBeenCalledWith({ repositoryName: 'MyRepo' });
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(2);
expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(4);
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(1, {
repositoryName: 'MyRepo',
imageIds: [
Expand All @@ -329,6 +350,32 @@ test('delete event where repo has many images does recurse appropriately', async
],
});
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(2, {
repositoryName: 'MyRepo',
imageIds: [
{
imageTag: 'tag1',
imageDigest: 'sha256-1',
},
{
imageTag: 'tag2',
imageDigest: 'sha256-2',
},
],
});
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(3, {
repositoryName: 'MyRepo',
imageIds: [
{
imageTag: 'tag3',
imageDigest: 'sha256-3',
},
{
imageTag: 'tag4',
imageDigest: 'sha256-4',
},
],
});
expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(4, {
repositoryName: 'MyRepo',
imageIds: [
{
Expand Down