-
Notifications
You must be signed in to change notification settings - Fork 917
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
Firebase CLI Supports Firestore Provisioning API #5616
Conversation
Addressed comments and after discussion with some of the team added another option to the create command to set delete protection state as well as another command - Please take a look and pending council approval i'll merge the changes |
CHANGELOG.md
Outdated
- firestore:databases:get | ||
- firestore:databases:delete | ||
- firestore:locations | ||
- Releases Cloud Firestore emulator v1.16.1, which adds support for read_time in ListCollectionIds. |
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.
Unsure as to why this line is there?
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.
ah yep good catch, another stray changelog from a bad merge
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.
Couple small things but code LGTM - will leave any discussion of the API surface to other contexts
return; | ||
} | ||
const deleteProtectionState: types.DatabaseDeleteProtectionState = | ||
options.deleteProtectionState || types.DatabaseDeleteProtectionState.DISABLED; |
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.
Nit: Prefer ?? over || here - it shouldn't make a functional difference with the current code, but ?? only falls through to the right when the left side is undefined (not just falsy), which is the intent here
CHANGELOG.md
Outdated
@@ -1,2 +1,8 @@ | |||
- Adds support for the Firestore Provisioning API commands: (#5616) |
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.
Consider rewording this message to be a little clearer to external users - ie "Adds new commands for provisioning and managing Firestore databases:"
Also, include the databases:update command here too
"JSON specification format." | ||
) | ||
.option("--database <databaseId>", "Name of database to be updated. (required)") | ||
.option( |
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.
Nit: If I'm reading the code correctly, type can be updated, but its not obvious from this help text. Consider clarifying to something like ie Type to update the database to
- with the current wording, it sound like i'm supposed to provide the current type/
appEngineIntegrationMode: string; | ||
keyPrefix: string; | ||
deleteProtectionState: string; | ||
etag: string; |
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 - I've been partial to string union types instead of JS enums, as they require a little less type casting when used. You could consider them instead:
deleteProtectionState: "DELETE_PROTECTION_ENABLED" || "DELETE_PROTECTION_DISABLED"
added changes based on comments from firebase api council:
|
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 last thing to fix, and this should be good to go!
|
||
export const command = new Command("firestore:databases:create <database>") | ||
.description("Create a database in your Firebase project.") | ||
.option("--json", "Prints raw json response of the create API call if specified") |
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.
Applies for all commands: No need to explicitly declare --json as an option - it is defined as a program option for firebase
, not on a per command basis (https://github.com/firebase/firebase-tools/blob/master/src/index.ts#L16)
To support --json, all you need to do is have your command return the JSON that should be shown - in this case, that's databaseResp, so you should be all good.
This looks great. Just confirming, does this allow multiple Firestore DBs in different locations? (e.g. 1 DB in |
yes, but for now only for projects that have the multiple databases feature enabled (in private preview). most projects will not see this capability until later when it is generally available (cannot confirm any exact dates, but keep an eye out for announcements). error messages will say something along the lines of max quota reached. |
THIS IS SO EXCITING |
b/267473272 - Add support for Firestore Provisioning API. This feature is experimental and may not yet be enabled on all projects. It introduces 6 new commands to the Firebase CLI
b/267473272 - Add support for Firestore Provisioning API. This feature is experimental and may not yet be enabled on all projects. It introduces 6 new commands to the Firebase CLI
b/267473272
Add support for Firestore Provisioning API.
This feature is experimental and may not yet be enabled on all projects. It introduces 6 new commands to the Firebase CLI:
List Locations (firestore:locations)
Testing:
Create Database (firestore:databases:create)
Testing:
List Databases (firestore:databases:list)
Testing:
Get Database (firestore:databases:get)
Testing:
Update Database (firestore:databases:update)
Testing
Delete Database (firestore:databases:delete)
Testing: