build(ci): Add/fix webpack instrumentation for GHA#22034
Conversation
This updates our webpack instrumentation so that it works on GHA
| CI, | ||
| } = process.env; | ||
| const IS_CI = !!TRAVIS_COMMIT; | ||
| const IS_CI = !!CI; |
There was a problem hiding this comment.
No. Please use a more specific identifier to do this rather than just checking CI. See #21415 and related bug for more context around this.
There was a problem hiding this comment.
@BYK FWIW it's only enabled when SENTRY_INSTRUMENTATION is configured: https://github.com/getsentry/sentry/pull/22034/files#diff-bb490b7bd0a004bce840fd8ce6b94911de263f1eb455491f30596ea4a0aee48eR29
IS_CI is used for Sentry tags to see if we are in a CI env.
There was a problem hiding this comment.
Sure but I've got burned by this before. Maybe we should rename it or something?
| CI, | ||
| } = process.env; | ||
| const IS_CI = !!CI; | ||
| const IS_CI = !!GITHUB_SHA; |
There was a problem hiding this comment.
Shouldn't this be GITHUB_REF as the connection is through this env var below. Also I wonder if we really need IS_CI? Why not just check GITHUB_REF directly below?
There was a problem hiding this comment.
We use IS_CI elsewhere, https://github.com/getsentry/sentry/pull/22034/files#diff-bb490b7bd0a004bce840fd8ce6b94911de263f1eb455491f30596ea4a0aee48eR76
GITHUB_REFis not guaranteed to exist and 2) we currently only useGITHUB_REF, but we could add other Sentry tags later
There was a problem hiding this comment.
Eh, okay. Then that usage becomes GHA-only though (won't detect GCB as "ci env" correctly if we wanted in the future).
I think this is fine right now, but I feel like there's a need to review and refactor all CI and IS_CI usages to make them accurate.
There was a problem hiding this comment.
Right, that's why it was using CI before
| CI, | ||
| } = process.env; | ||
| const IS_CI = !!CI; | ||
| const IS_CI = !!GITHUB_SHA; |
There was a problem hiding this comment.
Eh, okay. Then that usage becomes GHA-only though (won't detect GCB as "ci env" correctly if we wanted in the future).
I think this is fine right now, but I feel like there's a need to review and refactor all CI and IS_CI usages to make them accurate.
This updates our webpack instrumentation so that it works on GHA
4 participants
This updates our webpack instrumentation so that it works on GHA