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.

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

[RFC]: Environment group support in the CLI #9877

Closed
1 task done
orta opened this issue Jan 24, 2024 · 10 comments · Fixed by #9961
Closed
1 task done

[RFC]: Environment group support in the CLI #9877

orta opened this issue Jan 24, 2024 · 10 comments · Fixed by #9961

Comments

@orta
Copy link
Contributor

orta commented Jan 24, 2024

Summary

Add support for environment groups to the Redwood CLI, so that we can move env vars which are complementary into separate files and are explicitly loaded.

Right now this is handled by commenting out certain env vars, which requires a lot more context for a developer.

I could imagine this being a --env arg to yarn rw which dictates additional .env files which are picked up in addition to the .env:

yarn rw --env prod dev

is run with both: .env and .env.prod loaded.

yarn rw --env nakama,prod exec UpdateUsers

is run with .env, .env.nakama and .env.prod loaded.

Motivation

Today the Puzzmo .env file is sprawling. When you run the redwood server, you are optionally running a separate sibling server (or connecting to the staging/prod one), we have local/dev/staging dbs, stripe prod/staging env vars, session secrets etc etc

All of these have each sub-environments are commented off/on in the .env file and the developer is responsible for having a sense of what their env actually represents.

It's the general tension that I anticipated with #9297 in the number of scripts we have in our app ( ~30 files now, after a recent cleanup ) - but last night we had a developer who rarely touches the API run a prisma migration which ended up nuking everyone's games on prod when trying to add (and remove) a field to that table

I thought we were going to lose hours/days of prod data, but render could backup to the minute which was a pleasant surprise.

Detailed proposal

  • I think this hits all three of the CLI/web/api, though it mostly is important on a day to day for the cli/api.

  • Rails has something pretty similar, but does more stuff:

    rails server -e production 

    My RFC however isn't focused on the same definition of 'env' here, because you can't say rail server -e nakama,prod,stripe-staging, I'm looking for the ability to control the environment vars but maybe --env wants to be an argument used for something bigger further down the line?

  • My approach above is an env augment only proposal, so there'd be no way to not run the .env. You could have a more complex setup which allows excluding .env but I think that's overcomplicating it, if you have such a complex setup that you need to exclude it, make it empty.

Are you interested in working on this?

  • I'm interested in working on this
@cannikin
Copy link
Member

When we were building the .env stuff, I thought for sure the package we were using supported doing stuff like .env.test and .env.production and they were picked automatically based on your NODE_ENV. There are even docs explaining how it works! But, it doesn't work. Only .env and .env.defaults are supported. I don't know if we lost that functionality, or it never worked, and the docs were written in a fever dream. :(

@colbywhite
Copy link
Contributor

love the idea. quick suggestion on implementation.

to a certain extent, dotenv (which i believe is what is being used under the cover) already supports this via the DOTENV_CONFIG_PATH env var option (and its CLI equivalent dotenv_config_path). what you're suggesting is for a way to specify the path not by the full path, but by the suffix (./my-dir/.env.production vs production).

which i like. it's less typing and matches the way we talk about that file colloquially.

i'd just vote to implement it similarly to the way dotenv implemented it.

  • use both a env var and a CLI arg. the env var is handy in CI contexts
  • instead of --env aim for something a bit more verbose. maybe --env-name, --dotenv-suffix, --env-suffix, --extra-env-vars, --extra-env-suffix. something along those lines might help communicate that it's augmenting the env, not replacing. (granted, naming is hard and im not sure any of my suggestions are home runs.)

@orta
Copy link
Contributor Author

orta commented Jan 24, 2024

Just to be explicit, ^ would not solve my problem as I have multiple facets on which I want to set my env vars against - db local/prod/staging, stripe prod/staging, sibling API local/prod/staging etc

a single folder could work, but with massive duplication of env vars on my side

@cannikin
Copy link
Member

So you want to use like the local db, but the prod API, or some combination thereof?

@orta
Copy link
Contributor Author

orta commented Jan 24, 2024

Yeah, I'm always switching between different sets of env vars in our API for scripts and occasionally for dev

@colbywhite
Copy link
Contributor

i think my proposal still works for your "augment with multiple files" use case if you just allow for multiple env names like your proposal states. i.e. --dotenv-suffix=prod,foo,bar. (maybe --dotenv-suffixes?)

the original DOTENV_CONFIG_PATH doesn't work like that, but i like your strategy better.

@Tobbe
Copy link
Member

Tobbe commented Jan 25, 2024

My knee jerk reaction when first reading this, as far as naming goes, was --env-files=staging,foo,bar. But seeing your examples @colbywhite I'm kind of liking going with dotenv instead in some form. One option is to just do --dotenv, but let you specify it more than once. yarn rw --dotenv staging --dotenv foo --dotenv bar exec UpdateUsers. But maybe that's still not clear enough that it's additive to the standard .env and .env.defaults files

And just to be clear, or CLI uses https://www.npmjs.com/package/dotenv-defaults

@orta
Copy link
Contributor Author

orta commented Jan 25, 2024

Yeah, good points!

I think both are reasonable to me, and I'd like to steer away from --env as that could be something more grandiose in the future.

In the same space, --additional-envs staging,foo,bar?

@dac09
Copy link
Contributor

dac09 commented Feb 3, 2024

Just adding my 2p here!

I'd like to steer away from --env

Yep agreed! The flag should also indicate that by the default RW CLI will load some dotenv by default.

+1 for --additional-env or --include-env.

On how the args should be parsed:
We shouldn't modify how yargs will parse this - I believe the yargs parser will handle all the different ways of supplying array inputs, as long as we specify that its of type array.

Reading NODE_ENV to pick the correct one
As @cannikin mentioned it doesn't work correctly - and its true! But I believe these docs were meant to me specific to the serverless CLI (which handles .env differently to us).

Would it make sense to support this behaviour so that it listens to NODE_ENV? That will mean for example:

NODE_ENV=production yarn rw dev --include-env=staging_db

would load both .env.production, .env.staging_db and .env.defaults

The reason I bring this up, is so that changes to env variable handling all go in together.

@cannikin
Copy link
Member

cannikin commented Feb 3, 2024

Would it make sense to support this behaviour so that it listens to NODE_ENV?

That would be my preference: respond to the loaded environment by default, and you only need additional flags if you’re trying to do this cross-environment group loading. Keep the common case as simple as we can.

jtoar added a commit that referenced this issue Feb 24, 2024
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>
jtoar added a commit that referenced this issue Feb 24, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants