Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for python-dotenv at the config level #1000

Closed
PatrickAlphaC opened this issue Mar 19, 2021 · 14 comments · Fixed by #1063
Closed

Add support for python-dotenv at the config level #1000

PatrickAlphaC opened this issue Mar 19, 2021 · 14 comments · Fixed by #1063
Labels
enhancement New feature or request Help Wanted Pull requests to fix greatly appreciated!

Comments

@PatrickAlphaC
Copy link
Contributor

Overview

I'd love to see brownie support a .env file for environment variables. For new engineers, or a project that doesn't want to keep a ton of environment variables running, a .env file is a warm welcome. Truffle and Hardhat (the javascript smart contract frameworks) have really nice config files that allow them to work with .env files. A bit of a quality of life improvement.

Otherwise, we have to call in the .env file in all our script manually, seems pretty redundant to do so.

Specification

Not sure how this is normal done in yaml, but would like to see something like:
dotenv: true

And then they can load via a .env file. We could just use https://pypi.org/project/python-dotenv/ to load at the start of all the scripts.

Dependencies

https://pypi.org/project/python-dotenv/

What do we think?

@iamdefinitelyahuman
Copy link
Member

I see no harm in checking for the existence of this file when loading a project, and processing it if it's found.

@iamdefinitelyahuman iamdefinitelyahuman added enhancement New feature or request Help Wanted Pull requests to fix greatly appreciated! labels Mar 19, 2021
@omarish
Copy link
Contributor

omarish commented Mar 22, 2021

Would it make sense if the value of the dotenv key in the YAML config is a path instead of a boolean? That way you can specify if you're using a different dotenv.

@PatrickAlphaC
Copy link
Contributor Author

Ooooo. That's much better

@omarish
Copy link
Contributor

omarish commented Mar 22, 2021

Cool. And @iamdefinitelyahuman, do you think the dotenv file should be a) auto-loaded, or b) loaded only if dotenv: is specified in the YAML config?

@PatrickAlphaC
Copy link
Contributor Author

I vote for loading only if it's specified.

@skellet0r
Copy link
Collaborator

Loading only if explicitly opted in via the YAML config sounds better to me. Sort of how docker-compose allows one to load an env-file or multiple. Plus, security wise might be better to opt-in 🤔

https://docs.docker.com/compose/environment-variables/#the-env_file-configuration-option

@omarish
Copy link
Contributor

omarish commented Mar 22, 2021

I agree. I'd just like to get buy-in from @iamdefinitelyahuman before building something out.

When it comes to crypto projects, I'm an advocate for configuration over convention. Writing an extra line in a config file is a great price to pay, especially if it means avoiding unexpected configuration behavior.

@iamdefinitelyahuman
Copy link
Member

Sounds good to me! I agree that the explicitness of adding a line to the config is better than potential accidentally... some config settings.

@omarish
Copy link
Contributor

omarish commented Mar 23, 2021

Excellent. Another realization is that this should probably support interpolating envvars in the config, like this:

.env:

DEFAULT_NETWORK=development

brownie-config.yaml:

dotenv: ./.env
networks:
  default: ${DEFAULT_NETWORK}

Double-checking that this makes sense to you, @iamdefinitelyahuman and @PatrickAlphaC?

@omarish
Copy link
Contributor

omarish commented Mar 23, 2021

@iamdefinitelyahuman I have something basic up, but 1) it's not quite working yet, and 2) I'm still thinking about what should be in charge of loading the env file. It seems like a responsibility of main.py. An earlier implementation I had done was loading it in the config container, but I don't think the config container should be in charge of something stateful like loading an env file.

That said, if we do want to support POSIX-style variable interpolation, we'd probably want access in the ConfigContainer level so that we only have to parse once.

Thoughts?

@PatrickAlphaC
Copy link
Contributor Author

Excellent. Another realization is that this should probably support interpolating envvars in the config, like this:

.env:

DEFAULT_NETWORK=development

brownie-config.yaml:

dotenv: ./.env
networks:
  default: ${DEFAULT_NETWORK}

Double-checking that this makes sense to you, @iamdefinitelyahuman and @PatrickAlphaC?

Yes. 100%. This was something that I was surprised didn't work by default. This will be a massive improvement adding that to the yaml.

@gnattishness
Copy link
Contributor

As I didn't see this mentioned previously, direnv is an alternative to having brownie process .env files on its own.

Its a tradeoff between suggesting users install a separate tool and increasing the amount of things brownie needs to do.
(Though I don't have a particular opinion either way)

@omarish
Copy link
Contributor

omarish commented Apr 15, 2021

Makes sense. I can add this in; should be pretty straightforward. Sorry for the delay, recently made the 2to3 family migration and just getting back into my routine now.

@PatrickAlphaC
Copy link
Contributor Author

Congratulations!!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request Help Wanted Pull requests to fix greatly appreciated!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants