fix: prompt for existing instance as soon as instance name is known

prompting for existing instance overwrite happened at the end of prompting process, losing all prompting data if refused too quickly and not logical in term of UX as it should be asked as soon as instance name is known
PierreBeucher · Jan 2, 2025 · 81137aa · 81137aa
1 parent 6e6cc2f
commit 81137aa
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 20 deletions.
diff --git a/src/core/initializer.ts b/src/core/initializer.ts
@@ -4,8 +4,7 @@ import { CreateCliArgs } from "./input/cli"
 import { InputPrompter } from "./input/prompter"
 import { InstanceManagerBuilder } from "./manager-builder"
 import { StateInitializer } from "./state/initializer"
-import { confirm } from '@inquirer/prompts';
-import { StateLoader } from "./state/loader"
+import { confirm } from '@inquirer/prompts'
 
 export interface InstancerInitializerArgs {
     provider: CLOUDYPAD_PROVIDER
@@ -46,18 +45,6 @@ export class InteractiveInstanceInitializer {
 
         const input = await this.inputPrompter.completeCliInput(cliArgs)
 
-        const loader = new StateLoader()
-        if(await loader.instanceExists(input.instanceName) && !cliArgs.overwriteExisting){
-            const confirmAlreadyExists = await confirm({
-                message: `Instance ${input.instanceName} already exists. Do you want to overwrite existing instance config?`,
-                default: false,
-            })
-
-            if (!confirmAlreadyExists) {
-                throw new Error("Won't overwrite existing instance. Initialization aborted.")
-            }
-        }
-
         const state = await new StateInitializer({
             input: input,
             provider: this.provider,

diff --git a/src/core/input/prompter.ts b/src/core/input/prompter.ts
@@ -8,6 +8,7 @@ import { CommonInstanceInput } from "../state/state";
 import { getLogger } from "../../log/utils";
 import { PUBLIC_IP_TYPE, PUBLIC_IP_TYPE_DYNAMIC, PUBLIC_IP_TYPE_STATIC } from '../const';
 import { CreateCliArgs } from './cli';
+import { StateLoader } from '../state/loader';
 const { kebabCase } = lodash
 
 export interface InputPrompter {
@@ -19,6 +20,10 @@ export interface InputPrompter {
     completeCliInput(cliArgs: CreateCliArgs): Promise<CommonInstanceInput>
 }
 
+export interface InstanceCreateOptions {
+    overwriteExisting?: boolean
+}
+
 export abstract class AbstractInputPrompter<A extends CreateCliArgs, I extends CommonInstanceInput> implements InputPrompter {
 
     protected readonly logger = getLogger(AbstractInputPrompter.name)
@@ -27,18 +32,27 @@ export abstract class AbstractInputPrompter<A extends CreateCliArgs, I extends C
      * Prompt user for additional provider-specific inputs based on common provider inputs.
      * Returns a fully valid state for instance initialization. 
      */
-    async promptInput(partialInput: PartialDeep<I>): Promise<I> {
-        const commonInput = await this.promptCommonInput(partialInput)
+    async promptInput(partialInput: PartialDeep<I>, createOptions: InstanceCreateOptions): Promise<I> {
+        const commonInput = await this.promptCommonInput(partialInput, createOptions)
         const commonInputWithPartial = lodash.merge({}, commonInput, partialInput)
         const finalInput = await this.promptSpecificInput(commonInputWithPartial)
         return finalInput
     }
 
-    private async promptCommonInput(partialInput: PartialDeep<CommonInstanceInput>): Promise<CommonInstanceInput> {
+    private async promptCommonInput(partialInput: PartialDeep<CommonInstanceInput>, createOptions: InstanceCreateOptions): Promise<CommonInstanceInput> {
 
         this.logger.debug(`Initializing instance with default config ${JSON.stringify(partialInput)}`)
 
         const instanceName = await this.instanceName(partialInput.instanceName)
+
+        const alreadyExists = await new StateLoader().instanceExists(instanceName)
+        if(alreadyExists){
+            const overwriteExisting = await this.promptOverwriteExisting(instanceName, createOptions?.overwriteExisting ?? false)
+            if(!overwriteExisting) {
+                throw new Error(`Won't overwrite existing instance ${instanceName}. Initialization aborted.`)
+            }
+        }
+
         const sshKey = await this.privateSshKey(partialInput.provision?.ssh?.privateKeyPath)
         const sshUser = "ubuntu" // Harcoded default for now since we only support Ubuntu
 
@@ -67,7 +81,7 @@ export abstract class AbstractInputPrompter<A extends CreateCliArgs, I extends C
 
         const result = this.doTransformCliArgsIntoInput(cliArgs)
 
-        this.logger.debug(`Parsed CLI args ${JSON.stringify(cliArgs)} into ${JSON.stringify(input)}`)
+        this.logger.debug(`Parsed CLI args ${JSON.stringify(cliArgs)} into ${JSON.stringify(result)}`)
 
         return result
     }
@@ -79,7 +93,7 @@ export abstract class AbstractInputPrompter<A extends CreateCliArgs, I extends C
 
     async completeCliInput(cliArgs: A): Promise<I> {
         const partialInput = this.cliArgsIntoInput(cliArgs)
-        const input = await this.promptInput(partialInput)
+        const input = await this.promptInput(partialInput, { overwriteExisting: cliArgs.overwriteExisting })
         return input
     }
 
@@ -111,11 +125,21 @@ export abstract class AbstractInputPrompter<A extends CreateCliArgs, I extends C
                 return this.instanceName()
             }
         }
-
 
         return kebabCaseInstanceName 
     }
 
+    protected async promptOverwriteExisting(instanceName: string, overwriteExisting?: boolean): Promise<boolean> {
+        if (overwriteExisting !== undefined) {
+            return overwriteExisting
+        }
+
+        return await confirm({
+            message: `Instance ${instanceName} already exists. Do you want to overwrite existing instance config?`,
+            default: false,
+        })
+    }
+
     protected async privateSshKey(privateSshKey?: string): Promise<string> {
         if (privateSshKey) {
             return privateSshKey;

diff --git a/test/unit/core/initializer.spec.ts b/test/unit/core/initializer.spec.ts
@@ -46,6 +46,12 @@ describe('Instance initializer', () => {
         spot: TEST_INPUT.provision.useSpot,
     }
 
+    const TEST_CLI_ARGS_ALREADY_EXISTING: GcpCreateCliArgs = {
+        ...TEST_CLI_ARGS,
+        name: "gcp-dummy-already-exists-test",
+        overwriteExisting: false,
+    }
+
     // Check instanceInitializer creates instance state as expected
     // Testing here using GCP state, but Initializer is generic and should work with any statet
     it('should initialize instance state with provided arguments', async () => {
@@ -78,6 +84,24 @@ describe('Instance initializer', () => {
 
         assert.deepEqual(state, expectState)
     })
+
+    it('should failed to initialize for existing instance with no overwrite', async () => {
+
+        // Initialize dummy instance 
+        await new InteractiveInstanceInitializer({ 
+            provider: CLOUDYPAD_PROVIDER_GCP,
+            inputPrompter: new GcpInputPrompter()
+        }).initializeInstance(TEST_CLI_ARGS_ALREADY_EXISTING, { skipPostInitInfo: true })
+
+        await assert.rejects(
+            // Initialize again, should throw exception as overwriteExisting is false
+            new InteractiveInstanceInitializer({ 
+                provider: CLOUDYPAD_PROVIDER_GCP,
+                inputPrompter: new GcpInputPrompter()
+            }).initializeInstance(TEST_CLI_ARGS_ALREADY_EXISTING, { skipPostInitInfo: true }),
+            /Won't overwrite existing instance/
+        )
+    })
 })