-
Notifications
You must be signed in to change notification settings - Fork 30
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
ref(core): Switch to v2 options #237
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.
Nice!
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.
Looks good to me! Just had some minor comments but nothing blocking.
if (options.sourcemaps) { | ||
if (!options.authToken) { | ||
logger.warn("No auth token provided. Will not upload source maps."); | ||
} else if (!options.org) { | ||
logger.warn("No org provided. Will not upload source maps."); | ||
} else if (!options.project) { | ||
logger.warn("No project provided. Will not upload source maps."); | ||
} else if (!options.sourcemaps.assets) { | ||
logger.warn("No assets defined. Will not upload source maps."); | ||
} else { |
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: Personally I'd prefer to extract this validation to a function (per plugin) to declutter this one a little but feel free to leave it as is as.
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.
This currently has a use for TS. We could put it into a helper but I am not sure if it will actually simplify the code by a lot. Gonna leave this for now.
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.
Ahh I see, yeah, then let's just leave it.
hub.setTag("finalize-release", true); | ||
} | ||
if (deploy) { | ||
if (release.deploy) { | ||
hub.setTag("add-deploy", true); | ||
} | ||
|
||
// Miscelaneous options |
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-l: Should we add a tag for the new deleteAfterUpload option? Might be interesting to see how often it is used?
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.
We don't have it implemented yet but it probably doesn't hurt to add right now!
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Switches to the new options as decided in https://github.com/getsentry/rfcs/blob/main/text/0086-sentry-bundler-plugins-api.md