Implements logic to execute deployment plan by joehan · Pull Request #3107 · firebase/firebase-tools · GitHub

Description

Refactors release.ts to build and execute deploymentPlans when deploying functions.

This also changes the release code to use the same operation polling and throttler code as the rest of the codebase. In a follow up PR, I'll get rid of the last few usages of the old polling code (in the functions:delete command), and I'll clean up that code as well.

Scenarios Tested

I've been running a ton of test deploys with this code and I'm going to continue to do so while this PR is under review. Some situations I've tested so far:

Deploying 10+ functions across multiple regions, and verifying that sourceToken is used all deploys after the first in a region.
Deployments that include deleting functions
Updating scheduled pubsub functions into unscheduled ones, and vice versa
Deleting scheduled functions.

…ctually resolve

Figuring this out drove me up a wall for a day and a half :(. It turns out that our implementation of throttler.wait() returns a promise that never resolves if you call it after .process() and .close(). I'd like to fix this at some point, but that code is used in a bunch of other places that I don't understand well enough to test effectively.

Realized that I could turn this into an implementation detail of runRegionalFunctionDeployment, so I removed this from the type.

This file has a mix of random helper functions, and chaff from the old deployment code that I can't quite delete yet because its used on functions-delete and other code paths. I'm gonna make another PR later to clean up the old functions, and move the still relevant functions into a less generically named file inside of the src/deploy/functions directory.

The implementation for getFilterGroups() looks very different from what you've done here. Could you share a bit more insight into why this change was necessary?

(Also - if this change is intended, we could get rid of the import statement on getFilterGroups)

Good catch on the import! Its not the prettiest code flow, but in prepare.ts, we have a line that already does this work: context.filters = getFilterGroups().

I figured it would be nice to avoid redoing this work - however, it does sacrifice some readability, and it could lead to subtle bugs if this code ever gets used elsewhere. I'll make this fall back to getFilterGroups() if context.filters is undefined.

I keep getting confused what name refers to in different context.

Say we are given functions resource projects/my-projc/locations/us-central1/functions/myfunc. In this case, name has to bee the fully-qualified name (with project Id & location) right?

If that's right, I'm starting to get worried that this could become a source of pain and bugs. Is that your experience as well?

The Google API style guide defines myfunc as the "id" and projects/my-projc/locations/us-central1/functions/myfunc as the "name". IMO the only solution here is to be pedantically consistent.

This definitely is confusing and ive introduced a few bugs during development because of it. I'll go through and switch everything to id and name to follow the style guide

If I'm reading this correctly, I think this will print fully qualified name (including project, region) vs other tasks print function name only. Is that okay?

This is working as intended - printSuccess takes a fully qualified name. CloudFunctionTrigger.name is the fully qualified name, so the other tasks behave the same way.

nit* can we write this as

40 * 60 * 60 // 40 hours

Also - did I get this right? 40 hours looks like a very generous timeout.

If we're guessing what the units are in this timeout that's a good hint that it should be included in the variable name. E.g. masterTimeoutMs

This is in ms, so its 150 seconds. I'll add a comment clarifying this for now. I'd like to make some improvements to throttler at some point in the future since it wasn't the clearest to work with here - when I do so, I'll rename this option to masterTimeoutMs for clarity.

LGTM with the logging fixes Daniel pointed out.

enum string types make my heart sing. Thanks for this.

I think you mean unauthenticated. One never wants an unauthorized user to have access.

You are correct! I'm also gonna get @egilmorez to take a look at this to shop the language further at some point.

Or because roles/functions.developer doesn't have functions.setIamPermission on it. You need to be a roles/functions.admin. The required permissions is a major oversight in my opinion, but this is a battle we've already lost.

Ooof, I didn't realize that was the case. I'll reword this to have a bulleted list of common reasons, and include that as the first one.

what does "Dep" mean? I read "dependency" in my head, but it's nothing that starts with "dep" in the struct.

Deployment - I'll replace this with the full word to make this clearer

FWIW, I actually preferred the namespaced option since "ensure" and "check" are so generic. Up to you though.

If we're guessing what the units are in this timeout that's a good hint that it should be included in the variable name. E.g. masterTimeoutMs

Just to make sure I understand the latest JS, could this have been written fn?.eventTrigger?.eventType || "https"

Yep, it could have been written either way

This is any synchronous callback to be invoked when a polling operation happens? Surely we can at least type the argument to the function. And I'm assuming the return type is void?

Ahh yes, this was an oversight - will fix

Not for this PR for we should change functionName argument to functionId soon? I volunteer to do that later.

Ahhh, this one slipped past me. I like the idea of doing this in a separate PR later - it should be easier to track down any places we missed it then

Same here - functionName vs Id is getting tricky.

+100 on these - i just found another bug caused by this. I was accidentally using name instead of ID for a call to setIamPolicy, and didn't notice until now because that call was failing for me anyway due to the org policy.

joehan added 13 commits January 21, 2021 15:41

refactoring release step of functions deploy to use typescript

3be0dca

Adding logic to build regional deployments

e0e703e

Implementing createDeploymentPlan

046c7d7

First round of PR feedback, removing most usages of lodash

b876523

moving function prompts into their own file

9e0e6e9

seperating out a bunch of code from functionsDeployHelper

2a3b547

Resolves merge conflicts

51f2395

Implements core logic of running function deploys

6916000

adding timing and logs for deployments

85d0afe

cleaning up unused code

00b1989

Fixing some things that were broken while merging

397d7c4

Fixing up the order of wait and close to ensure that queue promsies a…

21f4906

…ctually resolve

Format and clean up typos

3b3edbd

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

joehan changed the title Jh execute deployment plans Implements logic to execute deployment plan Feb 4, 2021

joehan requested review from inlined, mbleigh and taeold February 4, 2021 21:10

joehan commented Feb 4, 2021

View reviewed changes

joehan commented Feb 5, 2021

View reviewed changes

Comment thread src/deploy/functions/release.ts Outdated

refactoring error handling to be cleaner

e428bcb

joehan commented Feb 8, 2021

View reviewed changes

cleaning up extera newlines

4c8e2fb

joehan mentioned this pull request Feb 8, 2021

Refactor functions-delete #3110

Merged

taeold reviewed Feb 8, 2021

View reviewed changes

inlined reviewed Feb 8, 2021

View reviewed changes

joehan added 3 commits February 8, 2021 16:20

first round of pr fixes

7f48130

Readding some changes that I accidenttally wiped out during a merge

39a7e86

Switching name to id where appropriate

1366955

joehan requested review from inlined and taeold February 9, 2021 01:15

taeold approved these changes Feb 9, 2021

View reviewed changes

fixing another bug caused by functionName vs Id

7513229

joehan merged commit 8d3d82d into jh-functions-refactor Feb 9, 2021

joehan deleted the jh-execute-deployment-plans branch February 9, 2021 20:48

               export interface RegionalDeployment {
                 region: string;
                 sourceToken?: string;
-                firstFunctionDeployment?: () => any;

               export async function checkHttpIam(context: any, options: any, payload: any): Promise<void> {
                 const functionsInfo = payload.functions.triggers;
-                const filterGroups = getFilterGroups(options);
+                const filterGroups = context.filters;

-                            projectId: projectId,
-                            region: helper.getRegion(name),
-                            functionName,
+                            functionName: name,

+                    );
+                    const operationResult = await pollOperation<void>(pollerOptions);
+                    params.timer.endTimer(fnName);
+                    helper.printSuccess(fnName, "delete");

+              const defaultPollerOptions = {
+                apiOrigin: functionsOrigin,
+                apiVersion: cloudfunctions.API_VERSION,
+                masterTimeout: 150000,

-                  throw new FirebaseError("Functions did not deploy properly.");
-                }
+                // Once everything has been added to queues, starting processing.
+                // Note: We need to set up these wait before calling process and close.

+              import Queue from "./throttler/queue";
-              // TODO: Get rid of this when switching to use operation-poller.
+              // TODO(joehan): Get rid of this once we refactor functions-delete.js

+              import * as logger from "../../logger";
+              import { FirebaseError } from "../../error";
+              type OperationType =

+                    for (const failedDep of failedIamCalls) {
+                      logger.info(`\t${failedDep.functionName}`);
+                    }
+                    logger.info("\nUnauthorized users will not be able access this function.");

+                  logger.info("\nTo continue deploying other features (such as database), run:");
+                  logger.info("    " + clc.bold("firebase deploy --except functions"));
+                  // Print all the original messages at debug level.
+                  for (const failedDep of this.errors) {

               import * as clc from "cli-color";
-              import * as ensureApiEnabled from "../../ensureApiEnabled";
+              import { ensure, check } from "../../ensureApiEnabled";

+                      "..."
+                  );
+                  params.timer.startTimer(fn.name, "create");
+                  const eventType = fn.eventTrigger ? fn.eventTrigger.eventType : "https";

+              export function createFunctionTask(
+                params: TaskParams,
+                fn: CloudFunctionTrigger,
+                onPoll?: (op: any) => any

                       region: helper.getRegion(fn.name),
                       eventType: eventType,
-                      functionName: helper.getFunctionName(fn.name),
+                      functionName: helper.getFunctionId(fn.name),

                   const scheduleName = helper.getScheduleName(name, appEngineLocation);
                   const topicName = helper.getTopicName(name);
-                  const functionName = helper.getFunctionName(name);
+                  const functionName = helper.getFunctionId(name);

Implements logic to execute deployment plan by joehan · Pull Request #3107 · firebase/firebase-tools · GitHub

Uh oh!

Implements logic to execute deployment plan - #3107

Implements logic to execute deployment plan#3107joehan merged 19 commits intojh-functions-refactorfirebase/firebase-tools:jh-functions-refactorfrom jh-execute-deployment-plansfirebase/firebase-tools:jh-execute-deployment-plansCopy head branch name to clipboard

Conversation

joehan commented Feb 4, 2021

Description

Scenarios Tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

inlined left a comment

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!

Reviewers

Assignees

Labels

Implements logic to execute deployment plan#3107
joehan merged 19 commits into
jh-functions-refactorfrom
jh-execute-deployment-plans