-
Notifications
You must be signed in to change notification settings - Fork 938
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
Add support for deploying 2nd gen Firestore auth context triggers #6734
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.
LGTM with some minor nits
CHANGELOG.md
Outdated
@@ -1 +1,2 @@ | |||
- Add new 2nd gen Firestore auth context triggers. (#1519) |
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.
- Add new 2nd gen Firestore auth context triggers. (#1519) | |
- Add new 2nd gen Firestore triggered functions with auth context . (#1519) |
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.
Mostly just want to make sure this includes the word 'functions' - feel free to reword as you see fit
src/deploy/functions/prompts.ts
Outdated
if (options.nonInteractive) { | ||
utils.logLabeledWarning( | ||
"functions", | ||
"Aborting updates for functions that may be unsafe to update. To update these functions anyway, deploy again in interactive mode or use the --force 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.
Super nit: "Skipping" instead of "Aborting" - in most other places, 'aborting' is used when we cancel the entire command, so it might be misleading here.
} | ||
for (const eu of fnsToUpdateSafe) { | ||
const e = eu.endpoint; | ||
const key = `${e.codebase}-${e.region}-${e.availableMemoryMb || "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.
Based on the linter warning, i think e.codebase
can be undefined. Consider a safe fallback for that case ie ${e.codebase ?? ""}-${e.region}-${e.availableMemoryMb || "default"}
eventTrigger: firestoreEventTrigger, | ||
}; | ||
|
||
it("should prompt if there are potentially unsafe function updates", async () => { |
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: In tests files, best practice is to use whitespace to split the test into steup, act, assert. For example (you can omit the comments when actually doing this):
// Setup
promptStub.resolves(false);
const epUpdates = [
{
endpoint: v2Endpoint0,
},
{
endpoint: v2Endpoint1,
unsafe: true,
},
];
// Act
await functionPrompts.promptForUnsafeMigration(epUpdates, SAMPLE_OPTIONS);
// Assert
expect(promptStub).to.have.been.calledOnce;
3e3328f
to
8344573
Compare
Auth context support has only been available for RTDB 1st gen functions — until now! The Firestore and Eventarc teams have recently added support for Firestore Cloud Events to include event actor information, adding four new event types. These changes enable us to offer the following four new 2nd gen Firestore trigger types in the Firebase Functions SDK, whose events will additionally contain the auth context along with the existing event data:
onDocumentWrittenWithAuthContext
onDocumentUpdatedWithAuthContext
onDocumentCreatedWithAuthContext
onDocumentDeletedWithAuthContext
See the functions SDK PR for more details: firebase/firebase-functions#1519
Users should take special care to avoid event loss when migrating from the original 2nd gen Firestore trigger to the auth-context trigger. The CLI will prompt the user to confirm unsafe migrations that may result in triggers dropping events during the event type update process. See the migration guide for more details about best practices.