fix: wrong env values could be used for non-dev context in netlify deploy --build --context
#5538
Conversation
| const siteEnv = formatEnvelopeData({ context, envelopeItems: siteEnvelopeItems, scope, source: 'ui' }) | ||
|
|
||
| if (raw) { | ||
| const entries = Object.entries({ ...accountEnv, ...siteEnv }) |
There was a problem hiding this comment.
Is it correct that only account and site env vars, and not general / addons / configFile env vars, need to be included as siteEnv for the functionsConfig? That's how it seems to be before -- only get(siteData, 'build_settings.env') was getting passed to functionsConfig.
There was a problem hiding this comment.
Yes, I think that's right.
There was a problem hiding this comment.
OK cool! then I think this would fix a bug where account-level env vars weren't getting applied to functions if a user deployed with this method.
📊 Benchmark resultsComparing with c4b15f6 Package size: 263 MB(no change) Legend
|
|
|
||
| const isUsingEnvelope = siteData && siteData.use_envelope | ||
| // if a context is passed besides dev, we need to pull env vars from that specific context | ||
| if (isUsingEnvelope && options.context && options.context !== 'dev') { |
There was a problem hiding this comment.
I think this additional check is unnecessary?
| if (isUsingEnvelope && options.context && options.context !== 'dev') { | |
| if (isUsingEnvelope && options.context !== 'dev') { |
There was a problem hiding this comment.
options.context could be undefined if the user doesn't pass the --context flag, but the Envelope call defaults to use the dev context in that case. So if context isn't passed, or if context is dev, then we can skip the call. We could remove this bit if we gave the option a default value besides undefined, but I'm unsure if that would be a breaking change!
| const siteEnv = formatEnvelopeData({ context, envelopeItems: siteEnvelopeItems, scope, source: 'ui' }) | ||
|
|
||
| if (raw) { | ||
| const entries = Object.entries({ ...accountEnv, ...siteEnv }) |
There was a problem hiding this comment.
Yes, I think that's right.
Summary
If a site has environment variables with different values for each deploy context, and they deploy to prod using
netlify deploy --build --prod --context production, then the dev values will be used instead. This is becauseoptions.contextwasn't making its way to the Envelope call (unlike how we do it innetlify build,netlify dev, andnetlify env). This PR passes the context from the--contextflag over to Envelope so the correct values are pulled.In doing so, I noticed that env vars from
build_settings.envwere getting included in the functionsConfig. While these values are the site's env vars from Envelope, they are the ones that apply to any scope and any context. Instead, we should pass in only the ones that match the given--context, and only thefunctionsscope.Customer report Slack thread: https://netlify.slack.com/archives/C02TE7L1QUA/p1678283162339439
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)