Refactor functions-delete #3110

joehan · 2021-02-05T22:15:26Z

Description

Refactoring functions-delete to use the same patterns as the rest of functions deployment. This lets us clean up a bunch of the old operation polling code, too.

Scenarios Tested

Delete a non-scheduled function that exists in 2 regions:

Delete a scheduled function:

…g code

joehan · 2021-02-08T19:10:20Z

src/functionsDelete.ts

+import { DeploymentTimer } from "./deploy/functions/deploymentTimer";
+import { ErrorHandler } from "./deploy/functions/errorHandler";
+
+export async function deleteFunctions(


In the future, I'd like to reuse this code during the normal functions deployment process too - I'm keeping it separate for now to avoid an ugly merge conflict with #3107

inlined · 2021-02-08T18:32:20Z

src/commands/functions-delete.ts

@@ -0,0 +1,88 @@
+"use strict";
+
+import * as _ from "lodash";


Unused import

inlined · 2021-02-08T18:34:27Z

src/commands/functions-delete.ts

+import { requirePermissions } from "../requirePermissions";
+import * as utils from "../utils";
+
+module.exports = new Command("functions:delete [filters...]")


How pervasive is module.exports =? It seems strange that we're mixing coding styles (e.g. getProjectId is an exports = but the other imports in this file aren't). It's been a while, but I thought that this style was generally considered outdated and bad, especially when making exports a non-map type.

This is definitely the wrong style - switched it over to our preferred export default new Command(....

inlined · 2021-02-08T18:36:10Z

src/commands/functions-delete.ts

+    const filterChunks = filters.map((filter: string) => {
+      return filter.split(".");
+    });
+    const config = await functionsConfig.getFirebaseConfig(options);


Totally not the point of this code review, but why is getFirebaseConfig part of functionsConfig? Is this a functions-specific portion of the firebase config?

inlined · 2021-02-08T18:41:17Z

src/commands/functions-delete.ts

+import * as clc from "cli-color";
+import * as cloudfunctions from "../gcp/cloudfunctions";
+import * as functionsConfig from "../functionsConfig";
+import { deleteFunctions } from "../functionsDelete";


TBF I can't honestly guess what the difference is between functionsDelete.js and functions-delete.ts

Good point, its super unclear and there probably isn't a good reason to keep the old names.

FWIW:
functions-delete follows the pattern we use for files defining the different firebase commands - firebase functions:delete is defined by the file functions-delete.ts, firebase database:get is defined by the file database-get, etc.
functionsDelete is just a file that contains code to delete functions - my guess is that whoever originally wrote the file named it as such so that it would show up in their IDE next to all of the other functions* files like functionsConfig, functionsDeployHelper, etc.

If I had a time machine I'd put functions-delete.ts in commands/functions/delete.ts but the ship has sailed.

inlined · 2021-02-08T18:46:45Z

src/commands/functions-delete.ts

+      return filter.split(".");
+    });
+    const config = await functionsConfig.getFirebaseConfig(options);
+    const appEngineLocation = functionsConfig.getAppEngineLocation(config);


AFAICT, the GAE location is only used for scheduler locations. I miiiiiight consider having the local variable be schedulerRegion. Then it might read better as "oooh, I need the location because it's for scheudles and apparently the scheduler region is the GAE region". I might even add a comment for such because it's not obvious to a reader unless they're versed with the history of scheudler as a feature of GAE.

inlined · 2021-02-08T18:52:56Z

src/deploy/functions/tasks.ts

@@ -24,8 +24,8 @@ const defaultPollerOptions = {

 export interface TaskParams {
  projectId: string;
-  runtime: string;
-  sourceUrl: string;
+  runtime?: string;


I'm always suspicious when we make a field optional. Why are we doing that here?

For deleteFunction tasks, there's no use for runtime, so it probably should have been optional (or moved out of this params type) to begin with

inlined · 2021-02-08T18:53:32Z

src/deploy/functions/tasks.ts

@@ -40,7 +40,7 @@ export function createFunctionTask(
    utils.logBullet(
      clc.bold.cyan("functions: ") +
        "creating " +
-        getHumanFriendlyRuntimeName(params.runtime) +
+        getHumanFriendlyRuntimeName(params.runtime!) +


per the comment about optionality, if we're calling runtime! then it seems like the field isn't actually optional.

inlined · 2021-02-08T18:57:42Z

src/functionsDelete.ts

@@ -0,0 +1,41 @@
+import * as helper from "./functionsDeployHelper";
+import { Queue } from "./throttler/queue";


=/ Why isn't this called a ThrottlerQueue or something that makes it not a CompSci 101 data structure? I didn't realize the importance of this class until I looked up why there was a second type parameter.

Not sure, but i agree that this isn't a very clear name. It will have a larger impact radius than i want for this PR since this structure is used across a bunch of code paths, but I think we ought to to rename Queue to ThrottlerQueue and Stack to ThrottlerStack once the functions refactor is fully merged in.

inlined · 2021-02-08T19:07:23Z

src/functionsDelete.ts

+  cloudFunctionsQueue.process();
+  schedulerQueue.process();
+
+  await Promise.all(queuePromises);


Isn't there a bug here where we could fail to delete a schedule but succeed in deleting a function? The next time the user tried to do a delete operation we wouldn't see the function so we wouldn't see its schedule and we wouldn't ever clean up the customer's project.

inlined · 2021-02-08T19:23:40Z

src/gcp/cloudfunctions.ts

+    eventTrigger?: any;
+    httpsTrigger?: any;
+  };
+  error?: { code: number; message: string };


I'm surprised there isn't already a type for a Google error. I'm also vaguely surprised that there isn't already an Operation type. E.g. this could roughly be

import {Operation} from '../cloud/basicTypes'; import {CloudFunction} from '../cloud/cloudFunctions'; interface CloudFunctionOperation = Operation | CloudFunction;

huangjeff5 · 2021-02-08T23:58:11Z

src/commands/functions-delete.ts

+    });
+    if (functionsToDelete.length === 0) {
+      return utils.reject(
+        "The specified filters do not match any existing functions in project " +


nit: would string substitution be better here i.e. The specified filters do not match any existing functions in project ${clc.bold(projectId}. for readability? Then we could fit it in one line.

huangjeff5 · 2021-02-09T00:04:53Z

src/commands/functions-delete.ts

+    const appEngineLocation = functionsConfig.getAppEngineLocation(config);
+    const existingFns = await cloudfunctions.listAllFunctions(projectId);
+    const functionsToDelete = existingFns.filter((fn) => {
+      const regionMatches = options.region ? helper.getRegion(fn.name) === options.region : true;


nit - this might be a simpler way to express this:
!options.region || helper.getRegion(fn.name) === options.region

It's only a bit shorter, but to me it's more comprehensible because I don't have to first mentally convert it to an if statement.

* Adding onPoll option to operation-poller (#3046) * Adds onPoll option * format and remove only * improved unit test * Typescriptify functionsDeployHelper (#3059) * Typescriptifying and adding CloudFunctionTrigger type * Using funcName instead of func * formats * Typescriptifying gcp.cloudfunctions (#3060) * Typescriptifying functionsConfig (#3063) * Typescriptifying deploymentTool (#3061) * Typescriptifying deploymentTool * Update src/deploymentTool.ts Co-authored-by: Michael Bleigh <[email protected]> Co-authored-by: Michael Bleigh <[email protected]> * Refactoring prepare stage of functions deploy (#3067) * refactoring prepare setp of functions deploy to use typescript, check for failure policies, and parse function triggers * refactoring prepare setp of functions deploy to use typescript, check for failure policies, and parse function triggers * commenting out functionNamesAreValid * formats * refactoring release step of functions deploy to use typescript * Adding logic to build regional deployments * Implementing createDeploymentPlan * First round of PR feedback, removing most usages of lodash * moving function prompts into their own file * seperating out a bunch of code from functionsDeployHelper * Resolves merge conflicts * refactoring release step of functions deploy to use typescript (#3071) * Implements core logic of running function deploys * Typescriptifying prepareFunctionsUpload (#3064) * Typescriptifying prepareFunctionsUpload, and updating filesize package to get types * fixing merge conflict * Implementing createDeploymentPlan (#3081) * refactoring release step of functions deploy to use typescript * Adding logic to build regional deployments * Implementing createDeploymentPlan * First round of PR feedback, removing most usages of lodash * moving function prompts into their own file * seperating out a bunch of code from functionsDeployHelper * round of pr fixes * adresses more pr comments, and adds some todos * cleaning up unused code * Fixing some things that were broken while merging * Fixing up the order of wait and close to ensure that queue promsies actually resolve * Format and clean up typos * refactoring error handling to be cleaner * cleaning up extera newlines * first round of pr fixes * Readding some changes that I accidenttally wiped out during a merge * Switching name to id where appropriate * fixing another bug caused by functionName vs Id * Refactor functions-delete (#3110) * Refactoring functions delete to use tasks, and cleaning up old polling code * Refactoring functions delete to use tasks, and cleaning up old polling code * refactoring to use new error handling * cleanup unused imports * small style fixes * Cleaning up error reporting * Implement validation for changing trigger types, and fixes from bug bash (#3131) * Implement validation for changing trigger types, and fixes from bug bash * more specifc error messages for different permutations of trigger types * fixes package.json Co-authored-by: Michael Bleigh <[email protected]>

* Adding onPoll option to operation-poller (firebase#3046) * Adds onPoll option * format and remove only * improved unit test * Typescriptify functionsDeployHelper (firebase#3059) * Typescriptifying and adding CloudFunctionTrigger type * Using funcName instead of func * formats * Typescriptifying gcp.cloudfunctions (firebase#3060) * Typescriptifying functionsConfig (firebase#3063) * Typescriptifying deploymentTool (firebase#3061) * Typescriptifying deploymentTool * Update src/deploymentTool.ts Co-authored-by: Michael Bleigh <[email protected]> Co-authored-by: Michael Bleigh <[email protected]> * Refactoring prepare stage of functions deploy (firebase#3067) * refactoring prepare setp of functions deploy to use typescript, check for failure policies, and parse function triggers * refactoring prepare setp of functions deploy to use typescript, check for failure policies, and parse function triggers * commenting out functionNamesAreValid * formats * refactoring release step of functions deploy to use typescript * Adding logic to build regional deployments * Implementing createDeploymentPlan * First round of PR feedback, removing most usages of lodash * moving function prompts into their own file * seperating out a bunch of code from functionsDeployHelper * Resolves merge conflicts * refactoring release step of functions deploy to use typescript (firebase#3071) * Implements core logic of running function deploys * Typescriptifying prepareFunctionsUpload (firebase#3064) * Typescriptifying prepareFunctionsUpload, and updating filesize package to get types * fixing merge conflict * Implementing createDeploymentPlan (firebase#3081) * refactoring release step of functions deploy to use typescript * Adding logic to build regional deployments * Implementing createDeploymentPlan * First round of PR feedback, removing most usages of lodash * moving function prompts into their own file * seperating out a bunch of code from functionsDeployHelper * round of pr fixes * adresses more pr comments, and adds some todos * cleaning up unused code * Fixing some things that were broken while merging * Fixing up the order of wait and close to ensure that queue promsies actually resolve * Format and clean up typos * refactoring error handling to be cleaner * cleaning up extera newlines * first round of pr fixes * Readding some changes that I accidenttally wiped out during a merge * Switching name to id where appropriate * fixing another bug caused by functionName vs Id * Refactor functions-delete (firebase#3110) * Refactoring functions delete to use tasks, and cleaning up old polling code * Refactoring functions delete to use tasks, and cleaning up old polling code * refactoring to use new error handling * cleanup unused imports * small style fixes * Cleaning up error reporting * Implement validation for changing trigger types, and fixes from bug bash (firebase#3131) * Implement validation for changing trigger types, and fixes from bug bash * more specifc error messages for different permutations of trigger types * fixes package.json Co-authored-by: Michael Bleigh <[email protected]>

joehan added 3 commits February 5, 2021 13:51

Refactoring functions delete to use tasks, and cleaning up old pollin…

ccd93ee

…g code

Refactoring functions delete to use tasks, and cleaning up old pollin…

623bfb4

…g code

refactoring to use new error handling

ed241a8

joehan requested a review from inlined February 5, 2021 22:15

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

joehan requested review from taeold and huangjeff5 February 5, 2021 22:15

joehan commented Feb 8, 2021

View reviewed changes

inlined approved these changes Feb 8, 2021

View reviewed changes

cleanup unused imports

516c0a3

huangjeff5 reviewed Feb 8, 2021

View reviewed changes

huangjeff5 reviewed Feb 9, 2021

View reviewed changes

Base automatically changed from jh-execute-deployment-plans to jh-functions-refactor February 9, 2021 20:48

removing js version

f79faa6

huangjeff5 approved these changes Feb 9, 2021

View reviewed changes

small style fixes

b1de227

joehan merged commit 6d2260e into jh-functions-refactor Feb 9, 2021

joehan deleted the jh-refactor-functions-delete branch February 9, 2021 22:44

		@@ -0,0 +1,41 @@
		import * as helper from "./functionsDeployHelper";
		import { Queue } from "./throttler/queue";

Refactor functions-delete #3110

Refactor functions-delete #3110

Uh oh!

Conversation

joehan commented Feb 5, 2021

Description

Scenarios Tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!