Implementing createDeploymentPlan #3081

joehan · 2021-01-26T07:27:11Z

Description

Implementing createDeploymentPlan, per the design at go/cf3-deployment-refactor. Part of my goal for this refactor is to separates out the logic for what to deploy from how to deploy it, to make the code more testable and easier to change in the future. This PR handles the first part of that, and will followed up by another PR to execute a deployment plan .

Scenarios Tested

In addition to the cases covered by the unit tests, I locally modified the release step to call createDeploymentPlan and print out the results, and manually tested out a bunch of different permutations of function creates, updates, and deletes, with and without schedules and --only flags.

bkendall

Honestly just some nitty things, but looks fine overall. Maybe someone on Functions should definitely check it out before merging though :)

bkendall · 2021-01-26T17:53:43Z

src/functionsDeployHelper.ts

+  return _.chain(regionMap)
+    .map((value: CloudFunctionTrigger[]) => {
+      return value;
+    })
+    .flatten()
+    .value();


What is your feeling about _.chain? Personally, I'm not a fan.

Is this equivalent?

const triggers: CloudFunctionTrigger[] = []; for (const [k, v] of Object.entries(regionMap)) { triggers.push(...v); } return triggers;

If you're really wanting to keep _.chain though, here's a simplification you can make:

Suggested change

return _.chain(regionMap)

.map((value: CloudFunctionTrigger[]) => {

return value;

})

.flatten()

.value();

return _.chain(regionMap)

.map((v: CloudFunctionTrigger[]) => v)

.flatten()

.value();

I'm not the biggest fan of _.chain either - its tough to understand what going on with it if you're not familiar with lodash, and its always been unintuitive to me that it doesn't execute any of the chained functions until you call '.value()'.

However, as I'm working on the follow up PR, I've realized that I can get rid of this method entirely with some minor refactoring of checkIam.ts. For now, I think I'll go with the simplified _.chain approach - in a follow up PR, this should be gone entirely.

heh - seriously though, my goal is to get rid of lodash entirely. So much of it can be done with plain JS (or a simple helper function), so it's not unattainable...

+💯 to getting rid of lodash (especially the more esoteric bits) everywhere we're rewriting code.

bkendall · 2021-01-26T17:54:12Z

src/functionsDeployHelper.ts

+export interface RegionalDeployment {
+  region: string;
+  sourceToken?: string;
+  firstFunctionDeployment?: CloudFunctionTrigger;
+  functionsToCreate: CloudFunctionTrigger[];
+  functionsToUpdate: CloudFunctionTrigger[];
+  schedulesToCreateOrUpdate: CloudFunctionTrigger[];
+}
+
+export interface DeploymentPlan {
+  regionalDeployments: RegionalDeployment[];
+  functionsToDelete: string[];
+  schedulesToDelete: string[];
+}


Can all the interfaces in this file be collected at the top? It'll make them easier to find later

bkendall · 2021-01-26T17:55:30Z

src/functionsDeployHelper.ts

+        const matchingExistingFunction = _.find(existingFunctions, (exFn) => {
+          return exFn.name === fn.name;
+        });


Suggested change

const matchingExistingFunction = _.find(existingFunctions, (exFn) => {

return exFn.name === fn.name;

});

const matchingExistingFunction = existingFunctions.find((exFn) => {

return exFn.name === fn.name;

});

(Can you tell my feelings on lodash in general now? haha)

Didn't realize there was a native version of this - much nicer.

bkendall · 2021-01-26T17:59:14Z

src/functionsDeployHelper.ts

+  });
+  // Also delete any schedules for functions that we are deleting.
+  _.forEach(functionsToDelete, (fn) => {
+    if (_.get(fn, "labels.deployment-scheduled") === "true") {


I think you could do fn.labels?.["deployment-scheduled"] === "true"

(the last thing is ["thing"] because of the kabab-casing)

I believe you're right - plus, its only written this way because I copied this over from old functions code that was written before optional chaining existed. I definitely prefer using the native option, so I'll switch this over.

mbleigh

I think this all mostly makes sense. I like that we are parsing the triggers into a concrete set of operations in the prepare phase, and I didn't see anything obviously wrong with any of the logic. Some high-level comments:

Please remove lodash wherever possible. We should aim to never use lodash in code we're writing today.
Don't feel beholden to the existing file structure. You're cramming a lot of stuff into functionsDeployHelper.ts which is a pretty poorly named file. Feel free to put things in deploy/functions/... in a more modular fashion if you think it will help clarity.

mbleigh · 2021-01-28T01:55:32Z

src/deploy/functions/prepare.ts

-  payload.functions.triggers = getFunctionsInfo(
-    options.config.get("functions.triggers"),
-    projectId
+  payload.functions.regionMap = createFunctionRegionMap(


Maybe byRegion instead of regionMap would be a more clear name? I keep expecting this to be some kind of region translation map.

mbleigh · 2021-01-28T01:57:30Z

src/deploy/functions/prepare.ts

-  // Display a warning and prompt if any functions have failurePolicies.
-  await promptForFailurePolicies(options, payload.functions.triggers);
+  // Check what --only filters have been passed in.
+  context.filters = getFilterGroups(options);


Do you have a sense of when to use payload and when to use context? I vaguely remember our rationale from eons ago but my guess is that it's mostly a distinction without a difference at this point. I think I'd probably bias toward stuffing things in context instead of payload, but 🤷 no strong opinion.

Honestly, I don't. From reading through the existing code, I kind of figured out that payload = 'stuff that is being deployed' and context = 'things we want to remember for later in the process', but a lot of the things in the code today blur the line between those. As I go thru and address comments on this, I'll probably follow your advice of using context whenever possible.

mbleigh · 2021-01-28T01:58:57Z

src/functionsDeployHelper.ts

+  if (!filterGroups.length) {
+    return true;
+  }
+  return _.some(filterGroups, (groupChunks) => {


Would love to 🔪 lodash everywhere we can, and I don't actually know what _.some does by sight. Can we rewrite in plain JS?

Almost certainly can get rid of lodash here, this was just some old code that I moved from its original home in release.ts and didn't think to rewrite.

mbleigh · 2021-01-28T01:59:37Z

src/functionsDeployHelper.ts

-    // SDK exports list of regions for each function to be deployed to, need to add a new entry
-    // to functionsInfo for each region.
+    // Create a separate CloudFunction for
+    // each region we deploy a function to
    _.forEach(trigger.regions, (region) => {
      const triggerDeepCopy = JSON.parse(JSON.stringify(trigger));


LOL this is horrible but probably fine.

mbleigh · 2021-01-28T02:00:20Z

src/functionsDeployHelper.ts

+  projectId: string,
+  parsedTriggers: CloudFunctionTrigger[]
+): RegionMap {
+  const regionMap: RegionMap = {};
  _.forEach(parsedTriggers, (trigger) => {


Please move to either for...of or .forEach from lodash. Don't have to do it everywhere, but I'd like us to do it in the places we're touching code.

mbleigh · 2021-01-28T02:10:45Z

src/functionsDeployHelper.ts

+        const isMatchingExisitingFnScheduled =
+          _.get(matchingExistingFunction, "labels.deployment-scheduled") === "true";
+        // Check if the local function is a scheduled function
+        const isScheduled = _.has(fn, "schedule");


Why not move this down below where the rest of the schedule handling logic is?

mbleigh · 2021-01-28T02:12:16Z

src/functionsDeployHelper.ts

+  // Delete any remaining existing functions that:
+  // 1 - Have the deployment-tool: 'firebase-cli' label and
+  // 2 - Match the --only filters, if any are provided.
+  const functionsToDelete = _.chain(existingFunctions)


👴 bernie voice I am once again asking you to factor out lodash.

mbleigh · 2021-01-28T02:13:28Z

src/functionsDeployHelper.ts

+        // Check if the local function is a scheduled function
+        const isScheduled = _.has(fn, "schedule");
+
+        if (!matchingExistingFunction) {


In general can you try to move the const declarations as close to their first use as possible? I think it'd read easier.

mbleigh · 2021-01-28T02:15:20Z

src/test/functionsDeployHelper.spec.ts

+    it("should put new functions into functionsToCreate", () => {
+      const regionMap: helper.RegionMap = {
+        "us-east1": [
+          {


Might be easier to read these tests if you had a helper to generate the basic schema here.

mbleigh · 2021-01-28T02:16:14Z

src/test/functionsDeployHelper.spec.ts

+      expect(deploymentPlan).to.deep.equal(expected);
+    });
+
+    it("should delete existing functions not in local code, only if they were deployed via CLI", () => {


Just checking that you didn't do anything to alter the logic that prompts before doing any deletes.

Nope (or at least I didn't intend to). This may have just been a confusing title for the test - basically, this is checking that when there are no --only filters, we delete all existing functions that a - have the deployment-tool:cli-firebase label and b- don't have a matching local function.

joehan · 2021-01-28T07:39:13Z

src/deploy/functions/prompts.ts

+import { FirebaseError } from "../../error";
+import { promptOnce } from "../../prompt";
+import * as utils from "../../utils";
+


There another prompt for deleting functions that is currently in release.ts - I'll move that here in next PR which refactors release.ts to execute these deploymentPlans.

mbleigh

I think this is looking good. Since this is a big risky change we'll need to make sure we do lots of manual testing, but I'm good to go.

mbleigh · 2021-01-28T23:44:33Z