Skip to content

Conversation

@leobm
Copy link

@leobm leobm commented May 21, 2022

Thanks, nice project, can use it well myself right now.
But I have extended your project a bit. Maybe you can use it too. What do you say to my changes?

  • Change the order how the environment variables from the .env file merged with the OS environment variables.
    Variables from the .env file can override OS environment variables.

  • Add the possibility within the .env file to refer to variables within the .env file or to replace the placeholder variable with the value from the OS environment variable.

jfwittmann added 2 commits May 21, 2022 14:19
…ile are merged with the OS environment variables.

Variables from the .env file can override OS environment variables.
- Add the possibility within the .env file to refer to variables within the .env file or to replace the placeholder variable with the value from the OS environment variable.
…vironment variables or if the placeholder is empty.
@fgm
Copy link
Owner

fgm commented May 21, 2022

Interesting, thanks for the ideas. These are two distinct changes:

  • swapping the overriding order: for my own use cases, overriding a file with an envvar is the typical need, but I can imagine the need being the reverse, e.g. when running with a fixed environment in different directories. I would not make it the default, but how about placing it behind a flag ?
  • interpreting variables: now that one is intriguing. It makes the most sense in the overriding order you suggest, but actually has a related version by default with the $(foo:-default} syntax. Since this is not part of the usual features of .env files, I'm more perplexed about that one. Have to think more about it.

In the meantime, if you're ok with putting the overriding reversal behind a flag, we could just merge that first part.

main.go Outdated
}
return part
}
log.Printf(`Replacing the variable failed, env key is empty: "%s"`, part)
Copy link
Owner

Choose a reason for hiding this comment

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

Probably only in verbose mode ?

.env.demo Outdated
SHELL=invalid
YOUR_NAME=Felix
GREETING=Hello ${YOUR_NAME}!
TEST_HOME_PATH=${HOME}
Copy link
Owner

Choose a reason for hiding this comment

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

missing terminal LF

Makefile Outdated
demo:
LOCAL=demo go run . -f .env.demo env | sort
demo-convert:
LOCAL=demo2 go run . -f .env.demo env | sort | sed -E 's/(.*)/-e \1/' | tr '\n' ' '
Copy link
Owner

Choose a reason for hiding this comment

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

missing terminal LF : looks like an editor configuration issue ; maybe add a .editorconfig for that ?

}
}()

env := envFromReader(rc)
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned in the general comment, this reverses the overriding logic, so I think it is better to put it behind a CLI flag.

@leobm
Copy link
Author

leobm commented May 21, 2022

Interesting, thanks for the ideas. These are two distinct changes:

swapping the overriding order: for my own use cases, overriding a file with an envvar is the typical need, but I can imagine the need being the reverse, e.g. when running with a fixed environment in different directories.
I would not make it the default, but how about placing it behind a flag ?

https://www.npmjs.com/package/env-cmd
uses e.g. the --no-override "Do not override existing environment variables" flag for something like this
You could also define an opposite flag "--do-override" or something?

interpreting variables: now that one is intriguing. It makes the most sense in the overriding order you suggest,

The Laravel framework uses exactly this format
.env files, that's why I came up with the idea, because it's really handy. I use something like this often when I work with Laravel.
https://laravel-news.com/using-variables-in-your-env-file
https://blog.quickadminpanel.com/how-to-use-laravel-env-example-files/
Which is probably based on https://github.com/vlucas/phpdotenv where you can also use this format.

Also Docker uses the same format e.g. in the Docker files etc.

but actually has a related version by default with the $(foo:-default} syntax.
Since this is not part of the usual features of .env files, I'm more perplexed about that one. Have to think more about it.

Variables makes sense in several cases

  1. to map existing OS environment variables to new names.
  2. it is also good if you have a default path that you can then use again and again in other environment variables without having to rewrite the complete path each time.
WORKSPACE_PATH=/www/system/data
COOP_PARTNER1=${WORKSPACE_PATH}/coop1
COOP_PARTNER2=${WORKSPACE_PATH}/coop2

Ok, I haven't thought about default values yet, would be cool of course, so without
defining an additional environment variable.
The idea with ${FOO:- default} I find not so bad or maybe ${FOO|default} ?
Or another idea, you could define private environment variables with the same name, just with a two exclamation mark in front and in top of the .env file?
!!FOO=VALUE

and then if you use ${FOO} somewhere, but when this variable does not exist as OS environment variable, then !!FOO is used? These private variables must then be deleted from the map if they are not used. These variables can only be used within the .env file and are only accessible there.

  1. to extend or completely replace existing environment variables e.g.
    PATH=~/dev/bin:${PATH}

In the meantime, if you're ok with putting the overriding reversal behind a flag, we could just merge that first part.

Sure, I don't have a problem with that, I could just imagine that sometimes it can be really useful to overwrite certain environment variables, e.g. for the development environment.

Best,

Felix

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.

2 participants