fix(fly-node): use spawn pty for interactive prompts

closed COD-239

Author: hakalb

packages/fly-node/src/lib/fly.class.spec.ts

@@ -1,10 +1,10 @@
 import { existsSync } from 'fs';
 import process from 'process';
 
-import { SpawnOptions, spawn } from '@codeware/core/utils';
+import { SpawnOptions, spawn, spawnPty } from '@codeware/core/utils';
 import { Mock, beforeEach, describe, expect, it, vi } from 'vitest';
 
-import { Fly } from './fly.class';
+import { ExecFlyOptions, Fly } from './fly.class';
 import {
   mockAppsListResponse,
   mockCreateAppResponse,
@@ -30,7 +30,8 @@ vi.mock('os');
 vi.mock('@codeware/core/utils', async () => ({
   // Only mock `spawn` function
   ...(await vi.importActual('@codeware/core/utils')),
-  spawn: vi.fn()
+  spawn: vi.fn(),
+  spawnPty: vi.fn()
 }));
 
 describe('Fly', () => {
@@ -64,6 +65,7 @@ describe('Fly', () => {
   };
 
   const mockSpawn = vi.mocked(spawn);
+  const mockSpawnPty = vi.mocked(spawnPty);
   const mockExistsSync = vi.mocked(existsSync);
 
   /**
@@ -86,7 +88,7 @@ describe('Fly', () => {
   const assertSpawn = (
     matchArgs: 'exact' | 'not' | 'some',
     args: Array<string>,
-    options?: SpawnOptions & {
+    options?: ExecFlyOptions & {
       /** The strategy to use for the `--access-token` flag */
       accessTokenStrategy?: 'append-to-args' | 'leave-as-is';
     }
@@ -101,10 +103,13 @@ describe('Fly', () => {
       optionsToCheck = options;
     }
 
+    // Determine which mock to use
+    const mockSpawnUsed = options?.prompt ? mockSpawnPty : mockSpawn;
+
     // Check negative test first - ignore options
     if (matchArgs === 'not') {
       // Assert that the call was not made
-      expect(mockSpawn).not.toHaveBeenCalledWith(
+      expect(mockSpawnUsed).not.toHaveBeenCalledWith(
         expect.stringMatching(/^fly/),
         expect.arrayContaining(args),
         expect.anything()
@@ -123,7 +128,7 @@ describe('Fly', () => {
         : args;
 
     // Find call that matches our arguments
-    const matchingCall = mockSpawn.mock.calls.find(
+    const matchingCall = mockSpawnUsed.mock.calls.find(
       (call) =>
         call[0].match(/^fly/) &&
         argsToCheck.every((arg) => call[1].includes(arg))
@@ -132,7 +137,7 @@ describe('Fly', () => {
     // Assert normally when a call could not be found.
     // It should fail but provide a better DX to the user.
     if (!matchingCall) {
-      expect(mockSpawn).toHaveBeenCalledWith(
+      expect(mockSpawnUsed).toHaveBeenCalledWith(
         expect.stringMatching(/^fly/),
         expect.arrayContaining(argsToCheck),
         optionsToCheck

packages/fly-node/src/lib/fly.class.ts

@@ -2,7 +2,12 @@ import { existsSync } from 'fs';
 import { join } from 'path';
 import { cwd } from 'process';
 
-import { SpawnOptions, spawn } from '@codeware/core/utils';
+import {
+  SpawnOptions,
+  SpawnPtyOptions,
+  spawn,
+  spawnPty
+} from '@codeware/core/utils';
 import { ZodError } from 'zod';
 
 import { AppsCreateTransformedResponseSchema } from './schemas/apps-create.schema';
@@ -42,6 +47,8 @@ import type {
   UnsetAppSecretsOptions
 } from './types';
 
+export type ExecFlyOptions = SpawnOptions & SpawnPtyOptions;
+
 /**
  * Manages deployments to Fly.io using the `flyctl` CLI tool.
  */
@@ -171,7 +178,8 @@ export class Fly {
           this.logger.info(
             `Detaching Postgres cluster '${cluster}' from application '${app}'`
           );
-          await this.detachPostgres(cluster, app);
+          const output = await this.detachPostgres(cluster, app);
+          this.logger.info(output);
         }
         // Now it's safe to destroy the app
         await this.destroyApp(app, options?.force);
@@ -331,6 +339,11 @@ export class Fly {
       );
     }
   };
+  postgres = {
+    detach: async (cluster: string, app: string) => {
+      await this.detachPostgres(cluster, app);
+    }
+  };
   /**
    * Manage application secrets
    */
@@ -667,13 +680,14 @@ export class Fly {
 
   /**
    * @private
+   * @returns Log output from the detach process
    * @throws An error if the Postgres database cannot be detached from an app
    */
-  private async detachPostgres(postgres: string, app: string): Promise<void> {
+  private async detachPostgres(postgres: string, app: string): Promise<string> {
     const args = ['postgres', 'detach', postgres, '--app', app];
 
     // Prompt for confirmation if the database name
-    await this.execFly(args, {
+    return await this.execFly<string>(args, {
       prompt: (output) => {
         if (output.match(/select .* detach/i)) {
           return app.replace(/-/g, '_');
@@ -1136,9 +1150,9 @@ ${JSON.stringify(response, null, 2)}`);
    * @returns The JSON output of the command, or the raw output if not JSON
    * @throws An error if the command fails
    */
-  private async execFly<T = void>(
+  private async execFly<T>(
     command: string | Array<string>,
-    options?: SpawnOptions
+    options?: ExecFlyOptions
   ): Promise<T>;
   /**
    * @private
@@ -1152,15 +1166,15 @@ ${JSON.stringify(response, null, 2)}`);
    * @returns The JSON output of the command, or the raw output if not JSON
    * @throws An error if the command fails and `onError` is `throwOnError`
    */
-  private async execFly<T = null>(
+  private async execFly<T>(
     command: string | Array<string>,
-    options?: SpawnOptions,
+    options?: ExecFlyOptions,
     onError?: 'nullOnError',
     skipToken?: boolean
   ): Promise<T>;
   private async execFly<T>(
     command: string | Array<string>,
-    options?: SpawnOptions,
+    options?: ExecFlyOptions,
     onError: 'nullOnError' | 'throwOnError' = 'throwOnError',
     skipToken?: boolean
   ): Promise<T> {
@@ -1176,14 +1190,21 @@ ${JSON.stringify(response, null, 2)}`);
 
     const flyCmd = 'flyctl';
     const toLogCmd = `${flyCmd} ${args.join(' ')}`;
-    let stdout = '';
+    let output = '';
 
     try {
       this.traceCLI('CALL', toLogCmd);
-      const result = await spawn(flyCmd, args, options);
-      stdout = result.stdout.trim();
-      const stderr = result.stderr.trim();
-      this.traceCLI('RESULT', `${stdout}${stderr ? `\n${stderr}` : ''}`);
+      // If the command requires interactive input, use spawnPty
+      if (options?.prompt) {
+        output = await spawnPty(flyCmd, args, { prompt: options.prompt });
+        this.traceCLI('RESULT', output);
+      } else {
+        // Otherwise use spawn
+        const result = await spawn(flyCmd, args, options);
+        output = result.stdout.trim();
+        const stderr = result.stderr.trim();
+        this.traceCLI('RESULT', `${output}${stderr ? `\n${stderr}` : ''}`);
+      }
     } catch (error) {
       const errorMsg = (error as Error).message;
       this.traceCLI('RESULT', errorMsg);
@@ -1195,13 +1216,13 @@ ${errorMsg}`);
     }
 
     try {
-      return JSON.parse(stdout) as T;
+      return JSON.parse(output) as T;
     } catch {
-      if (stdout) {
+      if (output) {
         if (this.logger.traceCLI) {
           this.logger.info(`Failed to parse as JSON, returning raw output`);
         }
-        return stdout as T;
+        return output as T;
       }
     }
     return undefined as T;

packages/nx-fly-deployment-action/src/lib/fly-deployment.spec.ts

@@ -27,6 +27,9 @@ vi.mock('fs', async () => {
     __esModule: true
   };
 });
+vi.mock('@homebridge/node-pty-prebuilt-multiarch', () => ({
+  spawn: vi.fn()
+}));
 vi.mock('@actions/core');
 vi.mock('@actions/exec');
 vi.mock('@actions/github', () => ({

packages/nx-fly-deployment-action/src/lib/main.spec.ts

@@ -13,6 +13,9 @@ import * as main from './main';
 import type { ActionInputs } from './schemas/action-inputs.schema';
 import type { ActionOutputs } from './schemas/action-outputs.schema';
 
+vi.mock('@homebridge/node-pty-prebuilt-multiarch', () => ({
+  spawn: vi.fn()
+}));
 vi.mock('@actions/core');
 vi.mock('./fly-deployment');