-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix: Reading env file when running project locally without docker #29471
base: master
Are you sure you want to change the base?
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.
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.
LGTM, but pinging @mistercrunch in case he knows of some intricacy I don't :) Thanks for opening this!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #29471 +/- ##
===========================================
+ Coverage 60.48% 83.66% +23.18%
===========================================
Files 1931 521 -1410
Lines 76236 37494 -38742
Branches 8568 0 -8568
===========================================
- Hits 46114 31371 -14743
+ Misses 28017 6123 -21894
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Looks like you'll need to run the pre-commit hook to make the linters happy. |
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 this potentially be a breaking change in some environments?
I checked that traditionally-set env vars take precedence over .env
files here, but there may be a small risk of envrionments where:
- a [previously unused]
.env
file exists - maybe some service template at some companies lay a default.env
file for all services .env
file happens to set something that's not desired / causes a change of behavior
Another potential issue would be around docker-compose and the .env
that already exist in the docker/
folder, and just want to make sure that this shouldn't be a problem there either. I can't think of anything but could see edge cases there.
Given issues are super unlikely, I'd like to push this forward, but would like to first:
- add a note in UPDATING.md giving a heads-up to people performing upgrades. Typically they would know whether this might create a change of behavior in their env
- add a note in our docs, letting users know that we support
.env
files
More generally it would be great for our config system to support more env vars, especially for simple / str parameters, maybe for an approved list of parameter and with a clear prefix, as in SUPERSET__SQLALCHEMY_EXAMPLES_URI
. I think this should be a matter of creating a list of params that support this, and for-loop over it while checking for env vars fitting the pattern. + maybe a bit of smart casting to also support boolean, int, maybe even json if we were feeling fancy
SUMMARY
Imported load_dotenv in config.py to read the .env file
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION