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

Add --override flag to override any existing environment variables #63

Closed
wants to merge 3 commits into from

Conversation

raxityo
Copy link

@raxityo raxityo commented Jan 18, 2022

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -70,8 +71,9 @@ if (argv.debug) {
process.exit()
}

const override = !!argv.override
paths.forEach(function (env) {
Copy link
Owner

Choose a reason for hiding this comment

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

should we reverse paths if override is true such that it doesn't have an effect on the order of -e? I.e. that if you have no system variable, it shouldn't matter whether you pass override or not.

Copy link
Author

Choose a reason for hiding this comment

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

I think if it's implicitly known to the users that the order of -e defines how resulting environment variables are applied, then the order should stay untouched regardless of the override flag. That way, the behavior stays consistent with multiple consecutive calls to dotenv in the code.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the problem is that --override only talks about system variables and not about the order of the different files.

If you'd look into the -c production function for example, there we load [.env.production.local, .env.production, .env.local, .env]. Such that local files take precedence over non-local and production over non-production, what you would expect.

In case both -c and --override are passed, with the current implementation, this would mean that values from .env take precedence over .env.production.local which is weird and not what the user would expect.

Copy link

Choose a reason for hiding this comment

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

+1 for this feature(!) - and for it reversing the order in which paths are applied.

this would mean passing both -c and --override with no other changes would have no effect in the case that a "raw" value is defined in multiple files imported via -c.

where it gets interesting (which happens to be my use case!) is where values are defined taking advantage of dotenv-expand to set based on previously-set values. reversing the path order when --override is set will let expanded values in local files refer to non-local, and .env.production refer to values set in .env - as I believe would be naturally expected, mirroring the hierarchy in which the different .env* files are applied (with system values at the root, applied first and available to expansions in all .env* files).

(whereas if the path order is not reversed in the case of --override, passing both -c and --override would cause expanded values in .env to unexpectedly expand to different values based on the presence of values in .env.local etc, whereas more natural is to have expanded values in .env.local take values from .env)

Copy link
Owner

Choose a reason for hiding this comment

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

So basically, there is no good way to make --override work with multiple env files if we just want to pass things to the dotenv library?

I don't want to maintain something that's more than a wrapper around dotenv package, so if there is no good solution, I think it's better to merge nothing. I'm happy to hear about concrete use cases though because some of the use cases that were shared, can be solved some other way already.

Comment on lines +50 to +51
$ NODE_ENV=production dotenv --override -p LOG_LEVEL
test
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe good to also give an example of what it would give without the override flag?

Suggested change
$ NODE_ENV=production dotenv --override -p LOG_LEVEL
test
# By default, dotenv-cli will not override env variables already set
$ NODE_ENV=production dotenv -p NODE_ENV
production
# You can opt-in to this behavior using `--override`
$ NODE_ENV=production dotenv --override -p NODE_ENV
test

Notice that you had LOG_LEVEL <> NODE_ENV in your example

@@ -70,8 +71,9 @@ if (argv.debug) {
process.exit()
}

const override = !!argv.override
paths.forEach(function (env) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think the problem is that --override only talks about system variables and not about the order of the different files.

If you'd look into the -c production function for example, there we load [.env.production.local, .env.production, .env.local, .env]. Such that local files take precedence over non-local and production over non-production, what you would expect.

In case both -c and --override are passed, with the current implementation, this would mean that values from .env take precedence over .env.production.local which is weird and not what the user would expect.

@kenberkeley
Copy link

Any update for this PR?

@entropitor
Copy link
Owner

The problem is that --override doesn't really work well together with multiple env files. I think it makes more sense to add a flag to ignore a specific env variable already set. Because all other use cases can be achieved already AFAIK.

It seems some people also want this flag for the wrong reasons (reverting the order of files, while you can just pass -e in the reverse order as well to achieve this)

@IdanDavidi
Copy link

@entropitor in my case I need to override an environment variable given to a pod (for example overriding the default connection string for test purposes). any chance we can merge this PR?

@entropitor
Copy link
Owner

@IdanDavidi This PR is broken as you can see here #63 (comment).

I think something like #63 (comment) could work but nobody seems to be willing to put the work in so I don't think it's happening.

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.

Override system variables
5 participants