-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(cloudflare): Automatically set the release id when CF_VERSION_METADATA is enabled #18855
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
base: develop
Are you sure you want to change the base?
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| const result = getFinalOptions(userOptions, env); | ||
|
|
||
| expect(result).toEqual({ dsn: 'test-dsn', release: undefined }); | ||
| }); |
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.
Missing integration or E2E test for new feature
Low Severity · Bugbot Rules
This feat PR adds support for CF_VERSION_METADATA as a release source but only includes unit tests. The user-specified review rules require that feat PRs include at least one integration or E2E test. While the unit tests thoroughly cover the getFinalOptions logic, an integration test validating the feature works correctly in an actual Cloudflare environment would strengthen confidence in the change.
|
|
||
| it('ignores CF_VERSION_METADATA when id is not a string', () => { | ||
| const userOptions = { dsn: 'test-dsn' }; | ||
| const env = { CF_VERSION_METADATA: { id: 123 } }; |
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.
l: can this happen/is it common to provide a non-string id? If so, we could stringify the release. Though only worth doing if this is actually a thing
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.
It is not defined what the version ID can be. Right now the id is always a UUID, so a string. But I'm not sure if this is always the case.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
The
CF_VERSION_METADATAonly has one reason - to add the release id.Since we already set the release when
SENTRY_RELEASEis enabled automatically, we can do the same forCF_VERSION_METADATA. The docs will then also be updated to state how the release id can be set in different ways: getsentry/sentry-docs#16006Closes #18856 (added automatically)
Closes JS-1489