-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix: environment variable build context filtering fix #5887
base: main
Are you sure you want to change the base?
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>
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
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? |
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 |
Quality Gate passedIssues Measures |
🎉 Thanks for submitting a pull request! 🎉
Summary
The pull request addresses the issue below.
The
getEnv
function within the filepackages/config/src/env/envelope.ts
was filtering out env variables that did not belong to theall
ordev
contexts.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 install
to install everything needednetlify sites:create
to create a new site and link the repo to itVITE_All_Context
This env variable has the same value in all contexts.VITE_Hello
thisenv
variable has different values depending on the contextenv
variables within the main react component and ran the following commandsenv
variables have to start withVITE
in order for vite to recognize themNext, we ran a series of
deploy
commands to see the resultNext we created second environment variable declared for
all
contexts to understand how varaibles declared withinall
contexts were being handled across different values passed to the--context
flag.command
netlify deploy --build --prod --context
Next we examined the handling of environment variables in
netlify dev
netlity dev
We dug further to isolate the problem by running
netlify build
netlify build context --dev
As we can see below the value of the
env
variable when using thedev
context is the correct value.The
VITE_All_Context
variable is the correct valuenetlify build context --production
As we can see below, the value of the
VITE_Hello
when using the production context isvoid
, howeverAll_Context
env
variable is showing the correct valuethe context of
--branch-deploy
anddeploy-preview
are the same as above. This led us to believe the issue is in thenetlify build
commandNote 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 build
command gathers environment variables by runninggetEnv()
.When running this function, if
siteInfo.use_envelope
is a truthy value, it will invokegetEnvelope
. We isolated this function as the source of env variable loss. Environment variables not belonging to thedev
orall
contexts were filtered out.We updated the
getEnvelope
to filter byall
andcontext
(the argument passed by user associated with the —context flag) and reran our isolated tests with the vite project above.Results:
netlify deploy --build --prod
Also
---siteId
was 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)