Fix rollouts:create to handle backend regionality & other fixes#7862
Fix rollouts:create to handle backend regionality & other fixes#7862blidd-google merged 10 commits intomasterfrom
Conversation
joehan
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
Ingrid recently added type safe helpers for getting status or message from an error (
Line 63 in 1d4b580
Applies throughout.
| @@ -0,0 +1,482 @@ | |||
| import * as clc from "colorette"; | |||
There was a problem hiding this comment.
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
| .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.
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 |
taeold
left a comment
There was a problem hiding this comment.
+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: