feat: Add option to set `dist` on sourcemaps #125

tanyagray · 2023-02-17T05:19:53Z

Adds support for dist as input value for GitHub Action:

    - uses: getsentry/action-release@v1
      with:
        environment: 'production'
        sourcemaps: './lib'
        dist: ${{ github.sha }}

Note that this repo uses @sentry/cli@^1.67.2 and there is a related fix in 2.3.1:

fix: move dist option to SentryCliUploadSourceMapsOptions (#1269) by @ikenfin
changelog

The above sentry-cli fix was to move the dist option from being under SentryCliOptions (incorrect) to be under SentryCliUploadSourceMapsOptions (correct)

This does not affect the GH action as those types are not referenced directly by the action.

Have not added tests for the value being passed to the CLI as that doesn't appear to be covered for any of the existing values. Did manually debug and ensure that the resulting execute call included the --dist fake-dist param.

Before: Sentry UI showing dist 'none' for sourcemap artifacts

After: Sentry UI showing SHA as the dist for sourcemaps (as passed to action):

Resolves #74

@ikenfin

Adds support for `dist` as input value for GitHub Action: ```yaml - uses: getsentry/action-release@v1 with: environment: 'production' sourcemaps: './lib' dist: ${{ github.sha }} ``` Note that this repo uses `@sentry/cli@^1.67.2` and there is a related fix in `2.3.1`: > fix: move dist option to SentryCliUploadSourceMapsOptions (#1269) by @ikenfin > [changelog](https://github.com/getsentry/sentry-cli/blob/master/CHANGELOG.md#231) This fix was to move the `dist` option from being under `SentryCliOptions` (incorrect) to be under `SentryCliUploadSourceMapsOptions` (correct) This does not affect the GH action as those types are not referenced directly by the action. Resolves getsentry#74

armenzg · 2023-02-22T20:26:14Z

__tests__/main.test.ts

+    });
+
+    test('should return a string when dist is provided', () => {
+      process.env['INPUT_DIST'] = 'foo-dist';


Why the usage of this env? I don't see it used in the code.

the github action runner creates all inputs as env vars, prefixed with INPUT_, so INPUT_DIST is equivalent to ${ inputs.dist } in the action itself, or with: dist: 'foo-dist' via a workflow.

When you specify an input in a workflow file or use a default input value, GitHub creates an environment variable for the input with the name INPUT_<VARIABLE_NAME>. The environment variable created converts input names to uppercase letters and replaces spaces with _ characters.
Source

This is how the Sentry Action tests are implemented, as it's (presumably) easier to unit test by setting env vars, than by running the workflow with different sets of with: params. I just followed the format used for the existing tests rather than implementing any new testing approach.

armenzg · 2023-02-22T20:29:17Z

README.md

@@ -79,6 +80,7 @@ Adding the following to your workflow will create a new Sentry release and tell
      with:
        environment: 'production'
        sourcemaps: './lib'
+        dist: ${{ github.sha }}


Does the github.sha not get figured out automatically? I thought the usage of dist is more for when we do not want github.sha to be used.

I do not know the code enough but that's what I remember.

oooh this is interesting... I wonder if this is a bug on its own then.

When using the github action as it is currently, dist is not set at all and shows as "none" in the Sentry UI, which causes source maps not to match up at all.

Without this change I've made in this PR, this is what I see in the Sentry UI when using the GH action to upload:

Note the "none" on the artifacts. This should be the dist and sourcemaps won't work correctly without the dist value matching on both sourcemaps and the code/application at runtime (via eg. Sentry.init({ dist }) in JS`)

If it's reasonable for dist to default to the github SHA, i could likely update the PR to do that. There isn't much in the way of docs about what dist should be, other than a suggestion it might be your build number (?) but that doesnt seem correct for use in GH workflows -- SHA seems more reasonable.

added screenshots to the main issue description too for before/after

tanyagray · 2023-02-23T23:00:36Z

src/main.ts

@@ -11,6 +11,7 @@ import * as options from './options';

    const environment = options.getEnvironment();
    const sourcemaps = options.getSourcemaps();
+    const dist = options.getDist();


this is where the env var INPUT_DIST is used, options.getDist() uses the github core.getInput('dist') to get the input.

Added a comment on that line too.

tanyagray · 2023-02-23T23:01:48Z

src/main.ts

@@ -45,6 +46,7 @@ import * as options from './options';
          const sourceMapOptions = {
            include: sourcemaps,
            projects: localProjects,
+            dist,


this is where $INPUT_DIST / ${ inputs.dist } / with: dist: 'foo-dist' is passed to the sentry sourcemap upload command.

This is the most important line, this is primarily what is missing right now. You need to pass dist in the sourcemap options for it to be included in the execution of the cli.uploadSourceMaps() function (line 53 below)

tanyagray · 2023-02-23T23:02:45Z

src/options.ts

+ * @returns string
+ */
+export const getDist = (): string | undefined => {
+  const distOption: string = core.getInput('dist');


core.getInput('dist') gets that env var $INPUT_DIST that GH creates on behalf of each input.

tanyagray · 2023-02-23T23:31:26Z

@armenzg have left a bunch more detailed comments and screenshots to help with the context-switching :)

src/options.ts

__tests__/main.test.ts

armenzg

This is fantastic work! Thank you for putting the extra effort to make comments on your code and putting the screenshots in place!

armenzg · 2023-02-24T14:10:13Z

It seems that the step ncc build src/main.ts fails to work. Do you know what it is about?
https://github.com/getsentry/action-release/actions/runs/4257979812/jobs/7419509851#step:4:56

hermitpopcorn · 2023-03-08T08:09:22Z

Maybe because the change made in ec7fe19 was not reflected in the function's return types? @tanyagray

action-release/src/options.ts

Line 88 in ec7fe19

export const getDist = (): string | undefined => {

Amphaal · 2023-03-28T08:12:01Z

Bumping, as this feature is now kinda required for the most recent versions of Sentry since sourcemaps automatic mappings depends on it 😢

tanyagray · 2023-03-28T08:31:28Z

Bumping, as this feature is now kinda required for the most recent versions of Sentry since sourcemaps automatic mappings depends on it 😢

This must be why I had trouble with it. I was very confused why I was following the docs and it still wasn't working.

I ended up abandoning using the github action and writing my own workflow using the CLI directly, but will try make some time tomorrow to make the change suggested by @hermitpopcorn, thanks for that!

Cheers for the bump 🙏

Amphaal · 2023-03-28T09:06:10Z

Bumping, as this feature is now kinda required for the most recent versions of Sentry since sourcemaps automatic mappings depends on it 😢

This must be why I had trouble with it. I was very confused why I was following the docs and it still wasn't working.

I ended up abandoning using the github action and writing my own workflow using the CLI directly, but will try make some time tomorrow to make the change suggested by @hermitpopcorn, thanks for that!

Cheers for the bump 🙏

So, I was also on my way to do exactly the same thing as you did...

I stumbled on https://docs.sentry.io/platforms/javascript/sourcemaps/troubleshooting_js/, which points to a very handy
(sentry-cli sourcemaps explain), which I tried on an error I produced after uploading the artifacts:

...Which indicates that the folder binding that this Action interprets from with.sourcemaps is not understood by Sentry !

So, I tried playing around with with.sourcemaps, testing both './build/web' and build/web, which resulted in incorrect binding in both cases :

Then, I replaced this action by a bulk command to test the naive behavior of the latest sentry-cli in my ./.github/workflows/deploy_web.yml:

      - name: "Sentry: Create Release w/ sourcemaps"
        env:
          SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_TOKEN }}
          SENTRY_ORG: qalisa
          SENTRY_PROJECT: ivy-app
          SENTRY_URL: https://sentry.ivy.community/
        run: |
          # Install Sentry CLI
          curl -sL https://sentry.io/get-cli/ | bash

          # Create new Sentry release
          export SENTRY_RELEASE=com.qalisa.ivy@${{ steps.version.outputs.value }}+${{ env.IVY_BUILD_NUMBER }}
          sentry-cli releases new $SENTRY_RELEASE
          sentry-cli releases set-commits --auto $SENTRY_RELEASE
          sentry-cli releases files $SENTRY_RELEASE upload-sourcemaps build/web
          sentry-cli releases finalize $SENTRY_RELEASE

Which produced correct outputs :

My final guess is that, either the outdated sentry-cli of this action does not have the user expected behavior, or with.sourcemaps enforces a prefixed /.
In both cases, for now, we cannot use this action since it would require --url-prefix handling, which is not implemented right now... 😭

In hope this lengthy response helps someone ! 👍

Amphaal · 2023-03-28T09:27:23Z

Bumping, as this feature is now kinda required for the most recent versions of Sentry since sourcemaps automatic mappings depends on it 😢

I can confirm that, if only the build number changes between release publications via sentry-cli, --dist is required, and must match the build number 👍 So this PR is indeed still relevant for this specific use-case.

armenzg · 2023-03-28T13:27:37Z

src/options.ts

+export const getDist = (): string | undefined => {
+  const distOption: string = core.getInput('dist');
+  if (!distOption) {
+    return null;


I'm seeing this error in the CI logs.

TS2322: Type 'null' is not assignable to type 'string | undefined'.

Would returning undefined work? or should the return type be changed?

i think this was my mistake, i originally has this function configured to return undefined, but changed it to null after seeing all the other properties use null not undefined... I just didn't update the return type.

vscode corrected me as soon as i opened the PR, not sure why it didn't point it out earlier.

hopefully this is enough for a CI pass 🤞

can see some of the sentry-cli integration tests use null so I expect this is correct.

made this change and committed 👍

src/options.ts

…fined`

armenzg · 2023-04-04T16:58:17Z

Thank you @tanyagray for your perseverance and willingness to help everyone! 🎉

benasher44 · 2023-04-18T16:09:54Z

Thanks for the fix! Can this please be added to the v1 tag? Our sourcemaps/traces are currently broken, and this should fix it. I went to go add the dist arg, but then I found that it wasn't accepted yet.

armenzg · 2023-04-19T16:28:59Z

Hi @benasher44
I believe I've handled it here:
#145

benasher44 · 2023-04-19T19:43:48Z

Amazing, thank you!

In #125, we added support for using `dist` with sourcemaps, however, we return `null` when not specified and that gets converted into the string `"null"` down the pipeline which has broken the sourcemaps for various customers in the last two weeks. Also, ss indicated in #138, `dist` is optional for sourcemaps when used with the CLI. Updating the README.me to handle it. This also removes a warning which shows when the action is run: ``` Warning: Unexpected input(s) 'dist', valid inputs are ['entryPoint', 'args', 'environment', 'sourcemaps', 'finalize', 'ignore_missing', 'ignore_empty', 'started_at', 'version', 'version_prefix', 'set_commits', 'projects', 'url_prefix', 'strip_common_prefix', 'working_directory'] ```

tanyagray requested a review from a team February 17, 2023 05:19

tanyagray force-pushed the feat/74-sourcemaps-dist branch from 9c18acc to 067fe64 Compare February 17, 2023 05:21

armenzg reviewed Feb 22, 2023

View reviewed changes

tanyagray commented Feb 23, 2023

View reviewed changes

tanyagray requested a review from armenzg February 23, 2023 23:30

Merge branch 'master' into feat/74-sourcemaps-dist

707e83c

tanyagray commented Feb 23, 2023

View reviewed changes

src/options.ts Outdated Show resolved Hide resolved

return null when dist has not been provided

ec7fe19

tanyagray commented Feb 24, 2023

View reviewed changes

__tests__/main.test.ts Outdated Show resolved Hide resolved

update dist input tests to expect null when dist is not provided

c9633a0

armenzg approved these changes Feb 24, 2023

View reviewed changes

Merge branch 'master' into feat/74-sourcemaps-dist

54f26f3

armenzg reviewed Mar 28, 2023

View reviewed changes

tanyagray commented Apr 3, 2023

View reviewed changes

src/options.ts Outdated Show resolved Hide resolved

fix: Alternative return type for getDist should be null not `unde…

4d6a716

…fined`

armenzg merged commit 455699b into getsentry:master Apr 4, 2023

rodolfoBee mentioned this pull request Apr 13, 2023

GHA actions are uploading source maps with dist="null" (string) #140

Closed

mitsuhiko mentioned this pull request Apr 13, 2023

feat(js): Skip null as option same as undefined getsentry/sentry-cli#1579

Merged

armenzg mentioned this pull request Apr 20, 2023

fix: dist option should be undefined if not specified #148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add option to set `dist` on sourcemaps #125

feat: Add option to set `dist` on sourcemaps #125

tanyagray commented Feb 17, 2023 •

edited

Loading

armenzg Feb 22, 2023

tanyagray Feb 23, 2023

armenzg Feb 22, 2023

tanyagray Feb 23, 2023 •

edited

Loading

tanyagray Feb 23, 2023

tanyagray Feb 23, 2023

tanyagray Feb 23, 2023 •

edited

Loading

tanyagray Feb 23, 2023 •

edited

Loading

tanyagray Feb 23, 2023

tanyagray commented Feb 23, 2023 •

edited

Loading

armenzg left a comment

armenzg commented Feb 24, 2023

hermitpopcorn commented Mar 8, 2023

Amphaal commented Mar 28, 2023

tanyagray commented Mar 28, 2023

Amphaal commented Mar 28, 2023

Amphaal commented Mar 28, 2023

armenzg Mar 28, 2023 •

edited

Loading

tanyagray Apr 3, 2023 •

edited

Loading

tanyagray Apr 3, 2023

tanyagray Apr 3, 2023

armenzg commented Apr 4, 2023

benasher44 commented Apr 18, 2023

armenzg commented Apr 19, 2023

benasher44 commented Apr 19, 2023

feat: Add option to set dist on sourcemaps #125

feat: Add option to set dist on sourcemaps #125

Conversation

tanyagray commented Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanyagray Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanyagray Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

tanyagray Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanyagray commented Feb 23, 2023 • edited Loading

armenzg left a comment

Choose a reason for hiding this comment

armenzg commented Feb 24, 2023

hermitpopcorn commented Mar 8, 2023

Amphaal commented Mar 28, 2023

tanyagray commented Mar 28, 2023

Amphaal commented Mar 28, 2023

Amphaal commented Mar 28, 2023

armenzg Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

tanyagray Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armenzg commented Apr 4, 2023

benasher44 commented Apr 18, 2023

armenzg commented Apr 19, 2023

benasher44 commented Apr 19, 2023

feat: Add option to set `dist` on sourcemaps #125

feat: Add option to set `dist` on sourcemaps #125

tanyagray commented Feb 17, 2023 •

edited

Loading

tanyagray Feb 23, 2023 •

edited

Loading

tanyagray Feb 23, 2023 •

edited

Loading

tanyagray Feb 23, 2023 •

edited

Loading

tanyagray commented Feb 23, 2023 •

edited

Loading

armenzg Mar 28, 2023 •

edited

Loading

tanyagray Apr 3, 2023 •

edited

Loading