-
Notifications
You must be signed in to change notification settings - Fork 81
fix: environment variable build context filtering fix #5887
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
initial commit Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
fixed issue where build was not injecting env variables that were not dev or all context Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com>
tlane25
left a comment
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.
lgtm
DanielSLew
left a comment
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.
Nice catch! Looks good to me
Thanks! Do you know why the circleCi tests are failing? |
lukasholzer
left a comment
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.
Looks good to me! Great catch! 🚀
|
@wconrad265 if you look at the logs for the failing test you can see your change introduced a discrepancy in one of the tests |
Thanks, I will look into why those tests are failing. I think we need to update the tests, since they were always returning the dev environment variables. Also, the test I was looking into was this, before the other tests had run. |
Co-authored-by: Thomas Lane <163203257+tlane25@users.noreply.github.com> Co-authored-by: Thomas Lane <tlane25@users.noreply.github.com>
…/build into build-env-context-fix
|
Updated the test that were failed with @tlane25. Previously, the DEV variable was always returned, since the |
|





🎉 Thanks for submitting a pull request! 🎉
Summary
The pull request addresses the issue below.
The
getEnvfunction within the filepackages/config/src/env/envelope.tswas filtering out env variables that did not belong to theallordevcontexts.More information on how to replicate and how we isolated the issue is below.
Understanding and Isolating the Problem
In order to replicate the issue, we did the following steps.
npm installto install everything needednetlify sites:createto create a new site and link the repo to itVITE_All_ContextThis env variable has the same value in all contexts.VITE_Hellothisenvvariable has different values depending on the contextenvvariables within the main react component and ran the following commandsenvvariables have to start withVITEin order for vite to recognize themNext, we ran a series of
deploycommands to see the resultNext we created second environment variable declared for
allcontexts to understand how varaibles declared withinallcontexts were being handled across different values passed to the--contextflag.command
netlify deploy --build --prod --contextNext we examined the handling of environment variables in
netlify devnetlity devWe dug further to isolate the problem by running
netlify buildnetlify build context --devAs we can see below the value of the
envvariable when using thedevcontext is the correct value.The
VITE_All_Contextvariable is the correct valuenetlify build context --productionAs we can see below, the value of the
VITE_Hellowhen using the production context isvoid, howeverAll_Contextenvvariable is showing the correct valuethe context of
--branch-deployanddeploy-previeware the same as above. This led us to believe the issue is in thenetlify buildcommandNote this is only an issue when deploying from the terminal with the above commands. When deploying from the website, the environment variables were injected correctly.
Solution
The
netlify buildcommand gathers environment variables by runninggetEnv().When running this function, if
siteInfo.use_envelopeis a truthy value, it will invokegetEnvelope. We isolated this function as the source of env variable loss. Environment variables not belonging to thedevorallcontexts were filtered out.We updated the
getEnvelopeto filter byallandcontext(the argument passed by user associated with the —context flag) and reran our isolated tests with the vite project above.Results:
netlify deploy --build --prodAlso
---siteIdwas not passed in the functiongetEnvelope(), so we adjusted the function to make sure it was passed in.For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)