From b9a1710d44f1af09543812025ff34faddca08096 Mon Sep 17 00:00:00 2001 From: Cole Rogers Date: Tue, 8 Nov 2022 17:02:02 -0500 Subject: [PATCH 1/5] fixing retry config for scheduled functions --- CHANGELOG.md | 1 + spec/runtime/manifest.spec.ts | 92 ++++++++++++++++++++++++++++- spec/v1/cloud-functions.spec.ts | 29 +++++++++ spec/v1/providers/pubsub.spec.ts | 26 ++++++++ spec/v2/providers/scheduler.spec.ts | 37 ++++++++++-- spec/v2/providers/testLab.spec.ts | 3 + src/v1/cloud-functions.ts | 6 ++ src/v1/function-builder.ts | 4 +- src/v2/providers/scheduler.ts | 55 +++++++++++++---- src/v2/providers/testLab.ts | 3 +- 10 files changed, 234 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb..7908e0203 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- Fixes bug supplying where preserveExternalChanges to scheduled functions would cause deployment failure. diff --git a/spec/runtime/manifest.spec.ts b/spec/runtime/manifest.spec.ts index 22c5f8af1..1932d0b3c 100644 --- a/spec/runtime/manifest.spec.ts +++ b/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,85 @@ 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({}); + }); +}); + +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", + }); + }); + + 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", + }); + }); +}); diff --git a/spec/v1/cloud-functions.spec.ts b/spec/v1/cloud-functions.spec.ts index 3a9c99b67..6ee3abc41 100644 --- a/spec/v1/cloud-functions.spec.ts +++ b/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, diff --git a/spec/v1/providers/pubsub.spec.ts b/spec/v1/providers/pubsub.spec.ts index 77b6fe24a..0a7b89ad6 100644 --- a/spec/v1/providers/pubsub.spec.ts +++ b/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: {}, + }, + }); + }); }); }); diff --git a/spec/v2/providers/scheduler.spec.ts b/spec/v2/providers/scheduler.spec.ts index 5ca9fb407..0b39b885c 100644 --- a/spec/v2/providers/scheduler.spec.ts +++ b/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,31 @@ 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: "* * * * *", + retryConfig: {}, + }, + }); + 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", diff --git a/spec/v2/providers/testLab.spec.ts b/spec/v2/providers/testLab.spec.ts index 7d59f8bca..15ff77248 100644 --- a/spec/v2/providers/testLab.spec.ts +++ b/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"], diff --git a/src/v1/cloud-functions.ts b/src/v1/cloud-functions.ts index d2551e516..9dd892db5 100644 --- a/src/v1/cloud-functions.ts +++ b/src/v1/cloud-functions.ts @@ -452,6 +452,12 @@ export function makeCloudFunction({ if (options.schedule) { endpoint.scheduleTrigger = initV1ScheduleTrigger(options.schedule.schedule, options); + // Note: setting preserveExternalChanges to true + // will not correctly init a retryConfig object, + // we must do that explicitly. + if (!endpoint.scheduleTrigger.retryConfig) { + endpoint.scheduleTrigger.retryConfig = {}; + } copyIfPresent(endpoint.scheduleTrigger, options.schedule, "timeZone"); copyIfPresent( endpoint.scheduleTrigger.retryConfig, diff --git a/src/v1/function-builder.ts b/src/v1/function-builder.ts index c7e5daa3e..6bf0267a7 100644 --- a/src/v1/function-builder.ts +++ b/src/v1/function-builder.ts @@ -274,7 +274,7 @@ export function region( * * Value must not be null. */ -export function runWith(runtimeOptions: RuntimeOptions): FunctionBuilder { +export function runWith(runtimeOptions: DeploymentOptions): FunctionBuilder { if (assertRuntimeOptionsValid(runtimeOptions)) { return new FunctionBuilder(runtimeOptions); } @@ -313,7 +313,7 @@ export class FunctionBuilder { * * Value must not be null. */ - runWith(runtimeOptions: RuntimeOptions): FunctionBuilder { + runWith(runtimeOptions: DeploymentOptions): FunctionBuilder { if (assertRuntimeOptionsValid(runtimeOptions)) { this.options = { ...this.options, diff --git a/src/v2/providers/scheduler.ts b/src/v2/providers/scheduler.ts index ec17e10e5..a61709c4d 100644 --- a/src/v2/providers/scheduler.ts +++ b/src/v2/providers/scheduler.ts @@ -41,11 +41,13 @@ import * as options from "../options"; interface ScheduleArgs { schedule: string | Expression; timeZone?: timezone | Expression | ResetValue; - retryCount?: number | Expression | ResetValue; - maxRetrySeconds?: number | Expression | ResetValue; - minBackoffSeconds?: number | Expression | ResetValue; - maxBackoffSeconds?: number | Expression | ResetValue; - maxDoublings?: number | Expression | ResetValue; + retryConfig?: { + retryCount?: number | Expression | ResetValue; + maxRetrySeconds?: number | Expression | ResetValue; + minBackoffSeconds?: number | Expression | ResetValue; + maxBackoffSeconds?: number | Expression | ResetValue; + maxDoublings?: number | Expression | ResetValue; + }; opts: options.GlobalOptions; } @@ -57,16 +59,30 @@ export function getOpts(args: string | ScheduleOptions): ScheduleArgs { opts: {} as options.GlobalOptions, }; } - return { + + const separatedOpts: ScheduleArgs = { schedule: args.schedule, - timeZone: args.timeZone, - retryCount: args.retryCount, - maxRetrySeconds: args.maxRetrySeconds, - minBackoffSeconds: args.minBackoffSeconds, - maxBackoffSeconds: args.maxBackoffSeconds, - maxDoublings: args.maxDoublings, opts: args as options.GlobalOptions, }; + if (args.timeZone) { + separatedOpts.timeZone = args.timeZone; + } + if ( + args.retryCount || + args.maxRetrySeconds || + args.minBackoffSeconds || + args.maxBackoffSeconds || + args.maxDoublings + ) { + separatedOpts.retryConfig = { + retryCount: args.retryCount, + maxRetrySeconds: args.maxRetrySeconds, + minBackoffSeconds: args.minBackoffSeconds, + maxBackoffSeconds: args.maxBackoffSeconds, + maxDoublings: args.maxDoublings, + }; + } + return separatedOpts; } /** @@ -191,10 +207,23 @@ export function onSchedule( scheduleTrigger: initV2ScheduleTrigger(separatedOpts.schedule, globalOpts, separatedOpts.opts), }; + // Note: setting preserveExternalChanges to true + // will not correctly init a retryConfig object, + // we must do that explicitly. + if (!ep.scheduleTrigger.retryConfig) { + ep.scheduleTrigger.retryConfig = {}; + } + copyIfPresent(ep.scheduleTrigger, separatedOpts, "timeZone"); + // copyIfPresent( + // ep.scheduleTrigger, + // separatedOpts, + // "retryConfig" + // ); + copyIfPresent( ep.scheduleTrigger.retryConfig, - separatedOpts, + separatedOpts.retryConfig, "retryCount", "maxRetrySeconds", "minBackoffSeconds", diff --git a/src/v2/providers/testLab.ts b/src/v2/providers/testLab.ts index 16d95ea6a..d24e6e003 100644 --- a/src/v2/providers/testLab.ts +++ b/src/v2/providers/testLab.ts @@ -20,7 +20,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. -import { ManifestEndpoint } from "../../runtime/manifest"; +import { initV2Endpoint, ManifestEndpoint } from "../../runtime/manifest"; import { CloudEvent, CloudFunction } from "../core"; import { EventHandlerOptions, getGlobalOptions, optionsToEndpoint } from "../options"; import { wrapTraceContext } from "../trace"; @@ -195,6 +195,7 @@ export function onTestMatrixCompleted( func.run = handler; const ep: ManifestEndpoint = { + ...initV2Endpoint(getGlobalOptions(), optsOrHandler), platform: "gcfv2", ...baseOpts, ...specificOpts, From 9e22a7c1b01c2753213023b9c39d4cf65431ba3d Mon Sep 17 00:00:00 2001 From: Cole Rogers Date: Wed, 9 Nov 2022 14:36:41 -0500 Subject: [PATCH 2/5] change task queue behavior and clean up things --- CHANGELOG.md | 2 +- spec/runtime/manifest.spec.ts | 7 ++- spec/v1/providers/tasks.spec.ts | 35 +++++++++++++++ spec/v2/providers/scheduler.spec.ts | 1 + spec/v2/providers/tasks.spec.ts | 67 +++++++++++++++++++++++++++++ src/runtime/manifest.ts | 22 +++++----- src/v1/cloud-functions.ts | 6 --- src/v1/providers/tasks.ts | 17 +++++++- src/v2/providers/scheduler.ts | 23 ++-------- src/v2/providers/tasks.ts | 17 +++++++- 10 files changed, 156 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7908e0203..c449ec439 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1 @@ -- Fixes bug supplying where preserveExternalChanges to scheduled functions would cause deployment failure. +- Fix a bug where supplying preserveExternalChanges to scheduled functions would cause deployment failure (#1305). diff --git a/spec/runtime/manifest.spec.ts b/spec/runtime/manifest.spec.ts index 1932d0b3c..eb553643b 100644 --- a/spec/runtime/manifest.spec.ts +++ b/spec/runtime/manifest.spec.ts @@ -201,7 +201,10 @@ describe("initTaskQueueTrigger", () => { const tt = initTaskQueueTrigger(opts); - expect(tt).to.deep.eq({}); + expect(tt).to.deep.eq({ + rateLimits: {}, + retryConfig: {}, + }); }); }); @@ -229,6 +232,7 @@ describe("initScheduleTrigger", () => { expect(st).to.deep.eq({ schedule: "every 30 minutes", + retryConfig: {}, }); }); @@ -255,6 +259,7 @@ describe("initScheduleTrigger", () => { expect(st).to.deep.eq({ schedule: "every 30 minutes", + retryConfig: {}, }); }); }); diff --git a/spec/v1/providers/tasks.spec.ts b/spec/v1/providers/tasks.spec.ts index dc0b5e0c6..040e30388 100644 --- a/spec/v1/providers/tasks.spec.ts +++ b/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, }, }, }); diff --git a/spec/v2/providers/scheduler.spec.ts b/spec/v2/providers/scheduler.spec.ts index 0b39b885c..b5644927b 100644 --- a/spec/v2/providers/scheduler.spec.ts +++ b/spec/v2/providers/scheduler.spec.ts @@ -155,6 +155,7 @@ describe("schedule", () => { labels: {}, scheduleTrigger: { schedule: "* * * * *", + timeZone: undefined, retryConfig: {}, }, }); diff --git a/spec/v2/providers/tasks.spec.ts b/spec/v2/providers/tasks.spec.ts index 4962b342e..473768371 100644 --- a/spec/v2/providers/tasks.spec.ts +++ b/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, diff --git a/src/runtime/manifest.ts b/src/runtime/manifest.ts index 9780b1af4..1ca34fa67 100644 --- a/src/runtime/manifest.ts +++ b/src/runtime/manifest.ts @@ -214,17 +214,17 @@ const RESETTABLE_RATE_LIMITS_OPTIONS: ResettableKeys< export function initTaskQueueTrigger( ...opts: ManifestOptions[] ): ManifestEndpoint["taskQueueTrigger"] { - let taskQueueTrigger = {}; + const taskQueueTrigger: ManifestEndpoint["taskQueueTrigger"] = { + retryConfig: {}, + rateLimits: {}, + }; if (opts.every((opt) => !opt?.preserveExternalChanges)) { - const retryConfig = {}; for (const key of Object.keys(RESETTABLE_RETRY_CONFIG_OPTIONS)) { - retryConfig[key] = RESET_VALUE; + taskQueueTrigger.retryConfig[key] = RESET_VALUE; } - const rateLimits = {}; for (const key of Object.keys(RESETTABLE_RATE_LIMITS_OPTIONS)) { - rateLimits[key] = RESET_VALUE; + taskQueueTrigger.rateLimits[key] = RESET_VALUE; } - taskQueueTrigger = { retryConfig, rateLimits }; } return taskQueueTrigger; } @@ -256,13 +256,15 @@ function initScheduleTrigger( schedule: string | Expression, ...opts: ManifestOptions[] ): ManifestEndpoint["scheduleTrigger"] { - let scheduleTrigger: ManifestEndpoint["scheduleTrigger"] = { schedule }; + let scheduleTrigger: ManifestEndpoint["scheduleTrigger"] = { + schedule, + retryConfig: {}, + }; if (opts.every((opt) => !opt?.preserveExternalChanges)) { - const retryConfig = {}; for (const key of Object.keys(resetOptions)) { - retryConfig[key] = RESET_VALUE; + scheduleTrigger.retryConfig[key] = RESET_VALUE; } - scheduleTrigger = { ...scheduleTrigger, timeZone: RESET_VALUE, retryConfig }; + scheduleTrigger = { ...scheduleTrigger, timeZone: RESET_VALUE }; } return scheduleTrigger; } diff --git a/src/v1/cloud-functions.ts b/src/v1/cloud-functions.ts index 9dd892db5..d2551e516 100644 --- a/src/v1/cloud-functions.ts +++ b/src/v1/cloud-functions.ts @@ -452,12 +452,6 @@ export function makeCloudFunction({ if (options.schedule) { endpoint.scheduleTrigger = initV1ScheduleTrigger(options.schedule.schedule, options); - // Note: setting preserveExternalChanges to true - // will not correctly init a retryConfig object, - // we must do that explicitly. - if (!endpoint.scheduleTrigger.retryConfig) { - endpoint.scheduleTrigger.retryConfig = {}; - } copyIfPresent(endpoint.scheduleTrigger, options.schedule, "timeZone"); copyIfPresent( endpoint.scheduleTrigger.retryConfig, diff --git a/src/v1/providers/tasks.ts b/src/v1/providers/tasks.ts index 9126588a5..c9bf70849 100644 --- a/src/v1/providers/tasks.ts +++ b/src/v1/providers/tasks.ts @@ -129,8 +129,21 @@ export class TaskQueueBuilder { ...optionsToEndpoint(this.depOpts), taskQueueTrigger: initTaskQueueTrigger(this.depOpts), }; - copyIfPresent(func.__endpoint.taskQueueTrigger, this.tqOpts, "retryConfig"); - copyIfPresent(func.__endpoint.taskQueueTrigger, this.tqOpts, "rateLimits"); + copyIfPresent( + func.__endpoint.taskQueueTrigger.retryConfig, + this.tqOpts?.retryConfig || {}, + "maxAttempts", + "maxBackoffSeconds", + "maxDoublings", + "maxRetrySeconds", + "minBackoffSeconds" + ); + copyIfPresent( + func.__endpoint.taskQueueTrigger.rateLimits, + this.tqOpts?.rateLimits || {}, + "maxConcurrentDispatches", + "maxDispatchesPerSecond" + ); convertIfPresent( func.__endpoint.taskQueueTrigger, this.tqOpts, diff --git a/src/v2/providers/scheduler.ts b/src/v2/providers/scheduler.ts index a61709c4d..e890a75f1 100644 --- a/src/v2/providers/scheduler.ts +++ b/src/v2/providers/scheduler.ts @@ -38,7 +38,7 @@ import * as logger from "../../logger"; import * as options from "../options"; /** @hidden */ -interface ScheduleArgs { +interface SeparatedOpts { schedule: string | Expression; timeZone?: timezone | Expression | ResetValue; retryConfig?: { @@ -52,7 +52,7 @@ interface ScheduleArgs { } /** @internal */ -export function getOpts(args: string | ScheduleOptions): ScheduleArgs { +export function getOpts(args: string | ScheduleOptions): SeparatedOpts { if (typeof args === "string") { return { schedule: args, @@ -60,13 +60,11 @@ export function getOpts(args: string | ScheduleOptions): ScheduleArgs { }; } - const separatedOpts: ScheduleArgs = { + const separatedOpts: SeparatedOpts = { schedule: args.schedule, + timeZone: args.timeZone, opts: args as options.GlobalOptions, }; - if (args.timeZone) { - separatedOpts.timeZone = args.timeZone; - } if ( args.retryCount || args.maxRetrySeconds || @@ -207,20 +205,7 @@ export function onSchedule( scheduleTrigger: initV2ScheduleTrigger(separatedOpts.schedule, globalOpts, separatedOpts.opts), }; - // Note: setting preserveExternalChanges to true - // will not correctly init a retryConfig object, - // we must do that explicitly. - if (!ep.scheduleTrigger.retryConfig) { - ep.scheduleTrigger.retryConfig = {}; - } - copyIfPresent(ep.scheduleTrigger, separatedOpts, "timeZone"); - // copyIfPresent( - // ep.scheduleTrigger, - // separatedOpts, - // "retryConfig" - // ); - copyIfPresent( ep.scheduleTrigger.retryConfig, separatedOpts.retryConfig, diff --git a/src/v2/providers/tasks.ts b/src/v2/providers/tasks.ts index d5974b722..c131346c6 100644 --- a/src/v2/providers/tasks.ts +++ b/src/v2/providers/tasks.ts @@ -246,8 +246,21 @@ export function onTaskDispatched( taskQueueTrigger: initTaskQueueTrigger(options.getGlobalOptions(), opts), }; - copyIfPresent(func.__endpoint.taskQueueTrigger, opts, "retryConfig"); - copyIfPresent(func.__endpoint.taskQueueTrigger, opts, "rateLimits"); + copyIfPresent( + func.__endpoint.taskQueueTrigger.retryConfig, + opts.retryConfig || {}, + "maxAttempts", + "maxBackoffSeconds", + "maxDoublings", + "maxRetrySeconds", + "minBackoffSeconds" + ); + copyIfPresent( + func.__endpoint.taskQueueTrigger.rateLimits, + opts.rateLimits || {}, + "maxConcurrentDispatches", + "maxDispatchesPerSecond" + ); convertIfPresent(func.__endpoint.taskQueueTrigger, opts, "invoker", "invoker", convertInvoker); func.__requiredAPIs = [ From 1c23b45964af9106a0579e5be883e10c4ff05fb6 Mon Sep 17 00:00:00 2001 From: Cole Rogers Date: Wed, 9 Nov 2022 15:35:36 -0500 Subject: [PATCH 3/5] small cleanup --- spec/v2/providers/scheduler.spec.ts | 8 +++++++- src/v2/providers/scheduler.ts | 22 ++++++---------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/spec/v2/providers/scheduler.spec.ts b/spec/v2/providers/scheduler.spec.ts index b5644927b..4f3e6f984 100644 --- a/spec/v2/providers/scheduler.spec.ts +++ b/spec/v2/providers/scheduler.spec.ts @@ -156,7 +156,13 @@ describe("schedule", () => { scheduleTrigger: { schedule: "* * * * *", timeZone: undefined, - retryConfig: {}, + retryConfig: { + retryCount: undefined, + maxRetrySeconds: undefined, + minBackoffSeconds: undefined, + maxBackoffSeconds: undefined, + maxDoublings: undefined, + }, }, }); expect(schfn.__requiredAPIs).to.deep.eq([ diff --git a/src/v2/providers/scheduler.ts b/src/v2/providers/scheduler.ts index e890a75f1..dfd4400e9 100644 --- a/src/v2/providers/scheduler.ts +++ b/src/v2/providers/scheduler.ts @@ -59,28 +59,18 @@ export function getOpts(args: string | ScheduleOptions): SeparatedOpts { opts: {} as options.GlobalOptions, }; } - - const separatedOpts: SeparatedOpts = { + return { schedule: args.schedule, timeZone: args.timeZone, - opts: args as options.GlobalOptions, - }; - if ( - args.retryCount || - args.maxRetrySeconds || - args.minBackoffSeconds || - args.maxBackoffSeconds || - args.maxDoublings - ) { - separatedOpts.retryConfig = { + retryConfig: { retryCount: args.retryCount, maxRetrySeconds: args.maxRetrySeconds, minBackoffSeconds: args.minBackoffSeconds, maxBackoffSeconds: args.maxBackoffSeconds, maxDoublings: args.maxDoublings, - }; - } - return separatedOpts; + }, + opts: args as options.GlobalOptions, + }; } /** @@ -208,7 +198,7 @@ export function onSchedule( copyIfPresent(ep.scheduleTrigger, separatedOpts, "timeZone"); copyIfPresent( ep.scheduleTrigger.retryConfig, - separatedOpts.retryConfig, + separatedOpts.retryConfig || {}, "retryCount", "maxRetrySeconds", "minBackoffSeconds", From 31d39f38aca2d04bfbea3c6d36368805b3930055 Mon Sep 17 00:00:00 2001 From: Cole Rogers Date: Fri, 11 Nov 2022 08:30:45 -0500 Subject: [PATCH 4/5] removing unnecessary '|| {}' from copyIfPresent --- src/v2/providers/scheduler.ts | 2 +- src/v2/providers/tasks.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/v2/providers/scheduler.ts b/src/v2/providers/scheduler.ts index dfd4400e9..9346c451c 100644 --- a/src/v2/providers/scheduler.ts +++ b/src/v2/providers/scheduler.ts @@ -198,7 +198,7 @@ export function onSchedule( copyIfPresent(ep.scheduleTrigger, separatedOpts, "timeZone"); copyIfPresent( ep.scheduleTrigger.retryConfig, - separatedOpts.retryConfig || {}, + separatedOpts.retryConfig, "retryCount", "maxRetrySeconds", "minBackoffSeconds", diff --git a/src/v2/providers/tasks.ts b/src/v2/providers/tasks.ts index f624c4d5c..adb0b81a7 100644 --- a/src/v2/providers/tasks.ts +++ b/src/v2/providers/tasks.ts @@ -253,7 +253,7 @@ export function onTaskDispatched( copyIfPresent( func.__endpoint.taskQueueTrigger.retryConfig, - opts.retryConfig || {}, + opts.retryConfig, "maxAttempts", "maxBackoffSeconds", "maxDoublings", @@ -262,7 +262,7 @@ export function onTaskDispatched( ); copyIfPresent( func.__endpoint.taskQueueTrigger.rateLimits, - opts.rateLimits || {}, + opts.rateLimits, "maxConcurrentDispatches", "maxDispatchesPerSecond" ); From 2081221b78c18115174b1d845d4567363edda00f Mon Sep 17 00:00:00 2001 From: Cole Rogers Date: Fri, 11 Nov 2022 12:11:01 -0500 Subject: [PATCH 5/5] move preserveExternalChanges to runtime options --- src/v1/function-builder.ts | 4 ++-- src/v1/function-configuration.ts | 21 +++++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/v1/function-builder.ts b/src/v1/function-builder.ts index 6bf0267a7..c7e5daa3e 100644 --- a/src/v1/function-builder.ts +++ b/src/v1/function-builder.ts @@ -274,7 +274,7 @@ export function region( * * Value must not be null. */ -export function runWith(runtimeOptions: DeploymentOptions): FunctionBuilder { +export function runWith(runtimeOptions: RuntimeOptions): FunctionBuilder { if (assertRuntimeOptionsValid(runtimeOptions)) { return new FunctionBuilder(runtimeOptions); } @@ -313,7 +313,7 @@ export class FunctionBuilder { * * Value must not be null. */ - runWith(runtimeOptions: DeploymentOptions): FunctionBuilder { + runWith(runtimeOptions: RuntimeOptions): FunctionBuilder { if (assertRuntimeOptionsValid(runtimeOptions)) { this.options = { ...this.options, diff --git a/src/v1/function-configuration.ts b/src/v1/function-configuration.ts index 504e392ca..e92971a48 100644 --- a/src/v1/function-configuration.ts +++ b/src/v1/function-configuration.ts @@ -246,6 +246,17 @@ export interface RuntimeOptions { * When false, requests with invalid tokens set context.app to undefiend. */ enforceAppCheck?: boolean; + + /** + * Controls whether function configuration modified outside of function source is preserved. Defaults to false. + * + * @remarks + * When setting configuration available in the underlying platform that is not yet available in the Firebase Functions + * SDK, we highly recommend setting `preserveExternalChanges` to `true`. Otherwise, when the Firebase Functions SDK releases + * a new version of the SDK with support for the missing configuration, your function's manually configured setting + * may inadvertently be wiped out. + */ + preserveExternalChanges?: boolean; } /** @@ -264,14 +275,4 @@ export interface DeploymentOptions extends RuntimeOptions { * Schedule for the scheduled function. */ schedule?: Schedule; - /** - * Controls whether function configuration modified outside of function source is preserved. Defaults to false. - * - * @remarks - * When setting configuration available in the underlying platform that is not yet available in the Firebase Functions - * SDK, we highly recommend setting `preserveExternalChanges` to `true`. Otherwise, when the Firebase Functions SDK releases - * a new version of the SDK with support for the missing configuration, your function's manually configured setting - * may inadvertently be wiped out. - */ - preserveExternalChanges?: boolean; }