-
Notifications
You must be signed in to change notification settings - Fork 50
feat(core): Allow multi-project sourcemaps upload #813
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
The `project` option now allows specifiying multiple projects via a string array. Source maps will be uploaded to all specified projects.
export function normalizeUserOptions(userOptions: UserOptions): NormalizedOptions { | ||
const options = { | ||
org: userOptions.org ?? process.env["SENTRY_ORG"], | ||
project: userOptions.project ?? process.env["SENTRY_PROJECT"], | ||
project: userOptions.project ?? (process.env["SENTRY_PROJECT"]?.includes(',') | ||
? process.env["SENTRY_PROJECT"].split(',').map(p => p.trim()) | ||
: process.env["SENTRY_PROJECT"]), | ||
authToken: userOptions.authToken ?? process.env["SENTRY_AUTH_TOKEN"], | ||
url: userOptions.url ?? process.env["SENTRY_URL"] ?? SENTRY_SAAS_URL, | ||
headers: userOptions.headers, |
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.
Potential bug: An invalid SENTRY_PROJECT
env var can create an array with empty strings, bypassing guards if a custom errorHandler
is used, causing CLI calls to fail.
-
Description: When the
SENTRY_PROJECT
environment variable is set to a value that results in empty strings after parsing (e.g.,SENTRY_PROJECT=","
), it creates an invalid project array like[""]
. ThevalidateOptions
function detects this, but if a user has configured a customerrorHandler
that doesn't throw an error, execution continues. Downstream functions likecreateRelease
andcanUploadSourceMaps
contain guards that only check if theproject
array is undefined or has a length of zero. These guards do not validate the contents of the array, so[""]
is considered valid. This leads tosentry-cli
being called with an empty project name, causing the sourcemap upload process to fail during the build. -
Suggested fix: Strengthen the guards in
createRelease
andcanUploadSourceMaps
. Instead of only checking!options.project || options.project.length === 0
, also ensure that all elements within theoptions.project
array are non-empty strings. This prevents invalid data from being passed to the Sentry CLI even if initial validation errors are suppressed.
severity: 0.65, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
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, but I think you still need to run yarn fix:formatting
to make the CI happy
org: options.org, | ||
project: options.project, | ||
project: Array.isArray(options.project) ? options.project[0] : options.project, | ||
projects: Array.isArray(options.project) |
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: There is a lot of code duplication like that, which would make future development harder as we need to know to implement it like that. We could make a function for that, reuse it and write a simple unit test for it (btw this line already differs from the ones below).
E.g. getProjects
and getFirstProject
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.
Yep, good point. I added getProjects
but opted to just use optionals for the first project.
return false; | ||
} | ||
if (!options.project) { | ||
if (!options.project || (Array.isArray(options.project) && options.project.length === 0)) { |
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: Theoretically if there is a getFirstProject
as recommended in another comment we could write following:
if (!options.project || (Array.isArray(options.project) && options.project.length === 0)) { | |
if (!getFirstProject(options.project)) { |
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.
Updated to use !getProjects(options.project)?.[0]
return false; | ||
} | ||
|
||
if (options.project) { |
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: you can flatten it by one if
if (options.project && Array.isArray(options.project)) {
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.
Updated
c39b661
to
db86ea3
Compare
| datasource | package | from | to | | ---------- | ------------------- | ----- | ----- | | npm | @sentry/vite-plugin | 4.4.0 | 4.5.0 | ## [v4.5.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#450) - docs: added info on debug flag value precedence ([#811](getsentry/sentry-javascript-bundler-plugins#811)) - feat: add debug statements after sourcemap uploads ([#812](getsentry/sentry-javascript-bundler-plugins#812)) - feat(core): Allow multi-project sourcemaps upload ([#813](getsentry/sentry-javascript-bundler-plugins#813)) - fix: propagate the debug option to the cli ([#810](getsentry/sentry-javascript-bundler-plugins#810))
The
project
option now allows specifiying multiple projects via a string array. Source maps will be uploaded to all specified projects.Closes: #752