Add support for additional env var files #9961

orta · 2024-02-03T12:19:14Z

Adds a new middleware step to the CLI booting up, which looks for --include-env and includes .env.[file] to the list of files to look at.

Also generally adds a .env var based on the NODE_ENV - I could take this or leave it, but I was in the space.

packages/cli/src/index.js

Agree, with your copy changes 👍 Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>

Tobbe · 2024-02-08T08:20:23Z

@jtoar highlighted to me that this PR currently only changes how the main CLI entry point loads the env files.

We also need to change how this is handled when deploying a RW app. So need to look into our different deploy providers and also our Dockerfile support.

Tobbe · 2024-02-09T08:45:29Z

I just now randomly found out there's some prior art on the flag naming with node's v20.6.0 release: https://nodejs.org/en/blog/release/v20.6.0

node --env-file=config.env index.js

There are also some good discussions in the related PR nodejs/node#48890

Tobbe · 2024-02-09T08:49:08Z

So what we could do, to keep that same naming convention, and also get around the "how do we let people know that it's additive?" question. Plus make it really easy to not load .env if you don't want to, is to just have the default value for --env-file be .env. If you specify that flag on your own you're overriding the default value. So if you wanted .env and .env.puzzmo you'd do yarn rw exec --env-file .env --env-file .env.puzzmo my-script.ts

Thoughts? Reactions?

dthyresson · 2024-02-09T13:39:32Z

As I am reading thought the discussions from #9877 and the comments here, I definitely am longing for a docs section in CLI commands to help me understand a few things:

when I see env I think "my current environment", so 9 times out of 10, dev
when I yarn rw dev ... I am in dev, so my .env and .defaults apply
then, I want to include "some additional settings" that would a) override what was set above b) warn on conflict? c) halt if collision?

I glanced through the Pr and not sure I saw or not, but as will anything envier or config related, I as a developer would want to know with 100% confidence what my envars are. I would hate to think I was using one db connection and in fact using another and break production.

So:

Should there be some safety net to handle override, collision ?
Should when used it output the current value of envars that yarn rw dev is using so it's clear what it currently in play
Should (or could) Studio have a way of presenting the current values of the project's envars?

On a side note, during the roadmap discussion we considered having better secrets management and https://github.com/Infisical/infisical was mentioned as an OSS secrets manager.

Tobbe · 2024-02-09T13:49:45Z

So:

Should there be some safety net to handle override, collision ?

Should when used it output the current value of envars that yarn rw dev is using so it's clear what it currently in play

Should (or could) Studio have a way of presenting the current values of the project's envars?

All valid concerns!

The same question was brought up in the linked nodejs PR. They decided it could be a follow-up PR, so I'll go with the same decision
That makes sense. But I do worry it's going to flood your screen. In my real-life deployed apps I easily have 20+ env vars. Would not want to see those printed for every yarn rw <cmd> command I run. Plus we'd have to figure out some kind of redaction/masking of sensitive values, while still making it clear what DATABASE_URL (etc) you're actually using. I think this should actually be a separate discussion/PR. And one that could be started right now too, no matter what happens with this PR
Something like Infisical integrated into Studio would be dope! Can you please create an "Enhancement" issue for it @dthyresson over in the Studio repo?

orta · 2024-02-09T16:08:27Z

Personally not a fan of allowing it outside of dev server and scripts, I don't really think you should be using .env files on prod and especially unconventional ones. IMO hot switching quickly is useful for building, not for long-term systems.
Probably better to do the node API, definitely, and do --env-file 👍🏻
I don't think you should need to to --env-file .env though, because then you'd need to replicate the existing infra like --env-file .env.defaults too and that's gonna get forgotten
I'm not against a single liner summary when these are added:

yarn rw exec Thing --env-file .env.stripe
[!] Also adding variables from .env.stripe: STRIPE_API_KEY, STRIPE_PRICE_ID, ETC,

Tobbe · 2024-02-17T10:20:39Z

FWIW, when starting the NextJS dev server they print the list of loaded .env files (but not the actual env vars)

I don't really think you should be using .env files on prod

Our own baremetal deploy provider uses .env for its env var support. So that's something we already do. Not saying we should/shouldn't support more than .env though. I'd ask for @cannikin's input before making a decision on that

I don't think you should need to to --env-file .env though, because then you'd need to replicate the existing infra like --env-file .env.defaults too and that's gonna get forgotten

Yes. True. But only if you start modifying what env files you want to load. By default (not passing any --env-file arg) we'd still load .env, .env.defaults etc. Not saying I don't agree with you. Just wanted to make it clear how it'd work 🙂

I'm not against a single liner summary when these are added:
yarn rw exec Thing --env-file .env.stripe
[!] Also adding variables from .env.stripe: STRIPE_API_KEY, STRIPE_PRICE_ID, ETC,

I think @dthyresson's point was if you unknowingly included two files that both have DATABASE_URL you could be connecting to your prod database when you thought you'd be connecting to your dev database. Just listing DATABASE_URL in the list of loaded env vars wouldn't help. You'd have to see the actual value to know. But obviously we don't want to log your full connection string... Which is why I said "we'd have to figure out some kind of redaction/masking". But as I said last time, this could be a separate PR later.

Tobbe · 2024-02-17T10:22:22Z

I've asked @jtoar to take lead on moving this PR forward next week. I definitely want --env-file support in some form. He'll just help make sure the last details gets decided on and that the implementation is in a good place so we can merge it 🙂

cannikin · 2024-02-20T02:00:46Z

We definitely want to keep loading .env in all environments by default—baremetal deploy assumes this when deploying, and as we become less targeted towards the Jamstack this is the easiest way to load your vars on a real server. Users are free to load ENV vars however they want, but I think going with a standard .env file is the simplest way to get started.

packages/cli/src/index.js

This reverts commit c7a19d3.

jtoar · 2024-02-24T04:12:54Z

I'm not sure that it's the best call to name the flag the same thing as the runtime's flag when their behaviors aren't one-to-one. The way the functionality works now, it's additive, so without bikeshedding too much some variation on "include" seems fitting. I've updated it to --include-env-files because the "-file" suffix does make it clear that it's working on files. It's just that right now this flag can be 1) specified multiple times or given multiple arguments and 2) only adds to process.env, not overrides both of which aren't what Node's --env-file flag does as far as I can tell. But yeah not a super strong opinion or anything—mostly just want to get this merged since it looks good to me and anything can still be changed. This will be released in the next minor so no rush. But don't want it to stay here unmerged.

A few more notes on some changes:

Here's yargs syntax for options that are arrays: https://github.com/yargs/yargs/blob/main/docs/tricks.md#arrays. You don't have to specify the flag multiple times but can
Added tests. Was also testing locally in a more e2e way but wasn't sure how to codify that yet
I haven't included the functionality here in other entry points yet because I'm not sure how much it applies. Most deploy providers doesn't have env files at deploy time, or they have a separate way of configuring them. Baremetal is the only one that is literally one to one and in which case, just use yarn rw serve anyway? But we could add the functionality to other entry points if we decide it makes sense to

Fixes #9877 Adds a new middleware step to the CLI booting up, which looks for `--include-env` and includes `.env.[file]` to the list of files to look at. Also generally adds a .env var based on the `NODE_ENV` - I could take this or leave it, but I was in the space. ![image](https://github.com/redwoodjs/redwood/assets/49038/267c9f8d-0b4d-4c83-8607-712b837e3cfc) --------- Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com> Co-authored-by: Dominic Saadi <dominiceliassaadi@gmail.com>

…onsolidate-vite-settings-rsc-builds * 'feat/rsc-build' of github.com:dac09/redwood: (34 commits) Add support for additional env var files (redwoodjs#9961) Revert startsWith change Remove outdated todo comment. And other comment formatting no more 'as string', and startsWith instead of includes innerText Update studio.md (redwoodjs#10062) doc comment formatting node-loader should look in distRsc now, not distServer Remove `serve` from test fixture Code comment tweaks/fixes Update some more comments Remove duplicated file Lint Another rename of build functions Fix paths test with new paths Rename build functions Tobbe review changes Apply suggestions from code review chore(rename): Be consistent with 'for' prefix for babel plugin option (redwoodjs#10059) RSC: Remove commented code from worker (redwoodjs#10058) ...

@cannikin

Follow up to #9961. Got some feedback from @cannikin and @Josh-Walker-GM. There are still some things I need to clarify, but the changes here are improvements, and we'll keep iterating before this goes out next week (currently in the v7.1 RC): - the name was shortened from `--include-env-files` to `--add-env-files` - `.env` files that are loaded based on `NODE_ENV` override instead of add - the logic was moved up from middleware because env loading should be done as soon as possible, and the earliest is right after we set the cwd I'll bring up the override vs at the next meeting. Just to document what currently happens to completeness, env files are loaded in two steps... #### Layer 1 - `.env.defaults` - `.env` - `.env.${NODE_ENV}` These are loaded in an overriding manner, so that if there are conflicts, `.env.${NODE_ENV}` has the final say. This can't be disabled. #### Layer 2 If users want, they can add more via `--add-env-file`, like `--add-env-file stripe`. The env vars loaded from files specified this way are additive—they won't override anything already set in the first layer. I'll see about moving this functionality out of the CLI package and into CLI helpers so its shareable in another PR.

@cannikin

Follow up to #9961. Got some feedback from @cannikin and @Josh-Walker-GM. There are still some things I need to clarify, but the changes here are improvements, and we'll keep iterating before this goes out next week (currently in the v7.1 RC): - the name was shortened from `--include-env-files` to `--add-env-files` - `.env` files that are loaded based on `NODE_ENV` override instead of add - the logic was moved up from middleware because env loading should be done as soon as possible, and the earliest is right after we set the cwd I'll bring up the override vs at the next meeting. Just to document what currently happens to completeness, env files are loaded in two steps... #### Layer 1 - `.env.defaults` - `.env` - `.env.${NODE_ENV}` These are loaded in an overriding manner, so that if there are conflicts, `.env.${NODE_ENV}` has the final say. This can't be disabled. #### Layer 2 If users want, they can add more via `--add-env-file`, like `--add-env-file stripe`. The env vars loaded from files specified this way are additive—they won't override anything already set in the first layer. I'll see about moving this functionality out of the CLI package and into CLI helpers so its shareable in another PR.

Last change before releasing the minor. This wraps up the env files feature started in #9961. To recap on the naming decision a bit, we can't use just "--env-file" because Node.js seems to parse it no matter what, and since Redwood's flag operates on suffixes, Node throws a "not found" error. It also seems to parse "--env-files", with an "s", which seems like a bug. I opted for "--add-env-files" back when this flag only added additional env vars, which didn't override existing ones. This is no longer the case (they override), so I don't want to imply that they don't. "--load-env-files" seems like the best alternative, and there's a precedence in the work done in Node.js here which exposes a new `loadEnvFile` function: nodejs/node#51476.

orta added 3 commits February 3, 2024 11:55

Add support for additional .env files in the cli = re: redwoodjs#9877

ec36eeb

Fixes

04f6550

Update the template

76b4eea

Tobbe reviewed Feb 3, 2024

View reviewed changes

packages/cli/src/index.js Outdated Show resolved Hide resolved

packages/cli/src/index.js Outdated Show resolved Hide resolved

Apply suggestions from code review

25d3428

Agree, with your copy changes 👍 Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>

jtoar added the release:feature This PR introduces a new feature label Feb 9, 2024

jtoar added this to the next-release milestone Feb 9, 2024

chore: lint fix

1579861

jtoar reviewed Feb 24, 2024

View reviewed changes

packages/cli/src/index.js Outdated Show resolved Hide resolved

jtoar added 12 commits February 23, 2024 17:33

fix: register middlewares once

6b679d0

update test project fixtures

aba2606

refactor to use path join and envPath

d12b6d3

revert rsc change

1eefb3e

Merge branch 'main' into includeEnvs

fdc1ea9

reapply rsc fixture change

dd10521

clean up argv

7c8e022

work on tests

c7a19d3

Revert "work on tests"

01ffc44

This reverts commit c7a19d3.

work on tests

573df8f

rename flag

89cac9c

changelog and closing changes

754585b

update server test

385f7c9

jtoar approved these changes Feb 24, 2024

View reviewed changes

jtoar merged commit 9b1dd05 into redwoodjs:main Feb 24, 2024
41 checks passed

jtoar mentioned this pull request Mar 2, 2024

Iterate on .env file loading #10093

Merged

jtoar mentioned this pull request Mar 7, 2024

fix(env files): rename to --load-env-files #10123

Merged

jtoar modified the milestones: next-release, v7.1.0 Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional env var files #9961

Add support for additional env var files #9961

orta commented Feb 3, 2024

Tobbe commented Feb 8, 2024

Tobbe commented Feb 9, 2024

Tobbe commented Feb 9, 2024 •

edited

Loading

dthyresson commented Feb 9, 2024 •

edited

Loading

Tobbe commented Feb 9, 2024 •

edited

Loading

orta commented Feb 9, 2024 •

edited by Tobbe

Loading

Tobbe commented Feb 17, 2024 •

edited

Loading

Tobbe commented Feb 17, 2024

cannikin commented Feb 20, 2024

jtoar commented Feb 24, 2024 •

edited

Loading

Add support for additional env var files #9961

Add support for additional env var files #9961

Conversation

orta commented Feb 3, 2024

Tobbe commented Feb 8, 2024

Tobbe commented Feb 9, 2024

Tobbe commented Feb 9, 2024 • edited Loading

dthyresson commented Feb 9, 2024 • edited Loading

Tobbe commented Feb 9, 2024 • edited Loading

orta commented Feb 9, 2024 • edited by Tobbe Loading

Tobbe commented Feb 17, 2024 • edited Loading

Tobbe commented Feb 17, 2024

cannikin commented Feb 20, 2024

jtoar commented Feb 24, 2024 • edited Loading

Tobbe commented Feb 9, 2024 •

edited

Loading

dthyresson commented Feb 9, 2024 •

edited

Loading

Tobbe commented Feb 9, 2024 •

edited

Loading

orta commented Feb 9, 2024 •

edited by Tobbe

Loading

Tobbe commented Feb 17, 2024 •

edited

Loading

jtoar commented Feb 24, 2024 •

edited

Loading