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

feat(): prioritize custom config value over environment's value when 'ignoreEnvVars' is 'true' #873

Closed
wants to merge 1 commit into from

Conversation

mahdavipanah
Copy link

@mahdavipanah mahdavipanah commented Feb 28, 2022

Closes #245
BREAKING CHANGE: changes the priority of variables loaded from different sources.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #245

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

…'ignoreEnvVars' is 'true'

Closes nestjs#245
BREAKING CHANGE: changes the priority of variables loaded from different sources.
@mahdavipanah mahdavipanah changed the title feat(): prioritize custom config value over environment's value when … feat(): prioritize custom config value over environment's value when 'ignoreEnvVars' is 'true' Mar 1, 2022
@hcharley
Copy link

hcharley commented Mar 23, 2022

I don't want to jump in on this late, but yes, I was googling today to see if this was possible.

The current default seems a little illogical, with runtime envvars taking precedent over explicitly set and closer-to-code .env files. Seems like the proximity should call for precedent.

I will admit to not knowing much about this topic, as I don't usually have to mix and match runtime envvars with .env files.

All this said, I feel like "ignoreEnvVars" is not what this feature is. The env vars will still be read in, they just won't override the .env file's values. Correct?

Wouldn't this be clearer if it was preferRuntimeEnvVars or preferRuntime for short hand? Or prefer with options [file|runtime]?

@mahdavipanah
Copy link
Author

I don't want to jump in on this late, but yes, I was googling today to see if this was possible.

The current default seems a little illogical, with runtime envvars taking precedent over explicitly set and closer-to-code .env files. Seems like the proximity should call for precedent.

I will admit to not knowing much about this topic, as I don't usually have to mix and match runtime envvars with .env files.

All this said, I feel like "ignoreEnvVars" is not what this feature is. The env vars will still be read in, they just won't override the .env file's values. Correct?

Wouldn't this be clearer if it was preferRuntimeEnvVars or preferRuntime for short hand? Or prefer with options [file|runtime]?

You’re right. I will fix this and make sure that when this option is enabled, env vars will be totally ignored. Just like the other “ignoreEnvFile” option.

@hcharley
Copy link

@mahdavipanah oh thanks for the quick reply!

You are going in the opposite direction of my hopes, but I like that you will make ignoreEnvVars consistent with ignoreEnvFile.

My hopes, and what I was googling for, is to mix and match values from both env files and env vars, but to have the env files take precedent over the runtime.

I share your goal of "changes the priority of variables loaded from different sources" and I don't personally have the usecase to completely ignore env vars, as you suggest.

But I'm not the maintainer, and this is your PR, so feel free to take it whichever direction makes sense for you.

@mahdavipanah
Copy link
Author

I completely understand your point of view but this pull request is specifically for this issue: #245.

Your point on the naming of ignoreEnvVars made sense and so I reviewed #245 and found out that the discussion on the issue suggested completely disabling the picking of env vars as the name suggests.

As for the priority feature you are looking for, I encourage you to open a feature request issue.

@kamilmysliwiec
Copy link
Member

ignoreEnvVars is incredibly misleading and we should just rename it in the next major version. Here's the explanation what it actually does #641 (comment) That means, it shouldn't really have any impact on what takes precedence (env/custom config files). A more relevant name for this configuration option would be ~ ignoreValidationOfEnvVars or skipEnvVarsValidation.

@mahdavipanah
Copy link
Author

mahdavipanah commented Mar 24, 2022

ignoreEnvVars is incredibly misleading and we should just rename it in the next major version. Here's the explanation what it actually does #641 (comment) That means, it shouldn't really have any impact on what takes precedence (env/custom config files). A more relevant name for this configuration option would be ~ ignoreValidationOfEnvVars or skipEnvVarsValidation.

I understand your point and agree that it should not have any impact on precedence and therefore need to update this pull request.

But isn’t it reasonable to couple this option with the behavior that makes ConfigService#get() not pick up the env variables altogether? (as mentioned in #245 comments)

@kamilmysliwiec
Copy link
Member

But isn’t it reasonable to couple this option with the behavior that makes ConfigService#get() not pick up the env variables altogether? (as mentioned in #245 (comment))

We should just introduce a new option for this and I'd be more than happy to accept a PR that does that!

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

Successfully merging this pull request may close these issues.

Unable to properly encapsulate ConfigService from process.env in tests
3 participants