fix: Only pass urlPrefix to sentry-cli if it's not empty #275
fix: Only pass urlPrefix to sentry-cli if it's not empty #275andreiborza merged 3 commits intomasterfrom
urlPrefix to sentry-cli if it's not empty #275Conversation
urlPrefix to sentry-cli if it's not empty
48ae124 to
da1a031
Compare
urlPrefix to sentry-cli if it's not empty urlPrefix to sentry-cli if it's not empty
| INPUT_DISABLE_TELEMETRY: ${{ inputs.disable_telemetry }} | ||
| INPUT_DISABLE_SAFE_DIRECTORY: ${{ inputs.disable_safe_directory }} | ||
| uses: docker://ghcr.io/getsentry/action-release-image:master | ||
| uses: docker://ghcr.io/getsentry/action-release-image:ab-avoid-sending-empty-urlPrefix |
There was a problem hiding this comment.
l: I guess this will be reverted before merging?
There was a problem hiding this comment.
+1, seems like this should not get merged
There was a problem hiding this comment.
There's a github action that will revert it on master after merging.
There was a problem hiding this comment.
ah okay, makes sense. Thank you for clarifying!
src/main.ts
Outdated
| return getCLI().uploadSourceMaps(release, sourceMapOptions); | ||
| }) | ||
| ); | ||
| const sourceMapsOptions: SentryCliUploadSourceMapsOptions & { |
There was a problem hiding this comment.
that's a nice change! I guess this improves upload speeds a bit in multi-project scenarios (?)
There was a problem hiding this comment.
I'd guess it does, not sure by how much tho but there are over 200 orgs that use multi-project scenarios so hopefully they'll see some kind of impact.
There was a problem hiding this comment.
Actually, turns out this does not work. It only uploads source maps for the first project supplied :(
I changed it back.
szokeasaurusrex
left a comment
There was a problem hiding this comment.
sentry-cli sourcemaps upload indeed only supports uploading one project at a time. I think it is possible to pass multiple projects but all except the first one are ignored. I realize this is a bad user experience (it was already this way before I took over maintaining Sentry CLI), and I would like to add support for uploading multiple projects at once, but for now, the action needs to only pass one project at a time.
| INPUT_DISABLE_TELEMETRY: ${{ inputs.disable_telemetry }} | ||
| INPUT_DISABLE_SAFE_DIRECTORY: ${{ inputs.disable_safe_directory }} | ||
| uses: docker://ghcr.io/getsentry/action-release-image:master | ||
| uses: docker://ghcr.io/getsentry/action-release-image:ab-avoid-sending-empty-urlPrefix |
There was a problem hiding this comment.
+1, seems like this should not get merged
src/main.ts
Outdated
| return getCLI().uploadSourceMaps(release, sourceMapOptions); | ||
| }) | ||
| ); | ||
| const sourceMapsOptions: SentryCliUploadSourceMapsOptions & { |
There was a problem hiding this comment.
Just wondering since I don't know much about typescript: what does this & do here?
There was a problem hiding this comment.
It widens the type to also include projects as a key. SentryCliUploadSourceMapsOptions does not have a projects key, I assumed that's because it's not really an upload option but rather an option to sentry-cli itself so I just widen here.
| core.debug(`Adding sourcemaps`); | ||
| await Promise.all( | ||
| projects.map(async project => { | ||
| // upload source maps can only do one project at a time |
There was a problem hiding this comment.
Thanks, will revert.
I had a look at both projects and saw that both had 4 artifacts uploaded, but upon clicking the artifacts link only one of them actually had source maps uploaded.
Project that has the uploads: https://sentry-sdks.sentry.io/explore/releases/3da6213c7f9ebf252ae055dea015b9cae688f1c9/?notification_uuid=6b722c0d-04be-4cf1-bd24-9fbd19ef7c13&project=4508602366230528
Project that shows it has 4 artifacts but when clicking on them no artifacts are to be found: https://sentry-sdks.sentry.io/explore/releases/3da6213c7f9ebf252ae055dea015b9cae688f1c9/?notification_uuid=6b722c0d-04be-4cf1-bd24-9fbd19ef7c13&project=4505545558327296
szokeasaurusrex
left a comment
There was a problem hiding this comment.
@andreiborza Could you please remove da1a031 from this PR? Now that there are multiple relevant commits, it's quite difficult to review this PR with da1a031 included here
|
@szokeasaurusrex removed and reapplied. Relevant code to review is in 140c6f0 reformatting in 1f07990. |
6b547a6 to
6ec3868
Compare
Also lets sentry-cli handle uploading source maps for multiple projects.
6ec3868 to
1f07990
Compare
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Looks good from CLI perspective, but since I am not super experienced in JS/TS please have a second approval before merging, from @Lms24 or someone else with JS/TS experience.
Please see 511c3f3 for the relevant changes.
I also reworked how projects are passed to the CLI. Previously the CLI was invoked once per project, but I saw that sentry-cli does actually support passing multiple projects so I reworked that.
Here's a screenshot from the report email showing the two projects I specified in a private repo.
Closes: #137