Better handling when updating from LOCATION to system param (#6202)

* Better handling when updating from LOCATION to system param * test fix * Format * Use the correct spec in test

Author: joehan

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);