build icon indicating copy to clipboard operation
build copied to clipboard

fix: environment variable build context filtering fix

Open wconrad265 opened this issue 1 year ago • 2 comments

🎉 Thanks for submitting a pull request! 🎉

Summary

The pull request addresses the issue below.

  • https://answers.netlify.com/t/build-step-injects-env-vars-only-when-using-same-value-for-all-contexts-since-today/122796

The getEnv function within the file packages/config/src/env/envelope.ts was filtering out env variables that did not belong to the all or dev 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.

  1. We created a simple vite react app, you can find the app for this test. here.
    1. https://github.com/wconrad265/env-test
    2. ran npm install to install everything needed
  2. ran netlify sites:create to create a new site and link the repo to it
  3. We created two different env variables, on the netlify website.
    1. VITE_All_Context This env variable has the same value in all contexts. image

    2. VITE_Hello this env variable has different values depending on the context image

  4. We placed the env variables within the main react component and ran the following commands
    1. env variables have to start with VITE in order for vite to recognize them

Next, we ran a series of deploy commands to see the result

Next we created second environment variable declared for all contexts to understand how varaibles declared within all contexts were being handled across different values passed to the --context flag.

command netlify deploy --build --prod --context

VITE_Hello VITE_All_Context —build —prod —context
no yes x x n/a
yes yes x x dev
no yes x x production
no yes x x deploy-preview
no yes x x branch-deploy

Next we examined the handling of environment variables in netlify dev

netlity dev

VITE_Hello VITE_All_Context —context
yes yes n/a
yes yes dev
yes yes production
yes yes deploy-preview
yes yes branch-deploy

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 the dev context is the correct value.

The VITE_All_Contextvariable is the correct value

image

netlify build context --production

As we can see below, the value of the VITE_Hello when using the production context is void, however All_Context env variable is showing the correct value

image

the context of --branch-deploy and deploy-preview are the same as above. This led us to believe the issue is in the netlify build command

Note 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 running getEnv().

When running this function, if siteInfo.use_envelope is a truthy value, it will invoke getEnvelope . We isolated this function as the source of env variable loss. Environment variables not belonging to the dev or all contexts were filtered out.

We updated the getEnvelopeto filter by all and context (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

VITE_Hello VITE_All_Context —build —prod —context
yes yes x x n/a
yes yes x x dev
yes yes x x production
yes yes x x deploy-preview
yes yes x x branch-deploy

Also siteId was not passed in the function getEnvelope(), 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:

  • [x] Open a bug/issue before writing your code 🧑‍💻. This ensures 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.
  • [x] Read the contribution guidelines 📖. This ensures your code follows our style guide and passes our tests.
  • [x] Update or add tests (if any source code was changed or added) 🧪
  • [x] Update or add documentation (if features were changed or added) 📝
  • [x] Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

wconrad265 avatar Oct 18 '24 16:10 wconrad265

Nice catch! Looks good to me

Thanks! Do you know why the circleCi tests are failing?

wconrad265 avatar Oct 19 '24 16:10 wconrad265

@wconrad265 if you look at the logs for the failing test you can see your change introduced a discrepancy in one of the tests image

DanielSLew avatar Oct 21 '24 13:10 DanielSLew

@wconrad265 if you look at the logs for the failing test you can see your change introduced a discrepancy in one of the tests image

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.

https://app.circleci.com/pipelines/github/netlify/build/7538/workflows/b783faf8-d6cd-4a18-a5e9-a5f608458a07/jobs/10768

wconrad265 avatar Oct 21 '24 15:10 wconrad265

Updated the test that were failed with @tlane25.

Previously, the DEV variable was always returned, since the getEnvelope function was always returning the dev environment variables regardless of the context. Because the tests run in the production context, the test has been updated to return the correct corresponding env variable.

wconrad265 avatar Oct 22 '24 22:10 wconrad265