refactor(kubernetes): removed unused displayName property from port…

… forward config (podman-desktop#9806) * refactor: removed unused display name forward config Signed-off-by: axel7083 <[email protected]> * fix(typecheck): remove unnecessary display name Signed-off-by: axel7083 <[email protected]> * fix: tests Signed-off-by: axel7083 <[email protected]> --------- Signed-off-by: axel7083 <[email protected]>
axel7083 · Nov 12, 2024 · d11bfa2 · d11bfa2
1 parent 0cd309f
commit d11bfa2
Show file tree

Hide file tree

Showing 19 changed files with 125 additions and 171 deletions.
diff --git a/packages/api/src/kubernetes-port-forward-model.ts b/packages/api/src/kubernetes-port-forward-model.ts
@@ -60,10 +60,6 @@ export interface ForwardOptions {
    * The forward to create
    */
   forward: PortMapping;
-  /**
-   * The display name for the forward configuration.
-   */
-  displayName: string;
 }
 
 /**
@@ -96,15 +92,3 @@ export interface ForwardConfig {
    */
   forward: PortMapping;
 }
-
-/**
- * Interface representing a user-specific forward configuration.
- * Extends the base {@link ForwardConfig} interface.
- * @see ForwardConfig
- */
-export interface UserForwardConfig extends ForwardConfig {
-  /**
-   * The display name for the forward configuration.
-   */
-  displayName: string;
-}
diff --git a/packages/main/src/plugin/index.ts b/packages/main/src/plugin/index.ts
@@ -87,7 +87,7 @@ import type { ImageInspectInfo } from '/@api/image-inspect-info.js';
 import type { ImageSearchOptions, ImageSearchResult, ImageTagsListOptions } from '/@api/image-registry.js';
 import type { KubeContext } from '/@api/kubernetes-context.js';
 import type { ContextGeneralState, ResourceName } from '/@api/kubernetes-contexts-states.js';
-import type { ForwardOptions, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
+import type { ForwardConfig, ForwardOptions } from '/@api/kubernetes-port-forward-model.js';
 import type { ManifestCreateOptions, ManifestInspectInfo, ManifestPushOptions } from '/@api/manifest-info.js';
 import type { NetworkInspectInfo } from '/@api/network-info.js';
 import type { NotificationCard, NotificationCardOptions } from '/@api/notification.js';
@@ -2464,23 +2464,20 @@ export class PluginSystem {
       },
     );
 
-    this.ipcHandle('kubernetes-client:getPortForwards', async (_listener): Promise<UserForwardConfig[]> => {
+    this.ipcHandle('kubernetes-client:getPortForwards', async (_listener): Promise<ForwardConfig[]> => {
       return kubernetesClient.getPortForwards();
     });
 
     this.ipcHandle(
       'kubernetes-client:createPortForward',
-      async (_listener, options: ForwardOptions): Promise<UserForwardConfig> => {
+      async (_listener, options: ForwardOptions): Promise<ForwardConfig> => {
         return kubernetesClient.createPortForward(options);
       },
     );
 
-    this.ipcHandle(
-      'kubernetes-client:deletePortForward',
-      async (_listener, config: UserForwardConfig): Promise<void> => {
-        return kubernetesClient.deletePortForward(config);
-      },
-    );
+    this.ipcHandle('kubernetes-client:deletePortForward', async (_listener, config: ForwardConfig): Promise<void> => {
+      return kubernetesClient.deletePortForward(config);
+    });
 
     this.ipcHandle(
       'openshift-client:createRoute',

diff --git a/packages/main/src/plugin/kubernetes/kubernetes-client.ts b/packages/main/src/plugin/kubernetes/kubernetes-client.ts
@@ -71,7 +71,7 @@ import type { KubernetesPortForwardService } from '/@/plugin/kubernetes/kubernet
 import { KubernetesPortForwardServiceProvider } from '/@/plugin/kubernetes/kubernetes-port-forward-service.js';
 import type { KubeContext } from '/@api/kubernetes-context.js';
 import type { ContextGeneralState, ResourceName } from '/@api/kubernetes-contexts-states.js';
-import type { ForwardOptions, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
+import type { ForwardConfig, ForwardOptions } from '/@api/kubernetes-port-forward-model.js';
 import type { V1Route } from '/@api/openshift-types.js';
 
 import type { ApiSenderType } from '../api.js';
@@ -1761,18 +1761,18 @@ export class KubernetesClient {
     return this.#portForwardService;
   }
 
-  public async getPortForwards(): Promise<UserForwardConfig[]> {
+  public async getPortForwards(): Promise<ForwardConfig[]> {
     return this.ensurePortForwardService().listForwards();
   }
 
-  public async createPortForward(config: ForwardOptions): Promise<UserForwardConfig> {
+  public async createPortForward(config: ForwardOptions): Promise<ForwardConfig> {
     const service = this.ensurePortForwardService();
     const newConfig = await service.createForward(config);
     await service.startForward(newConfig);
     return newConfig;
   }
 
-  public async deletePortForward(config: UserForwardConfig): Promise<void> {
+  public async deletePortForward(config: ForwardConfig): Promise<void> {
     return this.ensurePortForwardService().deleteForward(config);
   }
 }
diff --git a/packages/main/src/plugin/kubernetes/kubernetes-port-forward-service.spec.ts b/packages/main/src/plugin/kubernetes/kubernetes-port-forward-service.spec.ts
@@ -30,7 +30,7 @@ import {
 } from '/@/plugin/kubernetes/kubernetes-port-forward-service.js';
 import type { ConfigManagementService } from '/@/plugin/kubernetes/kubernetes-port-forward-storage.js';
 import type { IDisposable } from '/@/plugin/types/disposable.js';
-import type { ForwardConfig, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
+import type { ForwardConfig } from '/@api/kubernetes-port-forward-model.js';
 import { WorkloadKind } from '/@api/kubernetes-port-forward-model.js';
 
 vi.mock('/@/plugin/kubernetes/kubernetes-port-forward-connection.js');
@@ -79,18 +79,13 @@ describe('KubernetesPortForwardService', () => {
     forward: { localPort: 8080, remotePort: 80 },
   };
 
-  const sampleUserForwardConfig: UserForwardConfig = {
-    ...sampleForwardConfig,
-    displayName: 'test-display-name',
-  };
-
   beforeEach(() => {
     vi.clearAllMocks();
     mockConfigManagementService = {
-      createForward: vi.fn().mockResolvedValue(sampleUserForwardConfig),
+      createForward: vi.fn().mockResolvedValue(sampleForwardConfig),
       deleteForward: vi.fn().mockResolvedValue(undefined),
-      updateForward: vi.fn().mockResolvedValue(sampleUserForwardConfig),
-      listForwards: vi.fn().mockResolvedValue([sampleUserForwardConfig]),
+      updateForward: vi.fn().mockResolvedValue(sampleForwardConfig),
+      listForwards: vi.fn().mockResolvedValue([sampleForwardConfig]),
     } as unknown as ConfigManagementService;
 
     mockForwardingConnectionService = {
@@ -107,30 +102,29 @@ describe('KubernetesPortForwardService', () => {
 
   test('should create a forward configuration', async () => {
     vi.mocked(mockConfigManagementService.listForwards).mockResolvedValue([]);
-    const forward = sampleUserForwardConfig.forward;
+    const forward = sampleForwardConfig.forward;
     if (!forward) throw new Error('undefined forward');
 
     const result = await service.createForward({
-      name: sampleUserForwardConfig.name,
-      kind: sampleUserForwardConfig.kind,
-      namespace: sampleUserForwardConfig.namespace,
+      name: sampleForwardConfig.name,
+      kind: sampleForwardConfig.kind,
+      namespace: sampleForwardConfig.namespace,
       forward: forward,
-      displayName: sampleUserForwardConfig.displayName,
     });
-    expect(result).toEqual(sampleUserForwardConfig);
-    expect(mockConfigManagementService.createForward).toHaveBeenCalledWith(sampleUserForwardConfig);
+    expect(result).toEqual(sampleForwardConfig);
+    expect(mockConfigManagementService.createForward).toHaveBeenCalledWith(sampleForwardConfig);
     expect(apiSenderMock.send).toHaveBeenCalledWith('kubernetes-port-forwards-update', []);
   });
 
   test('should delete a forward configuration', async () => {
-    await service.deleteForward(sampleUserForwardConfig);
-    expect(mockConfigManagementService.deleteForward).toHaveBeenCalledWith(sampleUserForwardConfig);
+    await service.deleteForward(sampleForwardConfig);
+    expect(mockConfigManagementService.deleteForward).toHaveBeenCalledWith(sampleForwardConfig);
     expect(apiSenderMock.send).toHaveBeenCalledWith('kubernetes-port-forwards-update', []);
   });
 
   test('should list all forward configurations', async () => {
     const result = await service.listForwards();
-    expect(result).toEqual([sampleUserForwardConfig]);
+    expect(result).toEqual([sampleForwardConfig]);
     expect(mockConfigManagementService.listForwards).toHaveBeenCalled();
   });
 

diff --git a/packages/main/src/plugin/kubernetes/kubernetes-port-forward-service.ts b/packages/main/src/plugin/kubernetes/kubernetes-port-forward-service.ts
@@ -27,7 +27,7 @@ import { ForwardConfigRequirements } from '/@/plugin/kubernetes/kubernetes-port-
 import type { IDisposable } from '/@/plugin/types/disposable.js';
 import { Disposable } from '/@/plugin/types/disposable.js';
 import { isFreePort } from '/@/plugin/util/port.js';
-import type { ForwardConfig, ForwardOptions, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
+import type { ForwardConfig, ForwardOptions } from '/@api/kubernetes-port-forward-model.js';
 
 /**
  * Service provider for Kubernetes port forwarding.
@@ -94,14 +94,13 @@ export class KubernetesPortForwardService implements IDisposable {
    * @returns The created forward configuration.
    * @param options
    */
-  async createForward(options: ForwardOptions): Promise<UserForwardConfig> {
-    const result: UserForwardConfig = await this.configManagementService.createForward({
+  async createForward(options: ForwardOptions): Promise<ForwardConfig> {
+    const result: ForwardConfig = await this.configManagementService.createForward({
       id: randomUUID(),
       name: options.name,
       forward: options.forward,
       namespace: options.namespace,
       kind: options.kind,
-      displayName: options.displayName,
     });
 
     this.apiSender.send('kubernetes-port-forwards-update', await this.listForwards());
@@ -112,9 +111,9 @@ export class KubernetesPortForwardService implements IDisposable {
    * Deletes an existing forward configuration.
    * @param config - The forward configuration to delete.
    * @returns Void if the operation successful.
-   * @see UserForwardConfig
+   * @see ForwardConfig
    */
-  async deleteForward(config: UserForwardConfig): Promise<void> {
+  async deleteForward(config: ForwardConfig): Promise<void> {
     const disposable = this.#disposables.get(config.id);
     disposable?.dispose();
     this.#disposables.delete(config.id);
@@ -127,9 +126,9 @@ export class KubernetesPortForwardService implements IDisposable {
   /**
    * Lists all forward configurations.
    * @returns A list of forward configurations.
-   * @see UserForwardConfig
+   * @see ForwardConfig
    */
-  async listForwards(): Promise<UserForwardConfig[]> {
+  async listForwards(): Promise<ForwardConfig[]> {
     return this.configManagementService.listForwards();
   }
 

diff --git a/packages/main/src/plugin/kubernetes/kubernetes-port-forward-storage.spec.ts b/packages/main/src/plugin/kubernetes/kubernetes-port-forward-storage.spec.ts
@@ -30,7 +30,7 @@ import {
   MemoryBasedStorage,
   PreferenceFolderBasedStorage,
 } from '/@/plugin/kubernetes/kubernetes-port-forward-storage.js';
-import type { UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
+import type { ForwardConfig } from '/@api/kubernetes-port-forward-model.js';
 import { WorkloadKind } from '/@api/kubernetes-port-forward-model.js';
 
 vi.mock('node:fs/promises', () => ({
@@ -45,15 +45,15 @@ class TestFileBasedConfigStorage extends FileBasedConfigStorage {
     return super.ensureStorageInitialized();
   }
 
-  public override _createForward(config: UserForwardConfig): void {
+  public override _createForward(config: ForwardConfig): void {
     super._createForward(config);
   }
 
-  public override _deleteForward(config: UserForwardConfig): void {
+  public override _deleteForward(config: ForwardConfig): void {
     super._deleteForward(config);
   }
 
-  public override _updateForward(config: UserForwardConfig, newConfig: UserForwardConfig): void {
+  public override _updateForward(config: ForwardConfig, newConfig: ForwardConfig): void {
     super._updateForward(config, newConfig);
   }
 }
@@ -134,13 +134,12 @@ describe('FileBasedConfigStorage', () => {
     getStoragePath: vi.fn().mockReturnValue('/mock/storage/path'),
   };
 
-  const sampleConfig: UserForwardConfig = {
+  const sampleConfig: ForwardConfig = {
     id: 'fake-id',
     name: 'test-name',
     namespace: 'test-namespace',
     kind: WorkloadKind.POD,
     forward: { localPort: 8080, remotePort: 80 },
-    displayName: 'test-display-name',
   };
 
   beforeEach(() => {
@@ -206,16 +205,14 @@ describe('FileBasedConfigStorage', () => {
     const storage = new TestFileBasedConfigStorage(mockFileStorage, 'test-key');
     storage['configs'] = [sampleConfig];
 
-    expect(() => storage._createForward(sampleConfig)).toThrow(
-      'Found existed forward configuration with the same display name.',
-    );
+    expect(() => storage._createForward(sampleConfig)).toThrow('Found existed forward configuration with the same id.');
   });
 
   test('should throw an error if deleting a non-existing forward configuration', () => {
     const storage = new TestFileBasedConfigStorage(mockFileStorage, 'test-key');
 
     expect(() => storage._deleteForward(sampleConfig)).toThrow(
-      `Forward configuration with display name ${sampleConfig.displayName} not found.`,
+      `Forward configuration with id ${sampleConfig.id} not found.`,
     );
   });
 
@@ -224,7 +221,7 @@ describe('FileBasedConfigStorage', () => {
     const newConfig = { ...sampleConfig, displayName: 'new-display-name' };
 
     expect(() => storage._updateForward(sampleConfig, newConfig)).toThrow(
-      `Forward configuration with display name ${sampleConfig.displayName} not found.`,
+      `Forward configuration with id ${sampleConfig.id} not found.`,
     );
   });
 
@@ -257,13 +254,12 @@ describe('FileBasedConfigStorage', () => {
 });
 
 describe('MemoryBasedConfigStorage', () => {
-  const sampleConfig: UserForwardConfig = {
+  const sampleConfig: ForwardConfig = {
     id: 'fake-id',
     name: 'test-name',
     namespace: 'test-namespace',
     kind: WorkloadKind.POD,
     forward: { localPort: 8080, remotePort: 80 },
-    displayName: 'test-display-name',
   };
 
   beforeEach(() => {
@@ -286,7 +282,7 @@ describe('MemoryBasedConfigStorage', () => {
   });
 
   test('should update forward configuration', async () => {
-    const newConfig = { ...sampleConfig, displayName: 'new-display-name' };
+    const newConfig: ForwardConfig = { ...sampleConfig, namespace: 'new-ns' };
     const storage = new MemoryBasedStorage();
     storage['configs'] = [sampleConfig];
     await storage.updateForward(sampleConfig, newConfig);
@@ -302,29 +298,27 @@ describe('MemoryBasedConfigStorage', () => {
     expect(result).toContainEqual(sampleConfig);
   });
 
-  test('should throw an error if creating forward configuration with existing display name', () => {
+  test('should throw an error if creating forward configuration with existing id', () => {
     const storage = new MemoryBasedStorage();
     storage['configs'] = [sampleConfig];
 
-    expect(() => storage.createForward(sampleConfig)).toThrow(
-      'Found existed forward configuration with the same display name.',
-    );
+    expect(() => storage.createForward(sampleConfig)).toThrow('Found existed forward configuration with the same id.');
   });
 
   test('should throw an error if deleting a non-existing forward configuration', () => {
     const storage = new MemoryBasedStorage();
 
     expect(() => storage.deleteForward(sampleConfig)).toThrow(
-      `Forward configuration with display name ${sampleConfig.displayName} not found.`,
+      `Forward configuration with id ${sampleConfig.id} not found.`,
     );
   });
 
   test('should throw an error if updating a non-existing forward configuration', () => {
     const storage = new MemoryBasedStorage();
-    const newConfig = { ...sampleConfig, displayName: 'new-display-name' };
+    const newConfig: ForwardConfig = { ...sampleConfig, namespace: 'new-ns' };
 
     expect(() => storage.updateForward(sampleConfig, newConfig)).toThrow(
-      `Forward configuration with display name ${sampleConfig.displayName} not found.`,
+      `Forward configuration with id ${sampleConfig.id} not found.`,
     );
   });
 });
@@ -337,13 +331,12 @@ describe('ConfigManagementService', () => {
     listForwards: vi.fn(),
   } as unknown as ForwardConfigStorage;
 
-  const sampleConfig: UserForwardConfig = {
+  const sampleConfig: ForwardConfig = {
     id: 'fake-id',
     name: 'test-name',
     namespace: 'test-namespace',
     kind: WorkloadKind.POD,
     forward: { localPort: 8080, remotePort: 80 },
-    displayName: 'test-display-name',
   };
 
   test('should create forward configuration', async () => {
@@ -378,9 +371,8 @@ describe('ConfigManagementService', () => {
     mockConfigStorage.listForwards = vi.fn().mockResolvedValue([sampleConfig]);
     const service = new ConfigManagementService(mockConfigStorage);
 
-    const old: UserForwardConfig = {
+    const old: ForwardConfig = {
       forward: { localPort: 8080, remotePort: 80 },
-      displayName: 'dummy',
       namespace: 'default',
       kind: WorkloadKind.POD,
       name: 'hihi',