-
Notifications
You must be signed in to change notification settings - Fork 29
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): Use Sentry CLI for artifact deletion #92
Conversation
const span = addSpanToTransaction(ctx, "function.plugin.clean_artifacts"); | ||
|
||
if (options.cleanArtifacts) { | ||
// TODO: pull these checks out of here and simplify them | ||
if (options.authToken === undefined) { | ||
ctx.logger.warn('Missing "authToken" option. Will not clean existing artifacts.'); | ||
return Promise.resolve("nothing to do here"); | ||
return; |
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: really feels like this should be abstracted out - perhaps into an array that gets iterated on (we can then even validate this with types)
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.
Yup, agreed, this is terrible right now. We have the same block in many of the release creation steps. My plan was to move these to one validation step when converting user-facing to internal options. So ideally, in this release pipeline, we don't have to do any validation anymore. I'll leave this as-is for now and add a point to #91
sentryHub: ctx.hub, | ||
customHeader: options.customHeader, | ||
}); | ||
await ctx.cli.releases.execute(["releases", "files", options.release, "delete", "--all"], true); |
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.
surprised this doesn't have a return value
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.
In this particular command, it doesn't seem to return anything:
https://github.com/getsentry/sentry-cli/blob/668d142c1a808f1e695ca11a22663014e4a5d2c0/js/helper.js#L180
Replace previous custom implementation with Sentry CLI logic analogously to the webpack plugin
ref: #85