Fix schedule function deployment (#1305)

* fixing retry config for scheduled functions

* change task queue behavior and clean up things

* small cleanup

* removing unnecessary '|| {}' from copyIfPresent

* move preserveExternalChanges to runtime options

Author: colerogers

CHANGELOG.md

@@ -1 +1,2 @@
 - Deprecate typoed function name lessThanorEqualTo (#1284)
+- Fix a bug where supplying preserveExternalChanges to scheduled functions would cause deployment failure (#1305).

spec/runtime/manifest.spec.ts

@@ -1,8 +1,16 @@
 import { expect } from "chai";
-import { stackToWire, ManifestStack } from "../../src/runtime/manifest";
+import {
+  stackToWire,
+  ManifestStack,
+  initV2ScheduleTrigger,
+  initV1ScheduleTrigger,
+  initTaskQueueTrigger,
+} from "../../src/runtime/manifest";
+import { RESET_VALUE } from "../../src/common/options";
 import * as params from "../../src/params";
 import * as optsv2 from "../../src/v2/options";
 import * as v1 from "../../src/v1";
+import { DeploymentOptions } from "../../src/v1";
 
 describe("stackToWire", () => {
   afterEach(() => {
@@ -168,3 +176,90 @@ describe("stackToWire", () => {
     expect(stackToWire(stack)).to.deep.equal(expected);
   });
 });
+
+describe("initTaskQueueTrigger", () => {
+  it("should init a taskQueueTrigger without preserveExternalChanges", () => {
+    const tt = initTaskQueueTrigger();
+
+    expect(tt).to.deep.eq({
+      retryConfig: {
+        maxAttempts: RESET_VALUE,
+        maxDoublings: RESET_VALUE,
+        maxBackoffSeconds: RESET_VALUE,
+        maxRetrySeconds: RESET_VALUE,
+        minBackoffSeconds: RESET_VALUE,
+      },
+      rateLimits: {
+        maxConcurrentDispatches: RESET_VALUE,
+        maxDispatchesPerSecond: RESET_VALUE,
+      },
+    });
+  });
+
+  it("should init a taskQueueTrigger with preserveExternalChanges", () => {
+    const opts: DeploymentOptions = { preserveExternalChanges: true };
+
+    const tt = initTaskQueueTrigger(opts);
+
+    expect(tt).to.deep.eq({
+      rateLimits: {},
+      retryConfig: {},
+    });
+  });
+});
+
+describe("initScheduleTrigger", () => {
+  it("should init a v1 scheduleTrigger without preserveExternalChanges", () => {
+    const st = initV1ScheduleTrigger("every 30 minutes");
+
+    expect(st).to.deep.eq({
+      schedule: "every 30 minutes",
+      timeZone: RESET_VALUE,
+      retryConfig: {
+        retryCount: RESET_VALUE,
+        maxDoublings: RESET_VALUE,
+        maxRetryDuration: RESET_VALUE,
+        minBackoffDuration: RESET_VALUE,
+        maxBackoffDuration: RESET_VALUE,
+      },
+    });
+  });
+
+  it("should init a v1 scheduleTrigger with preserveExternalChanges", () => {
+    const opts: DeploymentOptions = { preserveExternalChanges: true };
+
+    const st = initV1ScheduleTrigger("every 30 minutes", opts);
+
+    expect(st).to.deep.eq({
+      schedule: "every 30 minutes",
+      retryConfig: {},
+    });
+  });
+
+  it("should init a v2 scheduleTrigger without preserveExternalChanges", () => {
+    const st = initV2ScheduleTrigger("every 30 minutes");
+
+    expect(st).to.deep.eq({
+      schedule: "every 30 minutes",
+      timeZone: RESET_VALUE,
+      retryConfig: {
+        retryCount: RESET_VALUE,
+        maxDoublings: RESET_VALUE,
+        maxRetrySeconds: RESET_VALUE,
+        minBackoffSeconds: RESET_VALUE,
+        maxBackoffSeconds: RESET_VALUE,
+      },
+    });
+  });
+
+  it("should init a v2 scheduleTrigger with preserveExternalChanges", () => {
+    const opts: DeploymentOptions = { preserveExternalChanges: true };
+
+    const st = initV2ScheduleTrigger("every 30 minutes", opts);
+
+    expect(st).to.deep.eq({
+      schedule: "every 30 minutes",
+      retryConfig: {},
+    });
+  });
+});

spec/v1/cloud-functions.spec.ts

@@ -189,6 +189,35 @@ describe("makeCloudFunction", () => {
     });
   });
 
+  it("should setup a scheduleTrigger in __endpoint given a schedule and preserveExternalChanges", () => {
+    const schedule = {
+      schedule: "every 5 minutes",
+      retryConfig: { retryCount: 3 },
+      timeZone: "America/New_York",
+    };
+    const cf = makeCloudFunction({
+      provider: "mock.provider",
+      eventType: "mock.event",
+      service: "service",
+      triggerResource: () => "resource",
+      handler: () => null,
+      options: {
+        schedule,
+        preserveExternalChanges: true,
+      },
+    });
+    expect(cf.__endpoint).to.deep.equal({
+      platform: "gcfv1",
+      scheduleTrigger: {
+        ...schedule,
+        retryConfig: {
+          ...schedule.retryConfig,
+        },
+      },
+      labels: {},
+    });
+  });
+
   it("should construct the right context for event", () => {
     const args: any = {
       ...cloudFunctionArgs,

spec/v1/providers/pubsub.spec.ts

@@ -388,6 +388,32 @@ describe("Pubsub Functions", () => {
         expect(result.__endpoint.availableMemoryMb).to.deep.equal(256);
         expect(result.__endpoint.timeoutSeconds).to.deep.equal(90);
       });
+
+      it("should return an appropriate endpoint when called with preserveExternalChanges", () => {
+        const result = functions
+          .region("us-east1")
+          .runWith({
+            timeoutSeconds: 90,
+            memory: "256MB",
+            preserveExternalChanges: true,
+          })
+          .pubsub.schedule("every 5 minutes")
+          .timeZone("America/New_York")
+          .onRun(() => null);
+
+        expect(result.__endpoint).to.deep.eq({
+          platform: "gcfv1",
+          labels: {},
+          region: ["us-east1"],
+          availableMemoryMb: 256,
+          timeoutSeconds: 90,
+          scheduleTrigger: {
+            schedule: "every 5 minutes",
+            timeZone: "America/New_York",
+            retryConfig: {},
+          },
+        });
+      });
     });
   });
 

spec/v1/providers/tasks.spec.ts

@@ -28,6 +28,7 @@ import { MockRequest } from "../../fixtures/mockrequest";
 import { runHandler } from "../../helper";
 import { MINIMAL_V1_ENDPOINT } from "../../fixtures";
 import { MINIMIAL_TASK_QUEUE_TRIGGER } from "./fixtures";
+import { runWith } from "../../../src/v1";
 
 describe("#onDispatch", () => {
   it("should return a trigger/endpoint with appropriate values", () => {
@@ -67,6 +68,7 @@ describe("#onDispatch", () => {
       ...MINIMAL_V1_ENDPOINT,
       platform: "gcfv1",
       taskQueueTrigger: {
+        ...MINIMIAL_TASK_QUEUE_TRIGGER,
         rateLimits: {
           maxConcurrentDispatches: 30,
           maxDispatchesPerSecond: 40,
@@ -83,6 +85,35 @@ describe("#onDispatch", () => {
     });
   });
 
+  it("should return an endpoint with appropriate values with preserveExternalChanges set", () => {
+    const result = runWith({ preserveExternalChanges: true })
+      .tasks.taskQueue({
+        rateLimits: {
+          maxConcurrentDispatches: 30,
+        },
+        retryConfig: {
+          maxAttempts: 5,
+          maxRetrySeconds: 10,
+        },
+        invoker: "private",
+      })
+      .onDispatch(() => undefined);
+
+    expect(result.__endpoint).to.deep.equal({
+      platform: "gcfv1",
+      taskQueueTrigger: {
+        rateLimits: {
+          maxConcurrentDispatches: 30,
+        },
+        retryConfig: {
+          maxAttempts: 5,
+          maxRetrySeconds: 10,
+        },
+        invoker: ["private"],
+      },
+    });
+  });
+
   it("should allow both region and runtime options to be set", () => {
     const fn = functions
       .region("us-east1")
@@ -114,6 +145,10 @@ describe("#onDispatch", () => {
         ...MINIMIAL_TASK_QUEUE_TRIGGER,
         retryConfig: {
           maxAttempts: 5,
+          maxBackoffSeconds: functions.RESET_VALUE,
+          maxDoublings: functions.RESET_VALUE,
+          maxRetrySeconds: functions.RESET_VALUE,
+          minBackoffSeconds: functions.RESET_VALUE,
         },
       },
     });

spec/v2/providers/scheduler.spec.ts

@@ -63,11 +63,13 @@ describe("schedule", () => {
       expect(schedule.getOpts(options)).to.deep.eq({
         schedule: "* * * * *",
         timeZone: "utc",
-        retryCount: 3,
-        maxRetrySeconds: 1,
-        minBackoffSeconds: 2,
-        maxBackoffSeconds: 3,
-        maxDoublings: 4,
+        retryConfig: {
+          retryCount: 3,
+          maxRetrySeconds: 1,
+          minBackoffSeconds: 2,
+          maxBackoffSeconds: 3,
+          maxDoublings: 4,
+        },
         opts: {
           ...options,
           memory: "128MiB",
@@ -139,6 +141,38 @@ describe("schedule", () => {
       ]);
     });
 
+    it("should create a schedule function with preserveExternalChanges", () => {
+      const schfn = schedule.onSchedule(
+        {
+          schedule: "* * * * *",
+          preserveExternalChanges: true,
+        },
+        () => console.log(1)
+      );
+
+      expect(schfn.__endpoint).to.deep.eq({
+        platform: "gcfv2",
+        labels: {},
+        scheduleTrigger: {
+          schedule: "* * * * *",
+          timeZone: undefined,
+          retryConfig: {
+            retryCount: undefined,
+            maxRetrySeconds: undefined,
+            minBackoffSeconds: undefined,
+            maxBackoffSeconds: undefined,
+            maxDoublings: undefined,
+          },
+        },
+      });
+      expect(schfn.__requiredAPIs).to.deep.eq([
+        {
+          api: "cloudscheduler.googleapis.com",
+          reason: "Needed for scheduled functions.",
+        },
+      ]);
+    });
+
     it("should have a .run method", async () => {
       const testObj = {
         foo: "bar",

spec/v2/providers/tasks.spec.ts

@@ -128,6 +128,73 @@ describe("onTaskDispatched", () => {
     });
   });
 
+  it("should return a minimal endpoint without preserveExternalChanges set", () => {
+    const result = onTaskDispatched(
+      {
+        retryConfig: {
+          maxAttempts: 4,
+          maxRetrySeconds: 10,
+        },
+        rateLimits: {
+          maxDispatchesPerSecond: 10,
+        },
+      },
+      () => undefined
+    );
+
+    expect(result.__endpoint).to.deep.equal({
+      ...MINIMAL_V2_ENDPOINT,
+      platform: "gcfv2",
+      labels: {},
+      taskQueueTrigger: {
+        retryConfig: {
+          maxAttempts: 4,
+          maxRetrySeconds: 10,
+          maxBackoffSeconds: options.RESET_VALUE,
+          maxDoublings: options.RESET_VALUE,
+          minBackoffSeconds: options.RESET_VALUE,
+        },
+        rateLimits: {
+          maxDispatchesPerSecond: 10,
+          maxConcurrentDispatches: options.RESET_VALUE,
+        },
+      },
+    });
+  });
+
+  it("should create a complex endpoint with preserveExternalChanges set", () => {
+    const result = onTaskDispatched(
+      {
+        ...FULL_OPTIONS,
+        retryConfig: {
+          maxAttempts: 4,
+          maxRetrySeconds: 10,
+        },
+        rateLimits: {
+          maxDispatchesPerSecond: 10,
+        },
+        invoker: "private",
+        preserveExternalChanges: true,
+      },
+      () => undefined
+    );
+
+    expect(result.__endpoint).to.deep.equal({
+      ...FULL_ENDPOINT,
+      platform: "gcfv2",
+      taskQueueTrigger: {
+        retryConfig: {
+          maxAttempts: 4,
+          maxRetrySeconds: 10,
+        },
+        rateLimits: {
+          maxDispatchesPerSecond: 10,
+        },
+        invoker: ["private"],
+      },
+    });
+  });
+
   it("should merge options and globalOptions", () => {
     options.setGlobalOptions({
       concurrency: 20,

spec/v2/providers/testLab.spec.ts

@@ -23,6 +23,7 @@
 import { expect } from "chai";
 import * as testLab from "../../../src/v2/providers/testLab";
 import * as options from "../../../src/v2/options";
+import { MINIMAL_V2_ENDPOINT } from "../../fixtures";
 
 describe("onTestMatrixCompleted", () => {
   afterEach(() => {
@@ -33,6 +34,7 @@ describe("onTestMatrixCompleted", () => {
     const fn = testLab.onTestMatrixCompleted(() => 2);
 
     expect(fn.__endpoint).to.deep.eq({
+      ...MINIMAL_V2_ENDPOINT,
       platform: "gcfv2",
       labels: {},
       eventTrigger: {
@@ -59,6 +61,7 @@ describe("onTestMatrixCompleted", () => {
     );
 
     expect(fn.__endpoint).to.deep.eq({
+      ...MINIMAL_V2_ENDPOINT,
       platform: "gcfv2",
       availableMemoryMb: 512,
       region: ["us-central1"],