-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(gatsby): Don't crash build when auth token is missing #7858
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
Conversation
packages/gatsby/gatsby-node.js
Outdated
if (message.includes('organization slug is required') || message.includes('project slug is required')) { | ||
return; | ||
} | ||
if (message.includes('authentication credentials were not provided')) { | ||
// eslint-disable-next-line no-console | ||
console.warn('Sentry Logger [Warn]: Cannot upload source maps due to missing SENTRY_AUTH_TOKEN env variable.') |
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.
console.warn('Sentry Logger [Warn]: Cannot upload source maps due to missing SENTRY_AUTH_TOKEN env variable.') | |
console.warn('Sentry [Warn]: Cannot upload source maps due to missing SENTRY_AUTH_TOKEN env variable.') |
logaf giga l
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.
Fair, a bit further below, we had Sentry Logger []
so I just duplicated it but we can also remove it in both places
@@ -46,10 +46,15 @@ exports.onCreateWebpackConfig = ({ plugins, getConfig, actions }) => { | |||
// Handle sentry-cli configuration errors when the user has not done it not to break | |||
// the build. | |||
errorHandler(err, invokeErr) { | |||
const { message } = err; | |||
const message = err.message && err.message.toLowerCase() || ''; | |||
if (message.includes('organization slug is required') || message.includes('project slug is required')) { |
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.
l: Do we maybe wanna add a similar error message right here as a quick win?
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.
As I wrote in the PR, there's an argument to be made that if these env vars are not specified, users don't want to upload source maps and hence there should be no warning. But let's just display a "info" level message here that we're not gonna upload source maps in case these variables aren't set.
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.
Ah sorry! I skipped over that. I think printing a message is good here since having source maps is such a productivity win - I think we wanna tell users about it :D
Our Gatsby SDK ships with the webpack plugin to automatically upload source maps (TIL).
When building for production, the webpack causes a crash if no auth token is specified. We decided that builds shouldn't crash because of a misconfiguration of our plugin, so this PR fixes that (see #7427 and #7846). Instead, we'll log a warning in this case.
Note: I opted to not log a warning if ORG or PROJECT were not specified because in this case one could argue that users don't want to upload at all. This is identical to the previous behaviour. Happy to change that though, if reviewers think we should notify users anyway.
closes #7852