-
Notifications
You must be signed in to change notification settings - Fork 57
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
Changes from 2 commits
067fe64
707e83c
ec7fe19
c9633a0
54f26f3
4d6a716
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import * as path from 'path'; | |
import * as process from 'process'; | ||
import { | ||
getBooleanOption, | ||
getDist, | ||
getSourcemaps, | ||
getStartedAt, | ||
getVersion, | ||
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the github action runner creates all inputs as env vars, prefixed with
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 |
||
expect(getDist()).toEqual('foo-dist'); | ||
}); | ||
}); | ||
|
||
describe('getStartedAt', () => { | ||
const errorMessage = | ||
'started_at not in valid format. Unix timestamp or ISO 8601 date expected'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import * as process from 'process'; | |
|
||
const environment = options.getEnvironment(); | ||
const sourcemaps = options.getSourcemaps(); | ||
const dist = options.getDist(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is where the env var 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); | ||
|
@@ -52,6 +53,7 @@ import * as process from 'process'; | |
const sourceMapOptions = { | ||
include: sourcemaps, | ||
projects: localProjects, | ||
dist, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is where This is the most important line, this is primarily what is missing right now. You need to pass |
||
urlPrefix, | ||
stripCommonPrefix, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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 | ||
|
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.
Does the
github.sha
not get figured out automatically? I thought the usage ofdist
is more for when we do not wantgithub.sha
to be used.I do not know the code enough but that's what I remember.
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.
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](https://user-images.githubusercontent.com/153077/221052071-734df804-514b-4eb3-a9ea-4298e2a26631.png)
Note the "none" on the artifacts. This should be the
dist
and sourcemaps won't work correctly without thedist
value matching on both sourcemaps and the code/application at runtime (via eg.Sentry.init({ dist })
in JS`)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.
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 whatdist
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.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.
added screenshots to the main issue description too for before/after