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

feat: brownie config supports env vars #1012

Merged
merged 4 commits into from
Apr 4, 2021

Conversation

omarish
Copy link
Contributor

@omarish omarish commented Mar 23, 2021

This feature adds POSIX-style environment variable expansion to Brownie's config. You can now access environment variables, like you would os.environ, or, define them using the new top-level dotenv flag brownie's configuration.

What I did

  • Adds dotenv: key to brownie-config.yml
  • Reads environment vars from both os.environ as well as a .env file (when specified)
  • Supports default values, like so: ${THE_REAL_DEAL:-dentacoin}, or ${NEEDS_MORE_COWBELL:-true}
  • Supports basic value inference for int and bool.

Related issue: #1000

Demos:

System env vars:

export SHOW_COLORS=true

brownie-config.yml

console:
  show_colows: ${SHOW_COLORS}

Using a .env file

.env:

BUILD_DIR=/dev/teapot

brownie-config.yml

dotenv: .env
project_structure:
  build: ${BUILD_DIR}
networks:
  default: ${DEFAULT_NETWORK:-development}

How I did it

How to verify it

See the tests.

Checklist

  • Few more failing tests
  • I have confirmed that my PR passes all linting checks
  • I have included test cases
  • I have updated the documentation
  • I have added an entry to the changelog
  • Docs

@omarish omarish marked this pull request as draft March 23, 2021 02:02
@PatrickAlphaC
Copy link
Contributor

Would love to see an example in the docs using a non-infura RPC_URL for the host too <3

Just a humble request, great work so far on this!

@omarish
Copy link
Contributor Author

omarish commented Mar 30, 2021

Thanks! Will try my best to get this done as soon as I can.

@omarish omarish force-pushed the feature/1000-dotenv branch 2 times, most recently from d7fa43b to c51dc7b Compare April 2, 2021 03:45
@omarish omarish changed the title [WIP] Adds support for dotenv configuration Adds support for dotenv configuration Apr 2, 2021
@omarish omarish force-pushed the feature/1000-dotenv branch 5 times, most recently from fee10bc to 305da12 Compare April 2, 2021 05:01
@omarish omarish changed the title Adds support for dotenv configuration feat: support for env vars in brownie config Apr 2, 2021
@omarish omarish changed the title feat: support for env vars in brownie config feat: brownie config supports env vars Apr 2, 2021
@omarish omarish force-pushed the feature/1000-dotenv branch 2 times, most recently from 7ddf206 to 5790af1 Compare April 2, 2021 06:17
@omarish
Copy link
Contributor Author

omarish commented Apr 2, 2021

Ok, I think this is coming together now and nearing completion. I'm still seeing a lot of test failures but I have a feeling they're all from the same root cause.

@omarish
Copy link
Contributor Author

omarish commented Apr 2, 2021

Ok, going to dive into this now. Will tidy up and minimize the diff.

@omarish omarish force-pushed the feature/1000-dotenv branch 4 times, most recently from f31081c to 66643db Compare April 2, 2021 22:02
@omarish omarish force-pushed the feature/1000-dotenv branch from 66643db to f05ad0d Compare April 2, 2021 22:07
@omarish
Copy link
Contributor Author

omarish commented Apr 3, 2021

Ok, I figured out what I was doing wrong:

  • I was setting some nonsensical values to test env var propagation. Example: 6ae54d6
  • I changed the tests to be more in-line with reasonable usage, and I'm back on the right path again and things seem to be behaving normally again.

Some takeaways:

  • We might want to put in some timeouts if connecting to a default network takes more than a reasonable amount of time (like 5 minutes). I think that's why the previous builds were hanging and taking ~6h before Github Actions timed them out.
  • Have a couple other ideas about de-coupling configuration a little bit. I started doing this by putting variable expansion in its own file (instead of in _config). I had expansion logic living in brownie.utils earlier, but ran into a circular import issue.
  • We can resolve this circular dependency by moving the color and notify calls from brownie/utils/__init__.py to brownie/utils/color.py. There's some monkey patching going on in a spec that will also need to be checked out as part of this. Also had a previous version of this PR where I had moved all the color logic to brownie.utils.colors.

As for the most recent failures, I believe I'm just getting rate-limited (sorry about that). Either way, my optimistic engineering self thinks that this should work when I try it again a little bit later on. Going to mark it ready for review to start getting feedback; @iamdefinitelyahuman and maintainers: feel free to re-run tests when you take a look at this PR and I think everything will be passing. I'll also check again in a couple of hours.

Also, apologies to anyone else who was held up waiting for my 6h builds to finish.

@omarish omarish marked this pull request as ready for review April 3, 2021 00:13
@omarish
Copy link
Contributor Author

omarish commented Apr 3, 2021

Also @PatrickAlphaC for sure re: example with RPC_URL. Where in the docs do you think it makes the most sense?

@omarish omarish force-pushed the feature/1000-dotenv branch from 6ae54d6 to a221f04 Compare April 3, 2021 04:20
@omarish omarish force-pushed the feature/1000-dotenv branch from a221f04 to 5d99f4f Compare April 3, 2021 04:21
Copy link
Member

@iamdefinitelyahuman iamdefinitelyahuman left a comment

Choose a reason for hiding this comment

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

Generally this looks really good! Just a few minor nitpicks / questions.

brownie/_expansion.py Outdated Show resolved Hide resolved
brownie/_expansion.py Outdated Show resolved Hide resolved
brownie/_config.py Outdated Show resolved Hide resolved
@iamdefinitelyahuman iamdefinitelyahuman merged commit 9f373ad into eth-brownie:master Apr 4, 2021
@omarish omarish deleted the feature/1000-dotenv branch April 5, 2021 00:10
@PatrickAlphaC
Copy link
Contributor

AMAZING

@omarish
Copy link
Contributor Author

omarish commented May 21, 2022

Hey @kkom: good question. It's been a little bit since I did this PR so I don't 100% remember if there was a reason I had pinned it that way. I think this one is @iamdefinitelyahuman's call, but here's what I think:

  1. I read python-dotenv's changelog between 0.16.x and the latest and I also re-read the code I wrote for this PR. Fortunately I don't think I'm calling any private APIs/doing anything that would require the restriction to be that strict.
  2. If we had infinite time I'd suggest testing it on 0.{17,18,19,20}.x, but I'd 👍 your PR to remove the 0.17.0 constraint entirely if it just worked on 0.20 as well and deal with potential issues for 0.17-0.19 in the future if they arise.

@kkom
Copy link

kkom commented May 21, 2022

Sounds good! Btw, what's your thought on the upper constraint? Technically according to semver anything can happen before you hit 1.0.0 so minor releases don't need to be backwards compatible AFAIK. But again, if I replaced <0.17.0 with <0.21.0 we'd be at square one in a few minor releases. So pragmatically I'm inclined to perhaps do <1.0.0, even though it's not 100% proper according to a pure interpretation of semantic versioning. WDYT?

@omarish
Copy link
Contributor Author

omarish commented May 25, 2022 via email

@kkom
Copy link

kkom commented May 25, 2022

Sure thing! My 2 cents are the following – before finalizing the PR I'd take a look at the usages of the library, if they look very basic and unlikely to change – I'd recommend to do <1.0.0. Worst case some forward compatibility patch will need to be issues for brownie, but I honestly suspect that relaxing the dependency graph will overall be a nice benefit for most users.

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.

4 participants