Functions deploy uses new "backend" types #3303

inlined · 2021-04-23T00:24:35Z

Rewrote the Firebase deploy code to use the newly introduced "backend" types. Some smaller things picked up along the way:

Customers can set FIREBASE_FUNCTIONS_DEFAULT_REGION to change the default from "us-central1"
Trigger parsing is moved to deploy/functions/discovery/jsExports. I need to eventually move much of prepare (e.g. Runtime discovery, package.json validation, etc) into discovery or some other directory. The end goal is that we've clearly separated JS code in a place where other languages can be dropped in and to put the JS export analyzing code somwhere it can be SxS with the upcoming HTTP contract.
Some files like functionsDeployHelper.ts have been moved into /deploy/functions to help clean up the swamp that is /src/*.{ts|js}

joehan · 2021-04-27T19:36:59Z

src/deploy/functions/deploymentPlanner.ts

-    if (fn.labels?.["deployment-scheduled"] === "true") {
-      deployment.schedulesToDelete.push(fn.name);
-    }
+  // NOTE(inlined): It's easiest to partition w/ deletes per region and might help


When originally implementing this, I started with functionsToDelete as a regional concept, and then turned it into a global one, for a few reasons:

We don't need to wait for sourceToken to do deletes, so they're not tied to the rest of the function operations in that region

As you mention, we display the full list of functionsToDelete when asking whether we should delete them.

I think the best reason to split them into regions is potential future features - it would make it very easy to add regional filters, for example.

Are you saying I should move deletes to global or that I should remove the comment (or incorporate your feedback into the comment)

Sorry, should have been clearer. I'm ok with either implementation. After reviewing the rest of the code the difference is pretty minor - looks like its just a single map().reduce() that you'd be able to get rid of in release.ts if you made functionsToDelete global.

Will try to remember to revisit this once the dust settles

google-cla · 2021-04-27T21:16:57Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

inlined · 2021-04-27T21:25:38Z

@googlebot I consent

google-cla · 2021-04-27T21:25:50Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

The Backend namespace defines types that are neither the __trigger annotations nor the GCP API types. This follows Google best practices for implementing services because it buffers the system from changes in storage or API layers. In this codebase, it will allow us to support new trigger discovery techniques (e.g. a container contract HTTP API) and new versions of APIs if they should ever be needed. In this change I try out a new method for using args.Context; it's based on the Go style of Context where a namespace can provide a value using Context as a private implementation detail. I'm personally a fan. WDYT?

joehan · 2021-04-28T22:44:19Z

src/deploy/functions/discovery/index.ts

+  options: args.Options,
+  runtimeConfigValues: backend.RuntimeConfigValues
+): Promise<backend.Backend> {
+  let strategy: Strategy | undefined = undefined;


Why did you use this pattern instead of the cleaner let strategy: Strategy? Is it just to specifically call out that strategy can be undefined if we can't find one to use?

Effectively, yes. TypeScript is smart enough to know that strategy is uninitialized and may not be initialized below. It becomes a compiler error to check.

joehan · 2021-04-28T22:44:57Z

src/deploy/functions/discovery/jsexports/parseTriggers.ts

@@ -0,0 +1,236 @@
+"use strict";


Not needed since this is ts

joehan · 2021-04-28T22:59:41Z

src/deploy/functions/discovery/jsexports/parseTriggers.ts

+        reject(
+          new FirebaseError(
+            "There was an unknown problem while trying to parse function triggers. " +
+              "Please ensure you are using Node.js v6 or greater.",


Seems out of date now?

Moving this discussion offline. It probably is out of date, but I'm wondering if it's worth removing something that probably protects us from cryptic errors.

joehan · 2021-04-28T23:04:45Z

src/deploy/functions/discovery/jsexports/parseTriggers.ts

+          new FirebaseError(
+            "There was an unknown problem while trying to parse function triggers. " +
+              "Please ensure you are using Node.js v6 or greater.",
+            { exit: 2 }


Any idea why this is exit code 2? Google says that means "Misuse of shell builtins (according to Bash documentation)", which doesn't really apply.

If we don't have a clear reasoning here, we can probably stop explicitly setting exit code.

We seem to use exit code 2 a fair amount. Moving that conversation offline too.

As an aside, I apologize that you, for some reason, had to review this whole file even though only the end of the file is new. I guess it tipped the scales enough that Git no longer saw the file as moved + appended.

joehan · 2021-04-29T16:28:11Z

src/deploy/functions/functionsDeployHelper.ts

+
+export function functionMatchesGroup(func: backend.TargetIds, groupChunks: string[]): boolean {
+  const functionNameChunks = func.id.split("-").slice(0, groupChunks.length);
+  return _.isEqual(groupChunks, functionNameChunks);


Optional: Seems only slightly annoying to change this from lodash to plain TS, and this is the last usage of lodash in this file. Removing this would let us get lodash out of another file.

joehan · 2021-04-29T18:51:04Z

src/test/deploy/functions/discovery/jsexports/parseTriggers.spec.ts

+    for (const failurePolicy of [undefined, false, true, { retry: {} }]) {
+      const name =
+        typeof failurePolicy === "undefined" ? "undefined" : JSON.stringify(failurePolicy);
+      it(`failurePolicy ${name}`, () => {


Suggested change

it(`failurePolicy ${name}`, () => {

it(`should handle failurePolicy=${name}`, () => {

src/test/deploy/functions/discovery/jsexports/parseTriggers.spec.ts

joehan · 2021-04-29T18:54:14Z

src/test/deploy/functions/discovery/jsexports/parseTriggers.spec.ts

+
+  const BASIC_FUNCTION_NAME: backend.TargetIds = Object.freeze({
+    id: "func",
+    region: "us-central1",


Does this test fail if FIREBASE_FUNCTIONS_DEFAULT_REGION is set to something else?

Will use a hack to set & reset api.functionsDefaultRegion. Unfortunately this won't work once we have ESM since those are readonly.

Alternative that should work once we have ESM - just set this to api.functionsDefaultRegion

🤦‍♂️ yes. That's obviously better.

joehan · 2021-04-29T18:56:32Z

src/test/deploy/functions/functionsDeployHelper.spec.ts

+  };
+
+  describe("functionMatchesGroup", () => {
+    it("should match empty filters", () => {


Nit: repo convention is to separate it blocks with newlines.

Looked at other files. The first it comes without a newline after the describe but multiple its have a newline. Applied that style to this file.

…ls into inlined.gcf-typing

joehan

A few more things, but this is pretty much LGTM after those.

joehan · 2021-05-03T22:36:44Z

src/deploy/functions/deploymentPlanner.ts

+  // Topics aren't created yet explicitly because the Functions API creates them
+  // automatically. This may change in GCFv2 and would certainly change in Run,
+  // so we should be ready to start creating topics before schedules or functions.
+  // OTOH, we could just say that schedules targeting Pub/Sub are just a v1 thing


We originally chose pubsub because private HTTPS functions didn't exist yet. As long as we can ensure that only Scheduler can invoke the underlying function, it seems preferable to simplify things in v2 by switching all scheduled functions to use HTTPS as a transport.

Oh, totally get the status quo. I think HTTPS is a better transport since it allows Cloud Scheduler's backff/retry policy to be meaningful, but I don't think we can convert v1 Pub/Sub functions to a v2 HTTP function though (unless we do some special magic...)

joehan · 2021-05-03T22:40:08Z

src/deploy/functions/deploymentPlanner.ts

+export function allRegions(
+  spec: Record<string, backend.FunctionSpec[]>,
+  existing: Record<string, backend.FunctionSpec[]>
+) {


nit: Add a return type

joehan · 2021-05-03T22:40:48Z

src/deploy/functions/deploymentPlanner.ts

+    if (haveFn) {
+      checkForInvalidChangeOfTrigger(fn, haveFn);
+
+      // Remember old environment varialbes that might have been set with


Typo in the word variables

joehan · 2021-05-03T22:48:52Z

src/deploy/functions/deploymentPlanner.ts

+  // functions across all regions. Should we go back to making functions to delete a global
+  // part of the DeploymentPlan?
+  // Alternatively we could make schedules and topics regional based on the service they feed into.
+  const allFunctionsToDelete: backend.FunctionSpec[] = [];


Unless I'm totally missing something, allFunctionsToDelete is unused after building it

You're right. Maybe I translated this before making it no longer global.

joehan · 2021-05-03T22:53:57Z

src/deploy/functions/discovery/jsexports/parseTriggers.ts

+/**
+ * Removes any inspect options (`inspect` or `inspect-brk`) from options so the forked process is able to run (otherwise
+ * it'll inherit process values and will use the same port).
+ * @param {string[]} options From either `process.execArgv` or `NODE_OPTIONS` envar (which is a space separated string)


No longer need these {string[]}s since we have real types now

joehan · 2021-05-03T23:14:05Z

src/deploy/functions/validate.ts

-  const invalidNames = _.reject(_.keys(functionNames), (name: string): boolean => {
-    return _.startsWith(name, ".") || validFunctionNameRegex.test(name);
+  const invalidIds = functions.filter((fn) => {
+    // NOTE(inlined): Why is it OK for a function to start with a "."?


Following up from the internal discussion - I just tested this out of curiosity, and it errors out on the deploy with : {"error":{"code":400,"message":"The request has errors","status":"INVALID_ARGUMENT","details":[{"@type":"type.googleapis.com/google.rpc.BadRequest","fieldViolations":[{"field":"function","description":"short function name .helloWorld does not match the expected pattern"}]}]}}

We should get rid of this.

joehan · 2021-05-03T23:21:05Z

src/deploy/functions/validate.ts

      )}] Changing from an HTTPS function to an background triggered function is not allowed. Please delete your function and create a new one instead.`
    );
  }
-  if (fn.eventTrigger?.service != exFn.eventTrigger?.service) {


Thanks for the explanation! Right when i joined, I spent some time debugging some GCF + CF3 tests that wouldn't work on staging, and I distinctly remember being super confused by why preprod talked to prod services.

Anecdotes aside, makes sense to me to deprecate this.

joehan · 2021-05-03T23:26:12Z

src/test/deploy/functions/discovery/jsexports/parseTriggers.spec.ts

+
+  const BASIC_FUNCTION_NAME: backend.TargetIds = Object.freeze({
+    id: "func",
+    region: "us-central1",


Alternative that should work once we have ESM - just set this to api.functionsDefaultRegion

joehan

LGTM!

Add backend namespace before rebasing codebase onto it. The Backend namespace defines types that are neither the __trigger annotations nor the GCP API types. This follows Google best practices for implementing services because it buffers the system from changes in storage or API layers. In this codebase, it will allow us to support new trigger discovery techniques (e.g. a container contract HTTP API) and new versions of APIs if they should ever be needed. In this change I try out a new method for using args.Context; it's based on the Go style of Context where a namespace can provide a value using Context as a private implementation detail. I'm personally a fan. WDYT?

…tructs (#3361) * Functions deploy uses new "backend" types (#3303) Add backend namespace before rebasing codebase onto it. The Backend namespace defines types that are neither the __trigger annotations nor the GCP API types. This follows Google best practices for implementing services because it buffers the system from changes in storage or API layers. In this codebase, it will allow us to support new trigger discovery techniques (e.g. a container contract HTTP API) and new versions of APIs if they should ever be needed. In this change I try out a new method for using args.Context; it's based on the Go style of Context where a namespace can provide a value using Context as a private implementation detail. I'm personally a fan. WDYT? * Add format converters for the new backend flags (#3333) * Wire the new flags into the backend refactor (#3334) Wire up new flag to the backend refactor * Create and Update Function should add deploymentTool labels (#3356) Function create and update should add deployment tool labels * Add prompts for new runWith options (#3352) Add CLI prompts for the new runWith options Also accidentally merged: Fix issues from bug bash 1. Remove the "No HTTPS functions" log line is no longer printed 2. The list of function URLs that are printed is now restricted to this deploy.

…tructs (firebase#3361) * Functions deploy uses new "backend" types (firebase#3303) Add backend namespace before rebasing codebase onto it. The Backend namespace defines types that are neither the __trigger annotations nor the GCP API types. This follows Google best practices for implementing services because it buffers the system from changes in storage or API layers. In this codebase, it will allow us to support new trigger discovery techniques (e.g. a container contract HTTP API) and new versions of APIs if they should ever be needed. In this change I try out a new method for using args.Context; it's based on the Go style of Context where a namespace can provide a value using Context as a private implementation detail. I'm personally a fan. WDYT? * Add format converters for the new backend flags (firebase#3333) * Wire the new flags into the backend refactor (firebase#3334) Wire up new flag to the backend refactor * Create and Update Function should add deploymentTool labels (firebase#3356) Function create and update should add deployment tool labels * Add prompts for new runWith options (firebase#3352) Add CLI prompts for the new runWith options Also accidentally merged: Fix issues from bug bash 1. Remove the "No HTTPS functions" log line is no longer printed 2. The list of function URLs that are printed is now restricted to this deploy.

inlined requested a review from joehan April 23, 2021 00:24

google-cla bot added the cla: yes Manual indication that this has passed CLA. label Apr 23, 2021

joehan reviewed Apr 27, 2021

View reviewed changes

google-cla bot added cla: no and removed cla: yes Manual indication that this has passed CLA. labels Apr 27, 2021

inlined force-pushed the inlined.gcf-typing branch from a66c8fe to 713bd56 Compare April 28, 2021 21:35

google-cla bot added cla: yes Manual indication that this has passed CLA. and removed cla: no labels Apr 28, 2021

joehan requested changes Apr 29, 2021

View reviewed changes

inlined added 2 commits April 30, 2021 11:19

PR feedback

857353f

Merge remote-tracking branch 'origin/master' into inlined.gcf-typing

3f039d6

inlined changed the base branch from master to launch.backend-refactor May 1, 2021 01:05

inlined added 2 commits April 30, 2021 18:18

Merge branch 'inlined.gcf-typing' of github.com:firebase/firebase-too…

f24606b

…ls into inlined.gcf-typing

Fix use before assignment bug

71e8bf2

joehan reviewed May 3, 2021

View reviewed changes

PR feedback

449043e

joehan approved these changes May 4, 2021

View reviewed changes

inlined added 2 commits May 4, 2021 13:59

Fix import issue with emulator

8c535df

Lint fix

f2c1070

inlined merged commit 192d514 into launch.backend-refactor May 4, 2021

inlined deleted the inlined.gcf-typing branch May 5, 2021 01:47

	it(`failurePolicy ${name}`, () => {
	it(`should handle failurePolicy=${name}`, () => {

Functions deploy uses new "backend" types #3303

Functions deploy uses new "backend" types #3303

Conversation

inlined commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Apr 27, 2021

inlined commented Apr 27, 2021

google-cla bot commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehan left a comment

Choose a reason for hiding this comment