Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: inject environment variables from .env files into edge functions locally #5620

Merged
merged 6 commits into from
Apr 17, 2023
Merged

feat: inject environment variables from .env files into edge functions locally #5620

merged 6 commits into from
Apr 17, 2023

Conversation

danez
Copy link
Contributor

@danez danez commented Apr 12, 2023 •
edited
Loading

Summary

This allows any environment variables that are coming from dot-env files (.env) to be accessed in edge functions.

Similar to what is described here: https://docs.netlify.com/configure-builds/environment-variables/#build-locally

For us to review and ship your PR efficiently, please perform the following steps:

  • 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.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

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

@danez danez self-assigned this Apr 12, 2023
@github-actions
Copy link

github-actions bot commented Apr 12, 2023 •
edited
Loading

📊 Benchmark results

Comparing with f30d409

Package size: 302 MB

(no change)

^  302 MB  302 MB  302 MB  302 MB  302 MB  302 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-5     T-4     T-3     T-2     T-1      T
Legend

@danez danez changed the title feat: inject environment variables from .env files into edge function… feat: inject environment variables from .env files into edge functions locally Apr 12, 2023
@danez danez requested a review from klavavej April 12, 2023 14:56
@danez danez marked this pull request as ready for review April 12, 2023 15:00
@danez danez requested a review from a team April 12, 2023 15:00
danez
danez commented Apr 12, 2023
View reviewed changes
src/utils/dev.mjs
* @param {Record<string, { sources: string[], value: string}>} env
* @return {void}
*/
export const injectEnvVariables = (env) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this function to only inject variables into process.env and split out all the dotenv stuff into the function above.

danez
danez commented Apr 12, 2023
View reviewed changes
src/commands/dev/dev.mjs
@@ -96,7 +96,8 @@ const dev = async (options, command) => {
log(`${NETLIFYDEVLOG} Injecting environment variable values for ${chalk.yellow('all scopes')}`)
}

await injectEnvVariables({ devConfig, env, site })
env = await addDotEnvVariables({ devConfig, env, site })
injectEnvVariables(env)
Copy link
Contributor Author

@danez danez Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addDotEnvVariables now mutates env here and adds the variables from the dot-env files to it, so that later one we can use them in edge-functions. Previously dotenv variables were only added to process.env directly.

danez
danez commented Apr 12, 2023
View reviewed changes
tests/integration/100.command.dev.test.cjs Outdated
@@ -1,4 +1,4 @@
// Handlers are meant to be async outside tests
const { platform } = require('os')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure out what this comment means or where it applies to :D

@klavavej
Copy link

@danez - I'm not sure what part of this PR you want me to review. I took a look through and couldn't identify any changes that will impact what's shown in the cli docs at https://deploy-preview-5620--cli.netlify.app/

Could you point me to the part of this PR that needs my review?

I do see that docs.netlify.com documents some limitations around this behavior that will need to be updated/removed (on the Environment variables and functions page and the Edge Functions limits page)- I can open a PR to get the ball rolling on those changes.

@danez
Copy link
Contributor Author

danez commented Apr 12, 2023

@danez - I'm not sure what part of this PR you want me to review. I took a look through and couldn't identify any changes that will impact what's shown in the cli docs at https://deploy-preview-5620--cli.netlify.app/

Could you point me to the part of this PR that needs my review?

I do see that docs.netlify.com documents some limitations around this behavior that will need to be updated/removed (on the Environment variables and functions page and the Edge Functions limits page)- I can open a PR to get the ball rolling on those changes.

Sorry, i didn't mean to ask for a review. I just wanted to let you know that this is something that is changing which we might want to document :)

eduardoboucas
eduardoboucas previously approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Left a couple of comments but nothing blocking.

src/commands/dev/dev-exec.mjs Outdated Show resolved Hide resolved
src/lib/edge-functions/registry.mjs Show resolved Hide resolved
@danez danez force-pushed the env-edge branch 3 times, most recently from 3494edb to 252a386 Compare April 14, 2023 12:53
@danez danez requested review from eduardoboucas and a team and removed request for klavavej April 17, 2023 11:47
khendrikse
khendrikse previously approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@khendrikse khendrikse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@danez danez added the automerge Add to Kodiak auto merge queue label Apr 17, 2023
@danez danez requested a review from khendrikse April 17, 2023 12:59
@kodiakhq kodiakhq bot merged commit 9066166 into main Apr 17, 2023
@kodiakhq kodiakhq bot deleted the env-edge branch April 17, 2023 13:10
@token-generator-app token-generator-app bot mentioned this pull request Apr 17, 2023
Merged
@CASABECI CASABECI mentioned this pull request May 8, 2024
Open
@MaxMood96 MaxMood96 mentioned this pull request May 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khendrikse khendrikse khendrikse approved these changes

@eduardoboucas eduardoboucas Awaiting requested review from eduardoboucas

Assignees

@danez danez

Labels
automerge Add to Kodiak auto merge queue needs docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@danez @klavavej @eduardoboucas @khendrikse