feat(vercel-integration): Create environment variables for all Vercel deployment environments #47441
Conversation
… deployment environments
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #47441 +/- ##
===========================================
+ Coverage 62.70% 80.68% +17.98%
===========================================
Files 4724 4760 +36
Lines 199971 201409 +1438
Branches 11629 11629
===========================================
+ Hits 125389 162506 +37117
+ Misses 74324 38645 -35679
Partials 258 258
|
| "key": key, | ||
| "value": value, | ||
| "target": ["production"], | ||
| "target": ["production", "preview", "development"], |
There was a problem hiding this comment.
Would having development work well here? Vercel itself wouldn't be running anything on development, so this would be environment variables that are pulled down for local use.
In localhost, you might run code in files that aren't committed yet. What would happen in terms of releases then?
In my perspective, an error without a source map for JavaScript should not exist. Would this change not make more of those?
There was a problem hiding this comment.
Hm, lemme think about this. When running vercel dev, we are building/running Next.js in development mode where we don't upload source maps anyhow. The errors thrown in dev mode should be dynamically source mapped - we built this in getsentry/sentry-javascript#7294. So I think you have a point here.
BUT, people are not exclusively deploying Next.js apps to Vercel. I think it might be beneficial to have the env vars available if you have added the integration for different kinds of setups. For example, we're currently working on a sveltekit SDK which might make use of the variables.
What do you think?
There was a problem hiding this comment.
There are ways in which it makes a lot of sense. Having the DSN in .env seems like a good thing for example. It makes sense to have more of the data locally for other types of projects I too I guess. My only concern is that one of your env vars is VERCEL_GIT_COMMIT_SHA That wouldn't work locally. Would it be somehow pulled as well? I think it might have to be excluded?
But as long as the default setup from npx @sentry/wizard -s -i nextjs can handle that, and with also when #5734 is fixed, I think it's fine to have them on development as well.
I personally don't use vercel dev, so I don't know what that does.
There was a problem hiding this comment.
My only concern is that one of your env vars is VERCEL_GIT_COMMIT_SHA That wouldn't work locally. Would it be somehow pulled as well? I think it might have to be excluded?
Ah, you're right. Gonna exclude this from the development target. 👍
There was a problem hiding this comment.
Actually not even SENTRY_AUTH_TOKEN is needed locally, and it is probably even dangerous to put it onto users' machines.
There was a problem hiding this comment.
Actually actually no env vars besides the DSN are useful in development
There was a problem hiding this comment.
Not SENTRY_PROJECT or SENTRY_ORG even?
There was a problem hiding this comment.
Not really, they're only used for uploading source maps, which we don't do in dev mode.
There was a problem hiding this comment.
Sounds good then! Thanks for discussing.
We changed the Next.js SDK to upload source maps for every deployment target in getsentry/sentry-javascript#7436. This requires the Vercel integration to follow suite.
Fixes getsentry/sentry-docs#5690
Fixes getsentry/sentry-javascript#5619