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

Optional use of another .env file as .env.defaults for shared variables #151

Open
1 of 4 tasks
refucktor opened this issue Aug 30, 2019 · 7 comments
Open
1 of 4 tasks
Assignees

Comments

@refucktor
Copy link

Issue type:

  • question
  • bug report
  • feature request
  • documentation issue

nestjs-config version
1.4.3

@nestjs/common+core or other package versions

  • @nestjs/common: 6.0.6
  • @nestjs/core: 6.0.0

Excepted behavior

First of all, Excellent Job! Thanks!
There are some scenarios where environment variables, which are not dependent on any environment, would/could/must be shared or included in your building process.
(e.g.) default email address for contact information, single OAUTH server for authentication, etc.

There is a package dotenv-defaults which provides this kind of functionality (it's just an extension of the original dotenv, but reads another file, like .env.defaults, and merge the values)

Would be a smaller but a nice enhancement of the functionality that your library already provides

Actual behavior or outcome (for issue)

Currently, this package supports a single parsing of a ".env" file

@bashleigh
Copy link
Collaborator

bashleigh commented Aug 30, 2019

hmmm that's a decent feature request!

Instead of using that package we could supply an array of strings to load. for env files and call ConfigService.loadEnv with each before we start building the config.

What would you suggest the outcome be if you have an env define in both files?

@refucktor
Copy link
Author

refucktor commented Aug 30, 2019

Happy you like the idea.

  • I agree on not using the package (dotenv-defaults) cause there is nothing really complex happening inside and the functionality is quite simple to reproduce.

  • Regarding the strategy of merging. I think there would always be different opinions. From my perspective, would be good just having a hierarchy/priorities, here is mine:

    1. Env Var from the system
    2. Env Var from a main file (which by defaults could be just the .env)
    3. Env Var from other files which would be dependent on the project/environment/personal-strategy

Fortunately, 1 and 2 is the current strategy of the original dotenv package, check it here

If you agree in that order then you can do it as the other package is doing it (merging the variables from the main file at last)

const merge = (main = {}, defaults = {}) => Object.assign({}, defaults, main)

So, what do you think? ;)

@bashleigh
Copy link
Collaborator

Regarding the strategy of merging. I think there would always be different opinions. From my perspective, would be good just having a hierarchy/priorities, here is mine

Realistically envs should be envs in production ;) so reading from .env on the server and having a hierarchy seems a little too much in terms of a production env. Ideally I'd like to encourage people not to create an .env file in production and not to depend on files to contain envs in production also. Dev yea why not, might be handy but I can see that if we allow this feature, someone will end up depending on files in a production environment which is a bit meh.
Does that sound reasonable? I'd also like to avoid a process.env.NODE_ENV === 'development' also as we'd probably end up with "It's not working on prod" issues.

Perhaps we should throw an exception? For duplicated env declaration because that's not going to happen in prod. But then we're depending on dotenv to load our envs. tough one 😂

@refucktor
Copy link
Author

Thanks for a nice exchange, and sorry for my long answer 😞

As I said it's quite opinionated the way developers handle environments, I put those priorities cause in my experience that's how most of the teams configure them.

Let's try to cover most of the scenarios (I said try, cause everyone has their own ways 🤓 )

  1. default environment variables: (.env.defaults)
  • They are no secrets, nor environment-dependent
  • They could/will be in your VCS
  • They must be included in every deployment regardless of the environment
  • Will be a nice feature cause then you don't need to repeat the same variable on every environment file (in my dev team we use most of the time 3 environments)
  • They can/could be replaced by specifics environments variables
  1. environment-dependent variables (.env.development, .env.staging, .env.production)
  • They are no secrets, BUT strongly related to the environment
  • They could/will be in your VCS
  • They have priority over default variables as they represent a specific configuration
  • (hopefully) Only one of them will be loaded for a specific environment recreation. (People tend to do this in their CI/CD scripts )
    cp .env.$NODE_ENV .env
  1. secrets environment variables
  • They might defer from environments, but for sure will come as part of CI/CD jobs or Cluster secrets (like k8s)
  • They will not be in your VCS
  • They have first priority and cannot be replaced by any environment file

And answering your ideas

Ideally I'd like to encourage people not to create an .env file in production and not to depend on files to contain envs in production also.

I can understand your approach, as I said, it's quite opinionated how developers handle this topic.
But isn't better to have non-secrets variables, which you need to define anyway, in a more maintainable way as part of your code (e.g. .env.development, .env.staging, .env.production)

I'd also like to avoid a process.env.NODE_ENV === 'development' also as we'd probably end up with "It's not working on prod" issues.

Totally agree with you. Unfortunately you can propose and encourage this approach, but in the end you cannot force a developer to not take the wrong path, it's up to them

Perhaps we should throw an exception? For duplicated env declaration because that's not going to happen in prod. But then we're depending on dotenv to load our envs. tough one

That could be an option, but makes it a bit over-engineering. However, validation of the environment variables is also a nice one. If you want to see a weird all-in-one strategy check this package for webpack, which by convention/configuration deals with a (.env.example) for validation and (.env.defaults) for defaults variables. A bit of dark magic but in the end, some developers find it useful.

@bashleigh bashleigh self-assigned this Sep 6, 2019
@ghost
Copy link

ghost commented Sep 7, 2019

Thumbs up, would be cool to have this feature!

@bashleigh
Copy link
Collaborator

haven't forgotten, just been incredibly busy with work :(

@artursudnik
Copy link

Hi, just came here while looking for a way to have .env.defaults in our project and it looks like this is quite an old discussion. Did it come to any conclusions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants