fix: remove image tag only not the image using id (#3837)

This fix:
* switches to use name:tag to delete image
* uses image id if name is '<none>'
* run delete commands as a chain when deleting multiple images to avoid conflicts when
  deleting different tags of the same image

Fixes #2673

Signed-off-by: Denis Golovin <dgolovin@redhat.com>

packages/renderer/src/lib/ImagesList.svelte

@@ -164,21 +164,14 @@ function toggleAllImages(checked: boolean) {
 // delete the items selected in the list
 let bulkDeleteInProgress = false;
 async function deleteSelectedImages() {
+  bulkDeleteInProgress = true;
   const selectedImages = images.filter(image => image.selected);
-
-  if (selectedImages.length > 0) {
-    bulkDeleteInProgress = true;
-    await Promise.all(
-      selectedImages.map(async image => {
-        try {
-          await window.deleteImage(image.engineId, image.id);
-        } catch (e) {
-          console.log('error while removing image', e);
-        }
-      }),
-    );
-    bulkDeleteInProgress = false;
-  }
+  await selectedImages.reduce((prev: Promise<void>, image) => {
+    return prev
+      .then(() => imageUtils.deleteImage(image))
+      .catch((e: unknown) => console.log('error while removing image', e));
+  }, Promise.resolve());
+  bulkDeleteInProgress = false;
 }
 
 let refreshTimeouts: NodeJS.Timeout[] = [];

packages/renderer/src/lib/image/ImageActions.svelte

@@ -8,6 +8,7 @@ import FlatMenu from '../ui/FlatMenu.svelte';
 import { runImageInfo } from '../../stores/run-image-store';
 import type { Menu } from '../../../../main/src/plugin/menu-registry';
 import ContributionActions from '/@/lib/actions/ContributionActions.svelte';
+import { ImageUtils } from './image-utils';
 
 export let onPushImage: (imageInfo: ImageInfoUI) => void;
 export let onRenameImage: (imageInfo: ImageInfoUI) => void;
@@ -19,6 +20,7 @@ export let contributions: Menu[] = [];
 let errorTitle: string | undefined = undefined;
 let errorMessage: string | undefined = undefined;
 let isAuthenticatedForThisImage = false;
+const imageUtils = new ImageUtils();
 
 async function runImage(imageInfo: ImageInfoUI) {
   runImageInfo.set(imageInfo);
@@ -29,7 +31,7 @@ $: window.hasAuthconfigForImage(image.name).then(result => (isAuthenticatedForTh
 
 async function deleteImage(): Promise<void> {
   try {
-    await window.deleteImage(image.engineId, image.id);
+    await imageUtils.deleteImage(image);
   } catch (error) {
     errorTitle = 'Error while deleting image';
     errorMessage = String(error);

packages/renderer/src/lib/image/ImageDetails.spec.ts

@@ -17,7 +17,7 @@
  ***********************************************************************/
 
 import '@testing-library/jest-dom/vitest';
-import { test, expect, vi, beforeAll } from 'vitest';
+import { test, expect, vi, beforeAll, afterEach } from 'vitest';
 import { fireEvent, render, screen } from '@testing-library/svelte';
 
 import ImageDetails from './ImageDetails.svelte';
@@ -44,6 +44,11 @@ const myImage: ImageInfo = {
   Containers: 0,
 };
 
+const myNoneNameImage: ImageInfo = {
+  ...myImage,
+};
+delete myNoneNameImage.RepoTags;
+
 const deleteImageMock = vi.fn();
 const hasAuthMock = vi.fn();
 
@@ -53,6 +58,10 @@ beforeAll(() => {
   (window as any).hasAuthconfigForImage = hasAuthMock;
 });
 
+afterEach(() => {
+  vi.clearAllMocks();
+});
+
 test('Expect redirect to previous page if image is deleted', async () => {
   const routerGotoSpy = vi.spyOn(router, 'goto');
   listImagesMock.mockResolvedValue([myImage]);
@@ -95,3 +104,30 @@ test('Expect redirect to previous page if image is deleted', async () => {
   const afterRoute = window.location;
   expect(afterRoute.href).toBe('http://localhost:3000/last');
 });
+
+test('expect delete image called with image id when image name is <none>', async () => {
+  listImagesMock.mockResolvedValue([myNoneNameImage]);
+  window.dispatchEvent(new CustomEvent('extensions-already-started'));
+
+  while (get(imagesInfos).length !== 1) {
+    await new Promise(resolve => setTimeout(resolve, 500));
+  }
+
+  hasAuthMock.mockImplementation(() => {
+    return new Promise(() => false);
+  });
+
+  // render the component
+  render(ImageDetails, {
+    imageID: 'myImage',
+    engineId: 'engine0',
+    base64RepoTag: Buffer.from('<none>', 'binary').toString('base64'),
+  });
+
+  // click on delete image button
+  const deleteButton = screen.getByRole('button', { name: 'Delete Image' });
+  await fireEvent.click(deleteButton);
+
+  // check that delete method has been called
+  expect(deleteImageMock).toHaveBeenCalledWith(myNoneNameImage.engineId, myNoneNameImage.Id);
+});

packages/renderer/src/lib/image/ImageDetails.svelte

@@ -42,13 +42,13 @@ onMount(() => {
   // loading image info
   return imagesInfos.subscribe(images => {
     const matchingImage = images.find(c => c.Id === imageID && c.engineId === engineId);
+    let tempImage;
     if (matchingImage) {
-      try {
-        image = imageUtils.getImageInfoUI(matchingImage, base64RepoTag);
-      } catch (err) {
-        console.error(err);
-      }
-    } else if (detailsPage) {
+      tempImage = imageUtils.getImageInfoUI(matchingImage, base64RepoTag);
+    }
+    if (tempImage) {
+      image = tempImage;
+    } else {
       // the image has been deleted
       detailsPage.close();
     }

packages/renderer/src/lib/image/RenameImageModal.svelte

@@ -62,6 +62,7 @@ async function renameImage(imageName: string, imageTag: string) {
 </script>
 
 <Modal
+  name="Edit Image"
   on:close="{() => {
     closeCallback();
   }}">
@@ -85,6 +86,7 @@ async function renameImage(imageName: string, imageTag: string) {
           class="w-full my-2 p-2 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
           on:input="{event => validateImageName(event)}"
           aria-invalid="{imageNameErrorMessage !== ''}"
+          aria-label="imageName"
           required />
         {#if imageNameErrorMessage}
           <ErrorMessage error="{imageNameErrorMessage}" />
@@ -100,6 +102,7 @@ async function renameImage(imageName: string, imageTag: string) {
           class="w-full my-2 p-2 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
           on:input="{event => validateImageTag(event)}"
           aria-invalid="{imageTagErrorMessage !== ''}"
+          aria-label="imageTag"
           required />
         {#if imageTagErrorMessage}
           <ErrorMessage error="{imageTagErrorMessage}" />

packages/renderer/src/lib/image/image-utils.ts

@@ -126,12 +126,13 @@ export class ImageUtils {
     }
   }
 
-  getImageInfoUI(imageInfo: ImageInfo, base64RepoTag: string): ImageInfoUI {
+  deleteImage(image: ImageInfoUI) {
+    const imageId = image.name === '<none>' ? image.id : `${image.name}:${image.tag}`;
+    return window.deleteImage(image.engineId, imageId);
+  }
+
+  getImageInfoUI(imageInfo: ImageInfo, base64RepoTag: string): ImageInfoUI | undefined {
     const images = this.getImagesInfoUI(imageInfo, []);
-    const matchingImages = images.filter(image => image.base64RepoTag === base64RepoTag);
-    if (matchingImages.length === 1) {
-      return matchingImages[0];
-    }
-    throw new Error(`Unable to find a matching image for id ${imageInfo.Id} and tag ${base64RepoTag}`);
+    return images.find(image => image.base64RepoTag === base64RepoTag);
   }
 }

tests/src/image-smoke.spec.ts

@@ -22,12 +22,12 @@ import { afterAll, beforeAll, test, describe, beforeEach, expect } from 'vitest'
 import { expect as playExpect } from '@playwright/test';
 import { PodmanDesktopRunner } from './runner/podman-desktop-runner';
 import { WelcomePage } from './model/pages/welcome-page';
-import { ImagesPage } from './model/pages/images-page';
 import { NavigationBar } from './model/workbench/navigation';
 import { ImageDetailsPage } from './model/pages/image-details-page';
 
 let pdRunner: PodmanDesktopRunner;
 let page: Page;
+let navBar: NavigationBar;
 
 beforeAll(async () => {
   pdRunner = new PodmanDesktopRunner();
@@ -36,6 +36,7 @@ beforeAll(async () => {
 
   const welcomePage = new WelcomePage(page);
   await welcomePage.handleWelcomePage(true);
+  navBar = new NavigationBar(page); // always present on the left side of the page
 });
 
 afterAll(async () => {
@@ -47,8 +48,15 @@ beforeEach<RunnerTestContext>(async ctx => {
 });
 
 describe('Image workflow verification', async () => {
+  async function pullImageByName(imageName: string) {
+    let imagesPage = await navBar.openImages();
+    const pullImagePage = await imagesPage.openPullImage();
+    imagesPage = await pullImagePage.pullImage(imageName);
+    await imagesPage.waitForImageExists(imageName);
+    return imagesPage;
+  }
+
   test('Pull image', async () => {
-    const navBar = new NavigationBar(page);
     const imagesPage = await navBar.openImages();
     await playExpect(imagesPage.heading).toBeVisible();
 
@@ -60,23 +68,37 @@ describe('Image workflow verification', async () => {
   });
 
   test('Check image details', async () => {
-    const imagesPage = new ImagesPage(page);
+    const imagesPage = await navBar.openImages();
     const imageDetailPage = await imagesPage.openImageDetails('quay.io/podman/hello');
 
     await playExpect(imageDetailPage.summaryTab).toBeVisible();
     await playExpect(imageDetailPage.historyTab).toBeVisible();
     await playExpect(imageDetailPage.inspectTab).toBeVisible();
   });
 
+  test('Rename image', async () => {
+    const imageDetailsPage = new ImageDetailsPage(page, 'quay.io/podman/hello');
+    const editPage = await imageDetailsPage.openEditImage();
+    await playExpect(editPage.cancelButton).toBeEnabled();
+    await playExpect(editPage.saveButton).toBeVisible();
+    await playExpect(editPage.saveButton).toBeDisabled();
+    await editPage.imageName.fill('quay.io/podman/hi');
+    await playExpect(editPage.saveButton).toBeEnabled();
+    await editPage.saveButton.click();
+    const imagesPage = await navBar.openImages();
+    expect(await imagesPage.waitForImageExists('quay.io/podman/hi')).equals(true);
+  });
+
   test('Delete image', async () => {
-    const imageDetailPage = new ImageDetailsPage(page, 'quay.io/podman/hello');
+    const imagesPage = await pullImageByName('quay.io/podman/hello');
+    expect(await imagesPage.waitForImageExists('quay.io/podman/hello')).equals(true);
+
+    const imageDetailPage = await imagesPage.openImageDetails('quay.io/podman/hello');
     await playExpect(imageDetailPage.deleteButton).toBeVisible();
     await imageDetailPage.deleteButton.click();
 
-    const imagesPage = new ImagesPage(page);
-    await playExpect(imagesPage.heading).toBeVisible();
-
-    const imageExists = await imagesPage.waitForImageDelete('quay.io/podman/hello');
-    playExpect(imageExists).toBeTruthy();
+    const imageDeleted = await imagesPage.waitForImageDelete('quay.io/podman/hello');
+    expect(imageDeleted).equals(true);
+    expect(await imagesPage.waitForImageExists('quay.io/podman/hi')).equals(true);
   });
 });

tests/src/model/pages/image-details-page.ts

@@ -19,6 +19,7 @@
 import type { Locator, Page } from 'playwright';
 import { BasePage } from './base-page';
 import { RunImagePage } from './run-image-page';
+import { ImageEditPage } from './image-edit-page';
 
 export class ImageDetailsPage extends BasePage {
   readonly name: Locator;
@@ -52,4 +53,9 @@ export class ImageDetailsPage extends BasePage {
     await this.runImageButton.click();
     return new RunImagePage(this.page, this.imageName);
   }
+
+  async openEditImage(): Promise<ImageEditPage> {
+    await this.editButton.click();
+    return new ImageEditPage(this.page, this.imageName);
+  }
 }

tests/src/model/pages/image-edit-page.ts

@@ -0,0 +1,36 @@
+/**********************************************************************
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ ***********************************************************************/
+
+import type { Locator, Page } from 'playwright';
+import { BasePage } from './base-page';
+
+export class ImageEditPage extends BasePage {
+  readonly name: string;
+  readonly cancelButton: Locator;
+  readonly saveButton: Locator;
+  readonly imageName: Locator;
+  readonly imageTag: Locator;
+  constructor(page: Page, name: string) {
+    super(page);
+    this.imageName = page.getByLabel('imageName');
+    this.cancelButton = page.getByRole('button', { name: 'Cancel' });
+    this.saveButton = page.getByRole('button', { name: 'Save' });
+    this.name = name;
+    this.imageTag = page.getByLabel('imageTag');
+  }
+}