Better handling when updating from LOCATION to system param

CHANGELOG.md

@@ -1 +1,2 @@
-- Fix bug where functions:secrets:\* family of commands did not work when Firebase CLI is authenticated via GOOGLE_APPLICATION_CREDENTIALS (#6190)
+- Fixed bug where `functions:secrets:\*` family of commands did not work when Firebase CLI is authenticated via GOOGLE_APPLICATION_CREDENTIALS (#6190)
+- Fixed bug where some extension instance updates would default to the wrong location.

src/commands/ext-configure.ts

@@ -76,9 +76,10 @@ export const command = new Command("ext:configure <extensionInstanceId>")
       instanceId,
       projectDir: config.projectDir,
     });
+    const params = (spec.params ?? []).concat(spec.systemParams ?? []);
     const [immutableParams, tbdParams] = partition(
-      (spec.params ?? []).concat(spec.systemParams ?? []),
-      (param) => param.immutable ?? false
+      params,
+      (param) => (param.immutable && !!oldParamValues[param.param]) ?? false
     );
     infoImmutableParams(immutableParams, oldParamValues);
 

src/extensions/paramHelper.ts

@@ -4,11 +4,10 @@ import * as fs from "fs-extra";
 
 import { FirebaseError } from "../error";
 import { logger } from "../logger";
-import { ExtensionInstance, ExtensionSpec, Param } from "./types";
+import { ExtensionSpec, Param } from "./types";
 import { getFirebaseProjectParams, substituteParams } from "./extensionsHelper";
 import * as askUserForParam from "./askUserForParam";
 import * as env from "../functions/env";
-import { cloneDeep } from "../utils";
 
 const NONINTERACTIVE_ERROR_MESSAGE =
   "As of firebase-tools@11, `ext:install`, `ext:update` and `ext:configure` are interactive only commands. " +
@@ -61,26 +60,19 @@ export function buildBindingOptionsWithBaseValue(baseParams: { [key: string]: st
  * @param newDefaults a map of { PARAM_NAME: default_value }
  */
 export function setNewDefaults(params: Param[], newDefaults: { [key: string]: string }): Param[] {
-  params.forEach((param) => {
-    if (newDefaults[param.param.toUpperCase()]) {
-      param.default = newDefaults[param.param.toUpperCase()];
+  for (const param of params) {
+    if (newDefaults[param.param]) {
+      param.default = newDefaults[param.param];
+    } else if (
+      (param.param = `firebaseextensions.v1beta.function/location` && newDefaults["LOCATION"])
+    ) {
+      // Special case handling for when we are updating from LOCATION to system param location.
+      param.default = newDefaults["LOCATION"];
     }
-  });
+  }
   return params;
 }
 
-/**
- * Returns a copy of the params for a extension instance with the defaults set to the instance's current param values
- * @param extensionInstance the extension instance to change the default params of
- */
-export function getParamsWithCurrentValuesAsDefaults(
-  extensionInstance: ExtensionInstance
-): Param[] {
-  const specParams = cloneDeep(extensionInstance?.config?.source?.spec?.params || []);
-  const currentParams = cloneDeep(extensionInstance?.config?.params || {});
-  return setNewDefaults(specParams, currentParams);
-}
-
 /**
  * Gets params from the user
  * or prompting the user for each param.
@@ -160,15 +152,37 @@ export async function promptForNewParams(args: {
     return left.filter((aLeft) => !right.find(sameParam(aLeft)));
   };
 
+  let combinedOldParams = args.spec.params.concat(
+    args.spec.systemParams.filter((p) => !p.advanced) ?? []
+  );
+  let combinedNewParams = args.newSpec.params.concat(
+    args.newSpec.systemParams.filter((p) => !p.advanced) ?? []
+  );
+
+  // Special case for updating from LOCATION to system param location
+  if (
+    combinedOldParams.some((p) => p.param === "LOCATION") &&
+    combinedNewParams.some((p) => p.param === "firebaseextensions.v1beta.function/location") &&
+    !!args.currentParams["LOCATION"]
+  ) {
+    newParamBindingOptions["firebaseextensions.v1beta.function/location"] = {
+      baseValue: args.currentParams["LOCATION"],
+    };
+    delete newParamBindingOptions["LOCATION"];
+    combinedOldParams = combinedOldParams.filter((p) => p.param !== "LOCATION");
+    combinedNewParams = combinedNewParams.filter(
+      (p) => p.param !== "firebaseextensions.v1beta.function/location"
+    );
+  }
+
   // Some params are in the spec but not in currentParams, remove so we can prompt for them.
-  const oldParams = args.spec.params.filter((p) =>
+  const oldParams = combinedOldParams.filter((p) =>
     Object.keys(args.currentParams).includes(p.param)
   );
-
-  let paramsDiffDeletions = paramDiff(oldParams, args.newSpec.params);
+  let paramsDiffDeletions = paramDiff(oldParams, combinedNewParams);
   paramsDiffDeletions = substituteParams<Param[]>(paramsDiffDeletions, firebaseProjectParams);
 
-  let paramsDiffAdditions = paramDiff(args.newSpec.params, oldParams);
+  let paramsDiffAdditions = paramDiff(combinedNewParams, oldParams);
   paramsDiffAdditions = substituteParams<Param[]>(paramsDiffAdditions, firebaseProjectParams);
 
   if (paramsDiffDeletions.length) {

src/test/extensions/paramHelper.spec.ts

@@ -3,7 +3,7 @@ import * as sinon from "sinon";
 import * as fs from "fs-extra";
 
 import { FirebaseError } from "../../error";
-import { ExtensionInstance, Param, ParamType } from "../../extensions/types";
+import { ExtensionSpec, Param, ParamType } from "../../extensions/types";
 import * as extensionsHelper from "../../extensions/extensionsHelper";
 import * as paramHelper from "../../extensions/paramHelper";
 import * as prompt from "../../prompt";
@@ -63,7 +63,7 @@ const TEST_PARAMS_3: Param[] = [
   },
 ];
 
-const SPEC = {
+const SPEC: ExtensionSpec = {
   name: "test",
   version: "0.1.0",
   roles: [],
@@ -152,96 +152,6 @@ describe("paramHelper", () => {
     });
   });
 
-  describe("getParamsWithCurrentValuesAsDefaults", () => {
-    let params: { [key: string]: string };
-    let testInstance: ExtensionInstance;
-    beforeEach(() => {
-      params = { A_PARAMETER: "new default" };
-      testInstance = {
-        config: {
-          source: {
-            state: "ACTIVE",
-            name: "",
-            packageUri: "",
-            hash: "",
-            spec: {
-              name: "",
-              version: "0.1.0",
-              roles: [],
-              resources: [],
-              params: [...TEST_PARAMS],
-              systemParams: [],
-              sourceUrl: "",
-            },
-          },
-          name: "test",
-          createTime: "now",
-          params,
-          systemParams: {},
-        },
-        name: "test",
-        createTime: "now",
-        updateTime: "now",
-        state: "ACTIVE",
-        serviceAccountEmail: "test@test.com",
-      };
-
-      it("should add defaults to params without them using the current state and leave other values unchanged", () => {
-        const newParams = paramHelper.getParamsWithCurrentValuesAsDefaults(testInstance);
-
-        expect(newParams).to.eql([
-          {
-            param: "A_PARAMETER",
-            label: "Param",
-            default: "new default",
-            type: ParamType.STRING,
-            required: true,
-          },
-          {
-            param: "ANOTHER_PARAMETER",
-            label: "Another",
-            default: "default",
-            type: ParamType.STRING,
-          },
-        ]);
-      });
-    });
-
-    it("should change existing defaults to the current state and leave other values unchanged", () => {
-      (testInstance.config?.source?.spec?.params || []).push({
-        param: "THIRD",
-        label: "3rd",
-        default: "default",
-        type: ParamType.STRING,
-      });
-      testInstance.config.params.THIRD = "New Default";
-      const newParams = paramHelper.getParamsWithCurrentValuesAsDefaults(testInstance);
-
-      expect(newParams).to.eql([
-        {
-          param: "A_PARAMETER",
-          label: "Param",
-          default: "new default",
-          type: ParamType.STRING,
-          required: true,
-        },
-        {
-          param: "ANOTHER_PARAMETER",
-          label: "Another Param",
-          default: "default",
-          required: true,
-          type: ParamType.STRING,
-        },
-        {
-          param: "THIRD",
-          label: "3rd",
-          default: "New Default",
-          type: ParamType.STRING,
-        },
-      ]);
-    });
-  });
-
   describe("promptForNewParams", () => {
     let promptStub: sinon.SinonStub;
 
@@ -319,6 +229,41 @@ describe("paramHelper", () => {
       expect(newParams).to.eql(expected);
     });
 
+    it("should map LOCATION to system param location and not prompt for it", async () => {
+      promptStub.resolves("user input");
+      const oldSpec = cloneDeep(SPEC);
+      const newSpec = cloneDeep(SPEC);
+      oldSpec.params = [
+        {
+          param: "LOCATION",
+          label: "",
+        },
+      ];
+      newSpec.params = [];
+      newSpec.systemParams = [
+        {
+          param: "firebaseextensions.v1beta.function/location",
+          label: "",
+        },
+      ];
+
+      const newParams = await paramHelper.promptForNewParams({
+        spec: oldSpec,
+        newSpec,
+        currentParams: {
+          LOCATION: "us-east1",
+        },
+        projectId: PROJECT_ID,
+        instanceId: INSTANCE_ID,
+      });
+
+      const expected = {
+        "firebaseextensions.v1beta.function/location": { baseValue: "us-east1" },
+      };
+      expect(newParams).to.eql(expected);
+      expect(promptStub).not.to.have.been.called;
+    });
+
     it("should not prompt the user for params that did not change type or param", async () => {
       promptStub.resolves("Fail");
       const newSpec = cloneDeep(SPEC);