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

Bug: env var priority: CLI value should override all others #642

Closed
dionjwa opened this issue Jun 10, 2020 · 13 comments · Fixed by #656
Closed

Bug: env var priority: CLI value should override all others #642

dionjwa opened this issue Jun 10, 2020 · 13 comments · Fixed by #656
Labels

Comments

@dionjwa
Copy link

dionjwa commented Jun 10, 2020

I would expect the order of env var priority would be (highest taken first):

  1. CLI
  2. .env file
  3. Defaults from env_var_or_default(...)

However the .env value clobbers the value given on the CLI

Reproduction:

justfile:

export SOME_VAL := env_var_or_default("SOME_VAL", "default")

@test:
    echo "SOME_VAL=$SOME_VAL"

.env:

SOME_VAL=from-file

just gives:

SOME_VAL=from-file

which is to be expected, but

SOME_VAL=fromcli just gives:

SOME_VAL=from-file

instead of:

SOME_VAL=fromcli

So there's no way to set env vars on the CLI if they are already set in the .env file.

@casey
Copy link
Owner

casey commented Jun 10, 2020

Thanks for opening this issue! I agree that this is a bug. I believe that most dotenv implementations don't set environment variables that are already present in the environment.

As a workaround for now, what does just SOME_VAL=fromcli do? Just supports arguments of the form NAME=VALUE, which can override justfile variables. (Note that it's after just, not before it, so it's interpreted by Just and not the shell.)

@casey casey added the bug label Jun 10, 2020
@dionjwa
Copy link
Author

dionjwa commented Jun 10, 2020

Ah! just SOME_VAL=fromcli gives:

SOME_VAL=fromcli

so that is a workaround!

@casey
Copy link
Owner

casey commented Jun 10, 2020

I think the way to fix this is to modify load_dotenv in load_dotenv.rs to remove anything from the dotenv dictionary that is set in the environment.

This would be a breaking change, so I'm eager to hear from people who use .env files with just whether or not this would be desirable.

@hyperupcall
Copy link

hyperupcall commented Jul 11, 2020

In my opinion the change is very welcome, even if it's breaking. I think most people would probably expect that this behavior already is implemented in the first place too

@casey
Copy link
Owner

casey commented Jul 16, 2020

I'm definitely leaning towards doing this. Is this the norm across other .env implementations? I think it is, but I don't have a ton of experience with any of them.

@hyperupcall
Copy link

hyperupcall commented Jul 16, 2020

so i did some digging to be certain, and it looks to be the norm

for node's dotenv they don't override existing variables

We will never modify any environment variables that have already been set. In particular, if there is a variable in your .env file which collides with one that already exists in your environment, then that variable will be skipped.

for go's godotenv, their Load function (which is used in the usage example), has the same behavior (they also have an Overload function that does override though)

It's important to note that it WILL NOT OVERRIDE an env variable that already exists - consider the .env file to set dev vars or sensible defaults

and the same for ruby

By default, it won't overwrite existing environment variables as dotenv assumes the deployment environment has more knowledge about configuration than the application does. To overwrite existing environment variables you can use Dotenv.overload.

so in all these cases if the user supplies the variable themselves, it "wins" over what is in the dotfile

@casey
Copy link
Owner

casey commented Jul 17, 2020

Thank you very much for the research, eankeen! I think keeping Just's dotenv functionality close to other dotenv implementations is probably what users would most expect, so I added this in #656.

Although I don't expect this to be disruptive, this is technically a breaking change, since users could have relied on the .env file overriding environment variables. To reflect this, I bumped the version from 0.6.1 to 0.7.0, and cut a new release with this change.

I just tagged 0.7.0, so binaries should be built and uploaded soon by continuous integration.

This don't technically fix this issue, since the issue is about CLI vs env variables. However, Just variables, set with just foo=bar, just --set foo bar, or foo := "bar", are distinct from .env and environment variables, so I don't think a just var set on the command line should necessarily override an environment variable, or vice versa. So, I'm going to go ahead and close this issue, although feel free to reopen or comment if I've overlooked something.

@casey casey closed this as completed Jul 17, 2020
@runeimp
Copy link

runeimp commented Aug 3, 2020

@casey This change makes sense but only if it includes the overridability used by other tools as well. I believe the .env thing was started with the Ruby project mentioned above. It would be great if we could add something like set dotenv-override := true to match what was done with #469 and could be noted in the docs to achieve the prior behavior before v0.7.0.

@casey
Copy link
Owner

casey commented Aug 4, 2020

@runeimp, so if set dotenv-override := true is set, variables from the .env would override set environment variables? Are there other tools that have this functionality? I'm also curious what the use case is, just so I understand the motivation. Adding a setting would be a pretty easy change, so I'm definitely not opposed to it.

@rohel01
Copy link

rohel01 commented Sep 29, 2023

I am using just to wrap a legacy build system, which is based on Makefiles. The current workflow is heavily reliant on developers sourcing configuration scripts (ex: Yocto) before running make. This is unpractical for at least two reasons:

  • Configuration scripts tend to interfere with each other in confusing ways
  • Tweaking the configuration requires restarting the user shells

This is why I am trying to move towards a just-based workflow with .env for shared configurations and explcit configuration script sourcing only in the relevant recipes.

Right now, I am trying to override the PATH from the .env file to expose basic utility tools (starting with make...) to every one. But my modifications are ignored because PATH is already defined in the environment.

Would this feature help in my use case? Could you suggest another approach?

@hyperupcall
Copy link

hyperupcall commented Sep 29, 2023

@rohel01 Please don't comment on a 3-year-old issue asking for help or suggestions. This repository has the "Discussions" tab enabled - you can also go to StackOverflow, various programming Discords, etc.

@rohel01
Copy link

rohel01 commented Sep 29, 2023

@hyperupcall The comment just above mine was asking for a use-case so I was providing one.
Opening a new discussion as you suggested anyway...

@hyperupcall
Copy link

hyperupcall commented Sep 29, 2023

Oh sorry, then that's my bad - then it's my appologies for misunderstanding and cluttering this discission

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

Successfully merging a pull request may close this issue.

5 participants