Skip to content
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

feat: Add option to set dist on sourcemaps #125

Merged
merged 6 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Adding the following to your workflow will create a new Sentry release and tell
|`ignore_missing`|When the flag is set and the previous release commit was not found in the repository, will create a release with the default commits count instead of failing the command.|`false`|
|`ignore_empty`|When the flag is set, command will not fail and just exit silently if no new commits for a given release have been found.|`false`|
|`sourcemaps`|Space-separated list of paths to JavaScript sourcemaps. Omit to skip uploading sourcemaps.|-|
|`dist`|Unique identifier for the distribution, used to further segment your release. Usually your build number. _Note: Required when uploading sourcemaps._|-|
|`started_at`|Unix timestamp of the release start date. Omit for current time.|-|
|`version`|Identifier that uniquely identifies the releases. _Note: the `refs/tags/` prefix is automatically stripped when `version` is `github.ref`._|<code>${{&nbsp;github.sha&nbsp;}}</code>|
|`version_prefix`|Value prepended to auto-generated version. For example "v".|-|
Expand All @@ -80,6 +81,7 @@ Adding the following to your workflow will create a new Sentry release and tell
with:
environment: 'production'
sourcemaps: './lib'
dist: ${{ github.sha }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tanyagray tanyagray Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screen Shot 2023-02-24 at 12 10 23 PM

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`)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```

- Create a new Sentry release for the `production` environment of your project at version `v1.0.1`.
Expand Down
16 changes: 16 additions & 0 deletions __tests__/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as path from 'path';
import * as process from 'process';
import {
getBooleanOption,
getDist,
getSourcemaps,
getStartedAt,
getVersion,
Expand Down Expand Up @@ -66,6 +67,21 @@ describe('options', () => {
});
});

describe('getDist', () => {
afterEach(() => {
delete process.env['INPUT_DIST'];
});

test('should return undefined when dist is omitted', async () => {
expect(getDist()).toBeUndefined();
});
tanyagray marked this conversation as resolved.
Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(getDist()).toEqual('foo-dist');
});
});

describe('getStartedAt', () => {
const errorMessage =
'started_at not in valid format. Unix timestamp or ISO 8601 date expected';
Expand Down
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ inputs:
sourcemaps:
description: 'Space-separated list of paths to JavaScript sourcemaps. Omit to skip uploading sourcemaps.'
required: false
dist:
description: 'Unique identifier for the distribution, used to further segment your release. Usually your build number.'
required: false
finalize:
description: 'When false, omit marking the release as finalized and released.'
default: true
Expand Down
2 changes: 2 additions & 0 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import * as process from 'process';

const environment = options.getEnvironment();
const sourcemaps = options.getSourcemaps();
const dist = options.getDist();
Copy link
Contributor Author

@tanyagray tanyagray Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const shouldFinalize = options.getBooleanOption('finalize', true);
const ignoreMissing = options.getBooleanOption('ignore_missing', false);
const ignoreEmpty = options.getBooleanOption('ignore_empty', false);
Expand Down Expand Up @@ -52,6 +53,7 @@ import * as process from 'process';
const sourceMapOptions = {
include: sourcemaps,
projects: localProjects,
dist,
Copy link
Contributor Author

@tanyagray tanyagray Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

urlPrefix,
stripCommonPrefix,
};
Expand Down
13 changes: 13 additions & 0 deletions src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,19 @@ export const getSourcemaps = (): string[] => {
return sourcemapsOption.split(' ');
};

/**
* Dist is optional, but should be a string when provided.
* @returns string
*/
export const getDist = (): string | undefined => {
tanyagray marked this conversation as resolved.
Show resolved Hide resolved
const distOption: string = core.getInput('dist');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!distOption) {
return undefined;
tanyagray marked this conversation as resolved.
Show resolved Hide resolved
}

return distOption;
};

/**
* Fetch boolean option from input. Throws error if option value is not a boolean.
* @param input string
Expand Down