-
Notifications
You must be signed in to change notification settings - Fork 423
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: support reading/loading multiple .env files #424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be improved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double checked
in cli.py we may use multiple files and getting multiple errors but want to avoid multiple errors if at least one file has the key
@duarte-pompeu will you take care of resolving the conflicts before we ping the maintainer for review? |
I can give it a try tonight, it would be interesting to merge this feature. |
Solved conflicts but there's still some work to do regarding failing tests. |
Think it looks better now @pheanex 🙂 It's missing more tests and documentation, but I'd prefer to have feedback from a maintainer before tackling those. |
Awesome! Let's see if @bbc2 or @theskumar is available for a quick review? |
Fundamentally as feature I'm good with supporting this. I like your approach and seeing how you are going about it. I haven't had time to look into the implementation details. But I feel free to improve on your implementation. Would really appreciate keeping it backward compatible. Thank you @duarte-pompeu for your contribution. |
Actually I already implemented it: Example as of 49c34a5: $ for f in $(ls .env*); do echo $f; cat $f; echo; done
.env1
a=1
b=2
.env2
b=2
c=3
# unset 'a', which is only present in .env1
$ python -m dotenv -f .env1 -f .env2 unset a
Successfully removed a from .env1.
$ for f in $(ls .env*); do echo $f; cat $f; echo; done
.env1
b=2
.env2
b=2
c=3
# unset 'b', which is present in both .env1 and .env2
$ python -m dotenv -f .env1 -f .env2 unset b
Successfully removed b from ['.env1', '.env2'].
$ for f in (ls .env*); do echo $f; cat $f; echo; done
.env1
.env2
c=3
# try to unset key which does not exist
$ python -m dotenv -f .env1 -f .env2 unset d
Key d not removed from ('.env1', '.env2') - key doesn't exist. |
dotenv_path.write_text(contents[0] + '\n') | ||
extra_dotenv_path.write_text(contents[1] + '\n') | ||
|
||
args = ['--file', dotenv_path, '--file', extra_dotenv_path, 'list'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I omitted tests with multiple formatting because that seems to me like a separate concern - if the variables are loaded correctly, the formatting behavior shouldn't change.
Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. This is fine.
I think this is ready for review @theskumar:
We didn't talk about this, but I only implemented this for the CLI utility. If there's interest to also enable this behavior from the library code, I'm up to contribute to it (but would prefer to have it in a separate PR, as I find this one already useful as is). |
Hi @theskumar and @pheanex , any feedback? |
Lgtm. Looking forward to this |
@duarte-pompeu thanks for all the hard work. Unfortunately, I've had not been able to review this yet. Looks like we Christmas 🎄 break would be a good time to taking care of it for me, along with few more pull requests. |
Cool, I also have some free time now so it would also work for me 😃 |
a=1 | ||
b=20 | ||
c=30 | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duarte-pompeu Consider document the precedence/loading order values from the of the files. I didn't see a test but based on the this example it seems the .env2
overrides the previous value from .env1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree: it's important to document this. I chose an overriding behavior, as my use case would be:
- have a big file with all values
- have smaller .env files with specific values to override in certain situations
I tried to improve it on 363aab5
README.md
Outdated
@@ -3,7 +3,7 @@ | |||
[![Build Status][build_status_badge]][build_status_link] | |||
[![PyPI version][pypi_badge]][pypi_link] | |||
|
|||
Python-dotenv reads key-value pairs from a `.env` file and can set them as environment | |||
Python-dotenv reads key-value pairs from `.env` files and can set them as environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to change this yet as the core/python API, doesn't support reading from multiple files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I agree it makes sense to only document this in the CLI section.
Resolved in 1d33b6a
changes only affect the CLI, not the whole package
Hi @theskumar , I agree with your feedback and addressed in the last commits. What do you think? |
@theskumar any chance to take a quick look again? |
@bbc2 any chance you can take a look at this? It seems @theskumar is currently unavailable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this (we might be opening Pandora's box but so be it) and it looks like you addressed all the concerns raised previously, so if no one objects for a few more days I'll go ahead and merge it. Thank you for your contribution!
@bbc2 as two more weeks have passed: Are we good to move forward with this? |
It seems to be a great feature. Any chances to it get merged? |
This is supported by dotenvx, which seems a better way to load dotenvs using the CLI instead of a library call. It's been 2 years since opening the PR, I'm going to close it. |
This PR allows reading from multiple
.env
files in the dotenv CLI, specifically the targetsget
,list
andrun
.This will allow users to have a big
.env
file with default configs, and smaller ones which only incremental changes - therefore removing the burden of repeating unchanged key-values.The goal for the new behavior is as follows:
Extra remarks:
.env
when no file is specifiedset
andunset
Example:
Ideas for the future:
.env
files from the code, egload_dotenv(f1, f2, ...)
.env
filesRelates to #418 .