Create rollout from branch or git commit #7687
Conversation
src/apphosting/index.ts
Outdated
| location: string; | ||
| backendId: string; | ||
| buildInput: DeepOmit<Build, apphosting.BuildOutputOnlyFields | "name">; | ||
| isFirstRollout?: boolean; |
There was a problem hiding this comment.
consider adding a note on why this field is necessary
| .description("create a rollout using a build for an App Hosting backend") | ||
| .option("-l, --location <location>", "specify the region of the backend", "us-central1") | ||
| .option("-i, --id <rolloutId>", "id of the rollout (defaults to autogenerating a random id)", "") | ||
| .option("-b, --git-branch <gitBranch>", "repository branch to deploy (defaults to 'main')", "") |
There was a problem hiding this comment.
can we do -gb and -gc for shorthand?
This will leave -b open for branches
| .description("create a rollout using a build for an App Hosting backend") | ||
| .option("-l, --location <location>", "specify the region of the backend", "us-central1") | ||
| .option("-i, --id <rolloutId>", "id of the rollout (defaults to autogenerating a random id)", "") | ||
| .option("-b, --git-branch <gitBranch>", "repository branch to deploy (defaults to 'main')", "") |
There was a problem hiding this comment.
let's remove the default comments here.
either add a default value or drop it entirely.
| .description("create a rollout using a build for an App Hosting backend") | ||
| .option("-l, --location <location>", "specify the region of the backend", "us-central1") | ||
| .option("-i, --id <rolloutId>", "id of the rollout (defaults to autogenerating a random id)", "") | ||
| .option("-b, --git-branch <gitBranch>", "repository branch to deploy (defaults to 'main')", "") | ||
| .option("-c, --commit <commit>", "git commit to deploy (defaults to the latest commit)", "") | ||
| .option("-f, --force", "skip confirmation") |
There was a problem hiding this comment.
I believe there's a withForce method that you can use here instead of a custom force flag
| .description("create a rollout using a build for an App Hosting backend") | ||
| .option("-l, --location <location>", "specify the region of the backend", "us-central1") | ||
| .option("-i, --id <rolloutId>", "id of the rollout (defaults to autogenerating a random id)", "") | ||
| .option("-b, --git-branch <gitBranch>", "repository branch to deploy (defaults to 'main')", "") | ||
| .option("-c, --commit <commit>", "git commit to deploy (defaults to the latest commit)", "") |
There was a problem hiding this comment.
consider adding a note that this is mutually exclusive with -gb
| backend.codebase.repository, | ||
| ); | ||
|
|
||
| let target; |
| const branchInfo = await getGitHubBranch(owner, repo, branch, readToken.token); | ||
| target = branchInfo.commit; | ||
| } else if (commit) { | ||
| if (!/^(?:[0-9a-f]{40}|[0-9a-f]{7})$/.test(commit)) { |
There was a problem hiding this comment.
consider pulling this up to a file level const with a proper name to make it clear that this is our commit sha regex
| const commitInfo = await getGitHubCommit(owner, repo, commit, readToken.token); | ||
| target = commitInfo; | ||
| } catch (err: unknown) { | ||
| if ((err as FirebaseError).status === 422) { |
There was a problem hiding this comment.
422 HTTP status code returned by GitHub indicates it was unable to find the commit in the repo. I'll add a comment to clarify this
| target = branchInfo.commit; | ||
| } | ||
|
|
||
| logBullet(`You are about to deploy [${target.sha.substring(0, 7)}]: ${target.commit.message}`); |
There was a problem hiding this comment.
let's make sure we update these as well
There was a problem hiding this comment.
What should we make sure to update here?
| buildInput: { | ||
| source: { | ||
| codebase: { | ||
| commit: target.sha, |
There was a problem hiding this comment.
this is making me realize we need an explicit type for the target declaration
There was a problem hiding this comment.
target is of type GitHubCommitInfo, which is defined in src/apphosting/githubConnections.ts. Exported the interface and explicitly typed targetCommit: GitHubCommitInfo.
src/apphosting/rollout.ts
Outdated
| * Create a new App Hosting rollout for a backend. | ||
| * Implements core logic for apphosting:rollouts:create command. | ||
| */ | ||
| export async function doSetup( |
There was a problem hiding this comment.
is there a better name fo rthis function? createRollout maybe?
| const commit = options.gitCommit as string | undefined; | ||
| if (branch && commit) { | ||
| throw new FirebaseError( | ||
| "Cannot specify both a branch and commit to deploy. Please specify either --git-branch or --commit.", |
|
|
Create rollouts from the CLI by specifying a backend and a Git branch or commit.