-
Notifications
You must be signed in to change notification settings - Fork 956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rollouts:create to handle backend regionality & other fixes #7862
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerned about the breaking flag changes
} | ||
|
||
location = | ||
location || (await promptLocation(projectId, "Select a location to host your backend:\n")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these commands support noninteractive use cases? If so, we should double check this code - it seems like it will prompt even in noninteractive mode if there is no location.
const rootDir = await promptOnce({ | ||
name: "rootDir", | ||
type: "input", | ||
default: "/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this default to . instead? / can easily be mistaken for an absolute path.
async function promptNewBackendId( | ||
projectId: string, | ||
location: string, | ||
prompt: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a stronger type than any here
try { | ||
await apphosting.getBackend(projectId, location, backendId); | ||
} catch (err: any) { | ||
if (err.status === 404) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ingrid recently added type safe helpers for getting status or message from an error (
Line 63 in 1d4b580
export function getErrStatus(err: unknown, defaultStatus?: number): number { |
Applies throughout.
@@ -0,0 +1,482 @@ | |||
import * as clc from "colorette"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional - this files has a mix of prompting functions and API calling functions. In the past, I've see this lead to really large and unwieldy files. If you'd like to get ahead of that, consider splitting this into backendPrompts.ts and backend.ts
@@ -7,13 +7,12 @@ import { createRollout } from "../apphosting/rollout"; | |||
|
|||
export const command = new Command("apphosting:rollouts:create <backendId>") | |||
.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("-l, --location <location>", "specify the region of the backend", "-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These flag changes are breaking changes, and ideally should be part of the next major version., unless App Hosting has a strong argument for allowing breaks in minor versions.
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for making this breaking change.
I currently using version 13.27.0 and the manual rollout option is missing: error: apphosting:rollouts:create is not a Firebase command |
Fixes the following issues: