feat: enhance type safety and error handling in extension

Fix #194.

- Updated package.json to include a typecheck script.
- Updated build script to include typecheck strp.
- Improved type definitions in extension.spec.ts and extension.ts for better type safety.
- Added error handling for missing context, cluster, and user in OpenShift functions.
- Refactored functions to ensure undefined checks and proper error messages.
- Enhanced the getSandboxAPIUrl function to return undefined when no URL is configured.

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

Author: dgolovin

package.json

@@ -74,10 +74,11 @@
     }
   },
   "scripts": {
-    "build": "vite build && node ./scripts/build.cjs",
+    "build": "pnpm typecheck && vite build && node ./scripts/build.cjs",
     "watch": "vite build -w",
     "format:check": "prettier --check \"**/*.ts\" \"scripts/*.cjs\"",
     "format:fix": "prettier --write \"**/*.ts\" \"scripts/*.cjs\"",
+    "typecheck": "tsc --noEmit",
     "test": "vitest run --coverage",
     "test:e2e:setup": "xvfb-maybe --auto-servernum --server-args='-screen 0 1280x960x24' --",
     "test:e2e": "pnpm run test:e2e:setup npx playwright test tests/src"

src/extension.spec.ts

@@ -25,7 +25,7 @@ import { URI } from 'vscode-uri';
 import * as openshift from './openshift.js';
 import * as sandbox from './sandbox.js';
 import * as kubeconfig from './kubeconfig.js';
-import { CoreV1Api, KubeConfig } from '@kubernetes/client-node';
+import { CoreV1Api, KubeConfig, type V1Secret } from '@kubernetes/client-node';
 import got from 'got';
 
 const context: podmanDesktopApi.ExtensionContext = {
@@ -91,7 +91,7 @@ suite('kubernetes provider connection factory', () => {
     config: KubeConfig = new KubeConfig(),
     mockSandboxCalls?: () => void,
     mockGetPipelineServiceAccountToken?: () => void,
-  ): Promise<{ error: Error; provider: podmanDesktopApi.Provider } | undefined> {
+  ): Promise<{ error: Error | undefined; provider: podmanDesktopApi.Provider }> {
     const providerMock: podmanDesktopApi.Provider = {
       setKubernetesProviderConnectionFactory: vi.fn(),
       registerKubernetesProviderConnection: () => ({
@@ -106,7 +106,7 @@ suite('kubernetes provider connection factory', () => {
     } as unknown as podmanDesktopApi.AuthenticationSession);
     await extension.activate(context);
     const connectionFactory = vi.mocked(providerMock.setKubernetesProviderConnectionFactory).mock.calls[0][0];
-    let verificationError: Error;
+    let verificationError: Error | undefined = undefined;
     try {
       mockSandboxCalls?.();
       if (!mockSandboxCalls) {
@@ -125,9 +125,12 @@ suite('kubernetes provider connection factory', () => {
       vi.spyOn(openshift, 'whoami').mockResolvedValue('system:serviceaccount:username-dev:pipeline');
       vi.spyOn(kubeconfig, 'createOrLoadFromFile').mockReturnValue(config);
       vi.spyOn(kubeconfig, 'exportToFile').mockImplementation(vi.fn());
-      await connectionFactory.create(params);
+
+      expect(connectionFactory).toBeDefined();
+      expect(typeof connectionFactory?.create).toBe('function');
+      await connectionFactory?.create?.(params);
     } catch (e) {
-      verificationError = e;
+      verificationError = e as Error;
     }
     return {
       error: verificationError,
@@ -139,7 +142,7 @@ suite('kubernetes provider connection factory', () => {
     const { error: verificationError } = await callCreate();
 
     expect(verificationError).toBeDefined();
-    expect(verificationError.message).is.equal('Context name is required.');
+    expect(verificationError?.message).is.equal('Context name is required.');
   });
 
   test('creates new context for sandbox with specified url/token and sets it as default context', async () => {
@@ -344,8 +347,8 @@ suite('kubernetes provider connection factory', () => {
             },
           ],
         });
-        vi.spyOn(CoreV1Api.prototype, 'createNamespacedSecret').mockResolvedValue(undefined);
-        const responseError = new Error();
+        vi.spyOn(CoreV1Api.prototype, 'createNamespacedSecret').mockResolvedValue({} as V1Secret);
+        const responseError: any = new Error();
         responseError['response'] = {
           statusCode: 404,
         };
@@ -444,7 +447,7 @@ test('push image to sandbox does not change title after it is finished', async (
     return config;
   });
   const registerCommandMock = vi.mocked(podmanDesktopApi.commands.registerCommand);
-  let commandCallback: (...args: any[]) => any;
+  let commandCallback: ((...args: any[]) => any) | undefined;
   registerCommandMock.mockImplementation((commandId: string, callback: (...args: any[]) => any) => {
     if (commandId === 'sandbox.image.push.to.cluster') {
       commandCallback = callback;
@@ -456,7 +459,7 @@ test('push image to sandbox does not change title after it is finished', async (
   let registeredConnection: { status: () => any };
   const providerMock: podmanDesktopApi.Provider = {
     setKubernetesProviderConnectionFactory: vi.fn(),
-    registerKubernetesProviderConnection: connection => {
+    registerKubernetesProviderConnection: (connection: podmanDesktopApi.KubernetesProviderConnection) => {
       registeredConnection = connection;
       return {
         dispose: vi.fn(),
@@ -479,20 +482,20 @@ test('push image to sandbox does not change title after it is finished', async (
       const progress: podmanDesktopApi.Progress<{ message?: string; increment?: number }> = {
         report,
       };
-      task(progress, undefined);
-      return;
+      task(progress, {} as podmanDesktopApi.CancellationToken);
+      return Promise.resolve();
     },
   );
-  let pushImageCallback: (name: string, data?: string) => void;
+  let pushImageCallback: ((name: string, data?: string) => void) | undefined;
   vi.mocked(podmanDesktopApi.containerEngine.pushImage).mockImplementation(
-    async (
+    (async (
       _engineId: string,
       _imageId: string,
-      callback: (name: string, data: string) => Promise<void>,
+      callback: (name: string, data?: string) => void,
       _authInfo?: podmanDesktopApi.ContainerAuthInfo,
     ) => {
       pushImageCallback = callback;
-    },
+    }) as any,
   );
   await extension.activate(context);
 
@@ -504,14 +507,14 @@ test('push image to sandbox does not change title after it is finished', async (
 
   const imageInfo = { engineId: 'podman', name: 'imageName', tag: 'registry-host/repository/image' };
 
-  await Promise.resolve(commandCallback(...[imageInfo]));
+  await Promise.resolve(commandCallback?.(...[imageInfo]));
 
   await vi.waitFor(async () => {
     expect(pushImageCallback).toBeDefined();
   }, 3000);
 
-  pushImageCallback('data', 'data-chunk-1');
-  pushImageCallback('end');
+  pushImageCallback?.('data', 'data-chunk-1');
+  pushImageCallback?.('end');
 
   expect(report).not.toHaveBeenCalledWith(expect.objectContaining({ message: 'data-chunk-1' }));
 });

src/extension.ts

@@ -56,7 +56,7 @@ export async function pushImageToOpenShiftRegistry(image: ImageInfo): Promise<vo
     );
     return;
   }
-  let targetSb: string;
+  let targetSb: string | undefined;
   if (qp.length > 1) {
     targetSb = await extensionApi.window.showQuickPick(qp);
     if (!targetSb) {
@@ -76,8 +76,9 @@ export async function pushImageToOpenShiftRegistry(image: ImageInfo): Promise<vo
         progress.report({ increment: 25 });
         const registryInfo = await getOpenShiftInternalRegistryPublicHost(targetSb);
         progress.report({ increment: 50 });
-        const lastIndexOfSlash = image.name.lastIndexOf('/');
-        const imageShortName = lastIndexOfSlash !== -1 ? image.name.substring(lastIndexOfSlash + 1) : image.name;
+        const imageName = image.name ?? '';
+        const lastIndexOfSlash = imageName.lastIndexOf('/');
+        const imageShortName = lastIndexOfSlash !== -1 ? imageName.substring(lastIndexOfSlash + 1) : imageName;
         const imageTagSuffix = image.tag ? `:${image.tag}` : ``;
         const localImageName = `${image.name}${imageTagSuffix}`;
         const remoteImageName = `${registryInfo.host}/${registryInfo.username}-dev/${imageShortName}${imageTagSuffix}`;
@@ -128,18 +129,23 @@ export async function pushImageToOpenShiftRegistry(image: ImageInfo): Promise<vo
 async function deleteContext(contextName: string): Promise<void> {
   const config = kubeconfig.createOrLoadFromFile(extensionApi.kubernetes.getKubeconfig().fsPath);
   const context = config.getContextObject(contextName);
+  if (!context) return;
   const cluster = config.getCluster(context.cluster);
   const user = config.getUser(context.user);
   config.getContexts().splice(config.getContexts().indexOf(context), 1);
-  config.getClusters().splice(config.getClusters().indexOf(cluster), 1);
-  config.getUsers().splice(config.getUsers().indexOf(user), 1);
+  if (cluster) {
+    config.getClusters().splice(config.getClusters().indexOf(cluster), 1);
+  }
+  if (user) {
+    config.getUsers().splice(config.getUsers().indexOf(user), 1);
+  }
   kubeconfig.exportToFile(config, extensionApi.kubernetes.getKubeconfig().fsPath);
 }
 
 function deleteConnection(contextName: string) {
   const deletedConnection = registeredConnections.get(contextName);
   registeredConnections.delete(contextName);
-  deletedConnection.disposable.dispose();
+  deletedConnection?.disposable?.dispose();
 }
 
 async function deleteConnectionAndUpdateKubeconfig(contextName: string): Promise<void> {
@@ -152,7 +158,7 @@ async function registerConnection(contextName: string, apiURL: string, token: st
   // const status = await getConnectionStatus(apiURL, token);
   const connection = {
     name: contextName,
-    status: () => registeredConnections.get(contextName).status,
+    status: () => registeredConnections.get(contextName)?.status ?? UnknownStatus,
     endpoint: {
       apiURL,
     },
@@ -169,7 +175,7 @@ async function registerConnection(contextName: string, apiURL: string, token: st
 }
 
 export async function getDevSandboxSignUpStatus(idToken: string): Promise<SBSignupResponse> {
-  let status: SBSignupResponse;
+  let status: SBSignupResponse | undefined;
   try {
     status = await getSignUpStatus(idToken);
   } catch (error) {
@@ -195,6 +201,10 @@ export async function getDevSandboxSignUpStatus(idToken: string): Promise<SBSign
     }
   }
 
+  if (!status) {
+    throw new Error(`Couldn't get Developer Sandbox status.`);
+  }
+
   if (!status.status.ready) {
     // If Developer Sandbox is not ready
     if (status.status.verificationRequired) {
@@ -343,7 +353,7 @@ export function deactivate(): void {
 }
 
 async function updateConnections(): Promise<void> {
-  let config: KubeConfig;
+  let config: KubeConfig | undefined;
   let attempts = 0;
   while (attempts < 5) {
     try {
@@ -367,16 +377,15 @@ async function updateConnections(): Promise<void> {
     contextName => !config.getContexts().find(context => context.name === contextName),
   );
   deletedConnections.forEach(contextName => {
-    const deletedConnection = registeredConnections.get(contextName);
     deleteConnection(contextName);
-    deletedConnection.disposable.dispose();
   });
 
-  // update status of existin connections
   const updateStatusRequests = Array.from(registeredConnections.keys()).map(contextName => {
-    // get current token from config file
-    const token = config.getUser(config.getContextObject(contextName).user).token;
+    const contextObj = config.getContextObject(contextName);
+    const userObj = contextObj ? config.getUser(contextObj.user) : undefined;
+    const token = userObj?.token ?? '';
     const connectionData = registeredConnections.get(contextName);
+    if (!connectionData) return Promise.resolve();
     return getConnectionStatus(connectionData.connection.endpoint.apiURL, token).then(status => {
       connectionData.status = status;
     });
@@ -391,9 +400,11 @@ async function updateConnections(): Promise<void> {
     .filter(context => context.cluster.startsWith('sandbox-cluster-'))
     .filter(context => !registeredConnections.get(context.name));
   await Promise.all(
-    addedSandboxContexts.map(context => {
+    addedSandboxContexts.map(async context => {
       const cluster = config.getCluster(context.cluster);
-      return registerConnection(context.name, cluster.server, config.getUser(context.user).token);
+      const user = config.getUser(context.user);
+      if (!cluster || !user?.token) return;
+      return registerConnection(context.name, cluster.server, user.token);
     }),
   );
 }

src/openshift.ts

@@ -49,8 +49,17 @@ export async function whoami(clusterUrl: string, token: string): Promise<string>
 export async function getOpenShiftInternalRegistryPublicHost(contextName: string): Promise<InternalRegistryInfo> {
   const config = kubeconfig.createOrLoadFromFile(extensionApi.kubernetes.getKubeconfig().fsPath);
   const context = config.getContextObject(contextName);
+  if (!context) {
+    throw new Error(`Context '${contextName}' not found in kubeconfig.`);
+  }
   const cluster = config.getCluster(context.cluster);
+  if (!cluster) {
+    throw new Error(`Cluster for context '${contextName}' not found in kubeconfig.`);
+  }
   const user = config.getUser(context.user);
+  if (!user || !user.token) {
+    throw new Error(`User or token for context '${contextName}' not found in kubeconfig.`);
+  }
   const gotOptions = {
     headers: {
       Authorization: `Bearer ${user.token}`,
@@ -118,11 +127,15 @@ async function installPipelineSecretToken(
   pipelineServiceAccount: V1ServiceAccount,
   username: string,
 ): Promise<V1Secret | undefined> {
+  if (!pipelineServiceAccount.metadata?.name || !pipelineServiceAccount.metadata?.uid) {
+    throw new Error('Service account is missing required metadata.');
+  }
+  const secretName = `pipeline-secret-${username}-dev`;
   const v1Secret = {
     apiVersion: 'v1',
     kind: 'Secret',
     metadata: {
-      name: `pipeline-secret-${username}-dev`,
+      name: secretName,
       annotations: {
         'kubernetes.io/service-account.name': pipelineServiceAccount.metadata.name,
         'kubernetes.io/service-account.uid': pipelineServiceAccount.metadata.uid,
@@ -132,19 +145,19 @@ async function installPipelineSecretToken(
   } as V1Secret;
 
   await k8sApi.createNamespacedSecret({ namespace: `${username}-dev`, body: v1Secret });
-  // wait for secret to be created
   const timeout = getRegistrationServiceTimeout();
   const startTime = Date.now();
   while (Date.now() - startTime < timeout) {
     try {
       const response = await k8sApi.readNamespacedSecret({
-        name: v1Secret.metadata.name,
+        name: secretName,
         namespace: `${username}-dev`,
       });
-      return response; // Return the Secret object if found
+      return response;
     } catch (error) {
-      if (error.response && error.response.statusCode === 404) {
-        console.error(`Cannot read created sandbox secret ${v1Secret.metadata.name}`);
+      const err = error as { response?: { statusCode?: number } };
+      if (err.response?.statusCode === 404) {
+        console.error(`Cannot read created sandbox secret ${secretName}`);
         await delay(250);
       } else {
         console.error(String(error));
@@ -163,20 +176,23 @@ export async function getPipelineServiceAccountToken(
   const k8sApi = kcu.makeApiClient(CoreV1Api);
   const serviceAccounts = await k8sApi.listNamespacedServiceAccount({ namespace: `${username}-dev` });
   const pipelineServiceAccount = serviceAccounts.items.find(
-    serviceAccount => serviceAccount.metadata.name === 'pipeline',
-  );
+    serviceAccount => serviceAccount.metadata?.name === 'pipeline',
+  ) as V1ServiceAccount | undefined;
   if (!pipelineServiceAccount) {
     throw new Error(`Couldn't find service account required to create Developer Sandbox connection.`);
   }
 
   const secrets = await k8sApi.listNamespacedSecret({ namespace: `${username}-dev` });
-  let pipelineTokenSecret = secrets?.items.find(secret => secret.metadata.name === `pipeline-secret-${username}-dev`);
+  let pipelineTokenSecret = secrets?.items.find(secret => secret.metadata?.name === `pipeline-secret-${username}-dev`);
   if (!pipelineTokenSecret) {
     try {
       pipelineTokenSecret = await installPipelineSecretToken(k8sApi, pipelineServiceAccount, username);
     } catch (error) {
       throw new Error(`An error occurred when creating secret for Developer Sandbox connection.`);
     }
   }
+  if (!pipelineTokenSecret?.data?.token) {
+    throw new Error('Failed to get required service account token.');
+  }
   return Buffer.from(pipelineTokenSecret.data.token, 'base64').toString();
 }

src/sandbox.ts

@@ -56,7 +56,7 @@ export interface OauthServerInfo {
   code_challenge_methods_supported: string[];
 }
 
-export function getSandboxAPIUrl(): string {
+export function getSandboxAPIUrl(): string | undefined {
   return configuration.getConfiguration('redhat').get('sandbox.registrationServiceUrl');
 }