Fix bug where function deployment no-oped for functions in bad state. (#8289)

* Fix bug where function deploy skipped even when function is broken.

* Add test.

* Don't need this.

* Add changelog.

* Run formatter

* Fix tests.

* Formatter.

Author: taeold

CHANGELOG.md

@@ -1,5 +1,6 @@
 - Fixed issue where `apps:init` fails to detect the output directory when it was run in a directory where `app` was the only module.
 - Set `LOG_EXECUTION_ID=true` by default for Cloud Functions (2nd gen) to improve debugging by displaying execution IDs in logs. (#8276)
+- Fix bug where function deployment no-oped for functions in bad state. (#8289)
 - Updated the Firebase Data Connect local toolkit to v1.8.4 which includes the following changes: (#8290)
   - React hooks for mutations without args no longer require `undefined` to be passed when calling `mutate`.
   - Fixed import resolution when `moduleResolution` is set to `bundler`.

src/deploy/functions/backend.spec.ts

@@ -22,6 +22,7 @@ describe("Backend", () => {
     entryPoint: "function",
     runtime: "nodejs16",
     codebase: projectConfig.DEFAULT_CODEBASE,
+    state: "ACTIVE",
   };
 
   const CLOUD_FUNCTION: Omit<gcf.CloudFunction, gcf.OutputOnlyFields> = {

src/deploy/functions/backend.ts

@@ -341,6 +341,8 @@ export function isBlockingTriggered(triggered: Triggered): triggered is Blocking
   return {}.hasOwnProperty.call(triggered, "blockingTrigger");
 }
 
+export type EndpointState = "ACTIVE" | "FAILED" | "DEPLOYING" | "DELETING" | "UNKONWN";
+
 /**
  * An endpoint that serves traffic to a stack of services.
  * For now, this is always a Cloud Function. Future iterations may use complex
@@ -381,6 +383,9 @@ export type Endpoint = TargetIds &
     // This may eventually be different than id because GCF is going to start
     // doing name translations
     runServiceId?: string;
+
+    // State of the endpoint.
+    state?: EndpointState;
   };
 
 export interface RequiredAPI {

src/deploy/functions/release/planner.spec.ts

@@ -36,6 +36,7 @@ describe("planner", () => {
       runtime: "nodejs16",
       entryPoint: "function",
       environmentVariables: {},
+      state: "ACTIVE",
     } as backend.Endpoint;
   }
 
@@ -137,7 +138,98 @@ describe("planner", () => {
     });
   });
 
-  describe("calculateRegionalChanges", () => {
+  describe("toSkipPredicate", () => {
+    it("should skip functions with matching hashes", () => {
+      const wantFn = func("skip", "region");
+      const haveFn = func("skip", "region");
+      wantFn.hash = "same-hash";
+      haveFn.hash = "same-hash";
+
+      const want = { skip: wantFn };
+      const have = { skip: haveFn };
+
+      const result = planner.calculateChangesets(want, have, (e) => e.region);
+
+      expect(result.region.endpointsToSkip).to.have.lengthOf(1);
+      expect(result.region.endpointsToSkip[0].id).to.equal("skip");
+    });
+
+    it("should not skip functions with different hashes", () => {
+      const funcWant = func("func", "region");
+      const funcHave = func("func", "region");
+      funcWant.hash = "local-hash";
+      funcHave.hash = "server-hash";
+
+      const want = { func: funcWant };
+      const have = { func: funcHave };
+
+      const result = planner.calculateChangesets(want, have, (e) => e.region);
+
+      expect(result.region.endpointsToSkip).to.have.lengthOf(0);
+      expect(result.region.endpointsToUpdate).to.have.lengthOf(1);
+      expect(result.region.endpointsToUpdate[0].endpoint.id).to.equal("func");
+    });
+
+    it("should not skip functions with missing hash values", () => {
+      const func1Want = func("func1", "region");
+      const func1Have = func("func1", "region");
+      func1Want.hash = "hash";
+      // func1Have has no hash
+
+      const func2Want = func("func2", "region");
+      const func2Have = func("func2", "region");
+      // func2Want has no hash
+      func2Have.hash = "hash";
+
+      // Neither has hash
+      const func3Want = func("func3", "region");
+      const func3Have = func("func3", "region");
+
+      const want = { func1: func1Want, func2: func2Want, func3: func3Want };
+      const have = { func1: func1Have, func2: func2Have, func3: func3Have };
+
+      const result = planner.calculateChangesets(want, have, (e) => e.region);
+
+      expect(result.region.endpointsToSkip).to.have.lengthOf(0);
+      expect(result.region.endpointsToUpdate).to.have.lengthOf(3);
+    });
+
+    it("should not skip functions targeted by --only flag", () => {
+      const funcWant = func("func", "region");
+      const funcHave = func("func", "region");
+      funcWant.hash = "same-hash";
+      funcHave.hash = "same-hash";
+      funcWant.targetedByOnly = true;
+
+      const want = { func: funcWant };
+      const have = { func: funcHave };
+
+      const result = planner.calculateChangesets(want, have, (e) => e.region);
+
+      expect(result.region.endpointsToSkip).to.have.lengthOf(0);
+      expect(result.region.endpointsToUpdate).to.have.lengthOf(1);
+      expect(result.region.endpointsToUpdate[0].endpoint.id).to.equal("func");
+    });
+
+    it("should not skip functions in broken state", () => {
+      const funcWant = func("func", "region");
+      const funcHave = func("func", "region");
+      funcWant.hash = "same-hash";
+      funcHave.hash = "same-hash";
+      funcHave.state = "FAILED";
+
+      const want = { func: funcWant };
+      const have = { func: funcHave };
+
+      const result = planner.calculateChangesets(want, have, (e) => e.region);
+
+      expect(result.region.endpointsToSkip).to.have.lengthOf(0);
+      expect(result.region.endpointsToUpdate).to.have.lengthOf(1);
+      expect(result.region.endpointsToUpdate[0].endpoint.id).to.equal("func");
+    });
+  });
+
+  describe("calculateChangesets", () => {
     it("passes a smoke test", () => {
       const created = func("created", "region");
       const updated = func("updated", "region");
@@ -291,6 +383,24 @@ describe("planner", () => {
         },
       });
     });
+
+    it("logs a message when skipping functions", () => {
+      // Create functions with matching hashes that will be skipped
+      const skipWant = func("skip", "region");
+      const skipHave = func("skip", "region");
+      skipWant.hash = "hash";
+      skipHave.hash = "hash";
+
+      const want = { skip: skipWant };
+      const have = { skip: skipHave };
+
+      planner.calculateChangesets(want, have, (e) => e.region);
+
+      expect(logLabeledBullet).to.have.been.calledWith(
+        "functions",
+        "Skipping the deploy of unchanged functions.",
+      );
+    });
   });
 
   describe("createDeploymentPlan", () => {

src/deploy/functions/release/planner.ts

@@ -58,6 +58,7 @@ export function calculateChangesets(
   const toSkipPredicate = (id: string): boolean =>
     !!(
       !want[id].targetedByOnly && // Don't skip the function if its --only targeted.
+      have[id].state === "ACTIVE" && // Only skip the function if its in a known good state
       have[id].hash &&
       want[id].hash &&
       want[id].hash === have[id].hash

src/gcp/cloudfunctions.spec.ts

@@ -24,6 +24,7 @@ describe("cloudfunctions", () => {
     entryPoint: "function",
     runtime: "nodejs16",
     codebase: projectConfig.DEFAULT_CODEBASE,
+    state: "ACTIVE",
   };
 
   const CLOUD_FUNCTION: Omit<cloudfunctions.CloudFunction, cloudfunctions.OutputOnlyFields> = {

src/gcp/cloudfunctions.ts

@@ -576,6 +576,18 @@ export function endpointFromFunction(gcfFunction: CloudFunction): backend.Endpoi
   if (gcfFunction.labels?.[HASH_LABEL]) {
     endpoint.hash = gcfFunction.labels[HASH_LABEL];
   }
+  proto.convertIfPresent(endpoint, gcfFunction, "state", "status", (status) => {
+    if (status === "ACTIVE") {
+      return "ACTIVE";
+    } else if (status === "OFFLINE") {
+      return "FAILED";
+    } else if (status === "DEPLOY_IN_PROGRESS") {
+      return "DEPLOYING";
+    } else if (status === "DELETE_IN_PROGRESS") {
+      return "DELETING";
+    }
+    return "UNKONWN";
+  });
   return endpoint;
 }
 

src/gcp/cloudfunctionsv2.spec.ts

@@ -31,6 +31,7 @@ describe("cloudfunctionsv2", () => {
     codebase: projectConfig.DEFAULT_CODEBASE,
     runServiceId: "service",
     source: { storageSource: CLOUD_FUNCTION_V2_SOURCE },
+    state: "ACTIVE",
   };
 
   const CLOUD_FUNCTION_V2: cloudfunctionsv2.InputCloudFunction = {

src/gcp/cloudfunctionsv2.ts

@@ -795,5 +795,6 @@ export function endpointFromFunction(gcfFunction: OutputCloudFunction): backend.
   if (gcfFunction.labels?.[HASH_LABEL]) {
     endpoint.hash = gcfFunction.labels[HASH_LABEL];
   }
+  proto.copyIfPresent(endpoint, gcfFunction, "state");
   return endpoint;
 }