Typescriptifying release step of functions deployment #3071
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
inlined
approved these changes
Jan 29, 2021
There was a problem hiding this comment.
Reviewed this one first to get into the groove of reviewing this codebase. I suspect this was largely just a rename as TS and changing a few syntax styles. I know you didn't write any of this whole cloth and don't expect any of my comments to actually be taken as action items.
For future reviews like this though, I like to do the fork as one commit and the edits as another. It offers the reviewer a way to scan through the changes much more easily.
| done: boolean; | ||
| eventType?: string; |
| interface Deployment { | ||
| name: string; | ||
| retryFunction: () => any; | ||
| trigger?: any; // TODO: type this |
There was a problem hiding this comment.
Is this just TriggerAnnotated from firebase-functions/src/cloud-function? We could add a dev dependency on firebase-functions
| deployments = []; // prevents analytics tracking of deployments | ||
| } | ||
|
|
||
| export async function release(context: any, options: any, payload: any): Promise<void> { |
There was a problem hiding this comment.
This is a 340LOC method that has nested conditionals since we can't early exit anywhere. It would probably be cleaner to actually use helper methods. Just a drive-by comment since this code was just translated, not written in the PR.
There was a problem hiding this comment.
Agree 100 percent - this is getting torn to shreds in a coming PR
| "Would you like to proceed with deletion? Selecting no will continue the rest of the deployments.", | ||
| }); | ||
| } | ||
| if (proceed) { |
There was a problem hiding this comment.
nit: If we tested for !proceed (which I'd probably call skip to avoid a double negative) we would have the smaller block sooner. Then we wouldn't forget what the else block nearly 100 lines lower is referring to.
| const functionsInDeploy = functionsInfo.filter((trigger: helper.CloudFunctionTrigger) => { | ||
| return functionFilterGroups.length > 0 ? _.includes(deleteReleaseNames, trigger.name) : true; | ||
| }); | ||
| await createOrUpdateSchedulesAndTopics( |
There was a problem hiding this comment.
nit: the name for "createOrUpdate" is typically "upsert"
| ); | ||
| const allOps = await utils.promiseAllSettled( | ||
| _.map(deployments, async (op) => { | ||
| const res = await op.retryFunction(); |
There was a problem hiding this comment.
If we only ever call this once, why is it called retryFunction?
joehan
added a commit
that referenced
this pull request
Feb 16, 2021
* 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]>
devpeerapong
pushed a commit
to devpeerapong/firebase-tools
that referenced
this pull request
Dec 14, 2021
* 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Refactoring release step of functions deploy to use Typescript. This PR doesn't have any significant functional changes - I wanted to first change this over to Typescript so that it would be easier to understand what is actually being changed in future PRs.
Scenarios Tested
Ran a series of function deploys to test the following situations: