fix: wrong env values could be used for non-dev context in netlify deploy --build --context
#5538
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasonbarry
commented
Mar 8, 2023
| @@ -145,6 +146,18 @@ export const getEnvelopeEnv = async ({ api, context = 'dev', env, key = '', scop | |||
|
|
|||
| const accountEnv = formatEnvelopeData({ context, envelopeItems: accountEnvelopeItems, scope, source: 'account' }) | |||
| 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
|
eduardoboucas
approved these changes
Mar 9, 2023
| @@ -567,6 +568,16 @@ const deploy = async (options, command) => { | |||
| return triggerDeploy({ api, options, siteData, siteId }) | |||
| } | |||
|
|
|||
| 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?
Suggested change
| 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!
| @@ -145,6 +146,18 @@ export const getEnvelopeEnv = async ({ api, context = 'dev', env, key = '', scop | |||
|
|
|||
| const accountEnv = formatEnvelopeData({ context, envelopeItems: accountEnvelopeItems, scope, source: 'account' }) | |||
| 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)