-
Couldn't load subscription status.
- Fork 2
S48-XX: update firestore event trigger type to document instead of da… #84
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
Conversation
|
🚀 Beta version published: 4.1.3-beta.pr.84.20250527165950 You can install this version with: npm install @space48/[email protected] |
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.
@ansnaldo Strictly speaking this is a breaking change, because it's changing how Cloud Seed gen2 firestore functions work today. While we are doing to make the gen2 function work like gen1 did, if someone had the latest version of Cloud Seed installed with a firestore function and they upgrade, then their function will stop working.
Therefore, we need a major version bump in the package and a breaking change note added to the changelog, please.
|
🚀 Beta version published: 5.0.0-beta.pr.84.20250528100948 You can install this version with: npm install @space48/[email protected] |
|
🚀 Beta version published: 5.0.0-beta.pr.84.20250528102549 You can install this version with: npm install @space48/[email protected] |
@tgerulaitis updated. This wouldn't have been working on the previous latest version though right? |
|
🚀 Beta version published: 5.0.0-beta.pr.84.20250528114904 You can install this version with: npm install @space48/[email protected] |
…r firestore functions config
|
🚀 Beta version published: 5.0.0-beta.pr.84.20250528123841 You can install this version with: npm install @space48/[email protected] |
@ansnaldo I haven't tested it to prove this, so I might be wrong, but the existing code should work, just not in the way the The So while the config value doesn't work the way it did in |
types/runtime.ts
Outdated
| export type FirestoreConfig = { | ||
| type: "firestore"; | ||
| document: string; | ||
| database: 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.
@ansnaldo I'm concerned that this is a required value, but it's not used for gen1 functions at all. It means that if someone upgrades Cloud Seed, but wants to keep gen1 functions, they would have to enter some dummy data that is unused.
Can we make this optional? Is it actually required for gen2 functions?
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.
@ansnaldo I'm concerned that this is a required value, but it's not used for
gen1functions at all. It means that if someone upgrades Cloud Seed, but wants to keepgen1functions, they would have to enter some dummy data that is unused.Can we make this optional? Is it actually required for
gen2functions?
@tgerulaitis good point. Yes it looks to be required looking at GCP docs. Also when I tested the first beta version (with document only) I got this error :
Error: Error creating function: googleapi: Error 400: Validation failed for trigger xxx : The request was invalid: missing required attribute "database" in trigger.event_filters
│
│ with google_cloudfunctions2_function.firestore-create-export,
│ on cdk.tf.json line 115, in resource.google_cloudfunctions2_function.firestore-create-export:
│ 115: },
│
╵
Updated now.
…ort gen1 functions
|
🚀 Beta version published: 5.0.0-beta.pr.84.20250528151136 You can install this version with: npm install @space48/[email protected] |
…tabase
Pull Request
Description
Update firestore event trigger type to document instead of database which currently throws an error
Type of Change
Reviewers
@AndrewBarber @tgerulaitis @iamsambrown @seanpertet
Release Checklist
If this PR is preparing a release, please ensure:
package.jsonhas been updated following Semantic Versioning principlesnpm run test)npm run lint)Post-Merge Automation
After merging to master, the following will happen automatically:
Testing
Additional Notes