ref(core): Replace release detection with Sentry CLI release detection #87
Conversation
Lms24
force-pushed
the
lms-cli-release
branch
from
503e535 to
b4bfb58
Compare
| authToken: userOptions.authToken, | ||
| url: userOptions.url ?? "https://sentry.io/", | ||
| release: userOptions.release ?? detectRelease(), | ||
| release: userOptions.release ?? "", |
There was a problem hiding this comment.
This seems odd but I want to keep release a required property, hence I'm assigning an empty string.
There was a problem hiding this comment.
Would it hurt us to make it an optional property? If it is actually optional now, we should define it as such. (Same probably down the line for project, authToken etc. because they could be defined via config/env-vars now right?)
There was a problem hiding this comment.
Well, we'd have to check for its definedness everywhere while right now we could be sure that it's defined. For env vars (whenever we bring them back) we can just assign them in our normalization step so we could still leave token, org, project, etc required.
Generally, I'm thinking of moving our validation logic (which def needs to be refactored) into or before the final internal option conversion step so we can error before the plugin does anything. WDYT, does this sound reasonable?
There was a problem hiding this comment.
release is a special case here because we can't easily await the cli.releases.proposeVersion promise in the top-level function we pass to createUnplugin. Hence I moved it to the buildStart hook
There was a problem hiding this comment.
because they could be defined via config/env-vars now right?
@lforst especially the config file is a valid point. Haven't thought about this when making the change here. So this means that, unless we want to read and parse the config file during normalization (which I don't want to do unless necessary), we have to refactor our internal options conversion to actually make these parameters (releaes, authtoken, etc) optional again...
I'm gonna merge this PR to get some of the quick CLI replacements in and then tackle the options again. In the end this will change this line above
| if (!internalOptions.release) { | ||
| internalOptions.release = await cli.releases.proposeVersion(); | ||
| } |
There was a problem hiding this comment.
In case we don't have a release (i.e. an empty string), we use the CLI to detect it automatically
This PR marks the first step of re-adding CLI functionality. It removes our previous release detection code (RIP) and replaces it with CLI functionality. Doing this to ensure feature parity with the CLI, we can revisit this if we have time.
ref #85