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

allow use of a .env-style files in BaseSettings (#607) #1011

Merged
merged 19 commits into from Jan 19, 2020
Merged

allow use of a .env-style files in BaseSettings (#607) #1011

merged 19 commits into from Jan 19, 2020

Conversation

acnebs
Copy link
Contributor

@acnebs acnebs commented Nov 19, 2019

Change Summary

Update BaseSettings class to allow the use of .env-style files for storing sensitive environment variables.

Just pass in the path (either absolute or relative to the working directory) to such a file in the Config subclass, and pydantic will do the rest for you.

.env

foo=baz

settings.py

from pydantic import BaseSettings

class Settings(BaseSettings):
    foo: str = 'bar'

    class Config:
        env_file = '.env'

test.py

from settings import Settings

s = Settings()
print(s.foo)
# output: baz

Related issue number

#607

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@23c9730). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1011   +/-   ##
========================================
  Coverage          ?    100%           
========================================
  Files             ?      20           
  Lines             ?    3507           
  Branches          ?     683           
========================================
  Hits              ?    3507           
  Misses            ?       0           
  Partials          ?       0
Impacted Files Coverage Δ
pydantic/datetime_parse.py 100% <ø> (ø)
pydantic/errors.py 100% <100%> (ø)
pydantic/class_validators.py 100% <100%> (ø)
pydantic/version.py 100% <100%> (ø)
pydantic/typing.py 100% <100%> (ø)
pydantic/tools.py 100% <100%> (ø)
pydantic/dataclasses.py 100% <100%> (ø)
pydantic/networks.py 100% <100%> (ø)
pydantic/validators.py 100% <100%> (ø)
pydantic/schema.py 100% <100%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23c9730...04c51df. Read the comment docs.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Looking good.

Also needs tests and documentation.

pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
- allow specifying `_env_file` kwarg in instantiation
  * overrides any `env_file` specified in the `Config` class
- cast `os.environ` as a dict for better consistenty of behavior
- `env_path` should be a `Path` type
- replace `with open()` with `read_text`
- use regex for parsing the dotenv files and throw error on invalid line
- factor out `read_env_file` into separate file for easier testing
@acnebs
Copy link
Contributor Author

acnebs commented Nov 20, 2019

Still haven't written any tests yet, but I figure it's best to leave that until we've settled on the rest of the implementation.

pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/read_env_file.py Outdated Show resolved Hide resolved
pydantic/read_env_file.py Outdated Show resolved Hide resolved
pydantic/read_env_file.py Outdated Show resolved Hide resolved
@euri10
Copy link
Contributor

euri10 commented Nov 21, 2019

Sorry to highjack the PR, I'm currently having a simple function to push .env fies into objects inheriting BaseSettings using load_dotenv from the dotenv package.

Stuff like this for instance:

class DatabaseSetting(BaseSettings):
    host: str = "localhost"
    port: int = 5432
    user: str
    password: str
    db: str

    class Config:
        env_prefix = "POSTGRES_"

def get_db_settings(dotenv_path: Union[str, PathLike]) -> DatabaseSetting:
    loaded = load_dotenv(dotenv_path=dotenv_path, verbose=True, override=True)
    return DatabaseSetting()

maybe the "loading" part could take inspiration from it, just an idea,

well either way that's a good idea and will reduce dependencies so thankls for that !

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Also need tests and documentation.

pydantic/env_settings.py Outdated Show resolved Hide resolved
@acnebs
Copy link
Contributor Author

acnebs commented Nov 23, 2019

@euri10 Thanks for the suggestion! I think in the end the dotenv approach is probably a bit much for this builtin functionality at the moment, which is why I initially went for something as simple as possible. I had also found this library while researching different approaches, which does something similar to what you're doing.

But yeah, since Settings handling is a first-class use-case of Pydantic, I felt that it was much nicer to have it integrated into the lib without dependencies.

@acnebs
Copy link
Contributor Author

acnebs commented Nov 23, 2019

Ok, I think I've sorted everything on the PR checklist. Let me know if anything else needs changing.

docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
docs/usage/settings.md Outdated Show resolved Hide resolved
#### `.env`

```
# ignore comment
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs an example of python code to read this file so the stripping of quotes etc. is shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in just include the actual code we're using to parse env files in the docs, or...?

@@ -0,0 +1,6 @@
# this is a comment
Copy link
Member

Choose a reason for hiding this comment

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

Better to use tmp_path than create actual files like this.

Copy link
Contributor Author

@acnebs acnebs Jan 13, 2020

Choose a reason for hiding this comment

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

Is it? It creates a lot of repetition in the tests since we're testing various different things but can re-use the same files. Will make the changes though if you think it is worth it.

tests/test_settings.py Show resolved Hide resolved
tests/test_settings.py Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
@acnebs
Copy link
Contributor Author

acnebs commented Nov 26, 2019

@samuelcolvin Do we really want existing environment variables to take precedence over the .env file?

One of the major benefits of using dotenv is that it makes it far easier to ensure consistency of your settings. In an actual environment, there can be many different places / programs / scripts etc. modifying your vars. If you use the .env file, you can be sure that everything in that file will take precedence. The idea is that you're taking the file, and if it's valid, you're setting all the values inside it as env vars. At least that's how other libs like python-dotenv behave.

That makes the most sense to me, as otherwise you might have a bunch of vars in this file you're loading that are silently ignored, which seems like it would do more harm than good.

@euri10
Copy link
Contributor

euri10 commented Nov 26, 2019

One of the major benefits of using dotenv is that it makes it far easier to ensure consistency of your settings. In an actual environment, there can be many different places / programs / scripts etc. modifying your vars. If you use the .env file, you can be sure that everything in that file will take precedence. The idea is that you're taking the file, and if it's valid, you're setting all the values inside it as env vars. At least that's how other libs like python-dotenv behave.

Don't quote me because on that, I'm not too sure about it, but I think in python-dotenv you need to be explicit about precedence using the override kwarg.

@samuelcolvin
Copy link
Member

No, environment variables should definitely take precedence over variables in .env. As they do in other libraries:

  • with create-react-app environment variables take priority, see here
  • with starlette environment variables take priority, see here
  • I've just tried with python-dotenv, as expected environment variables take priority:
(env) pydantic 1 333ms ➤  pip install python-dotenv
...
(env) pydantic 0 971ms ➤  echo FOO=whatever >> .env
(env) pydantic 0  13ms ➤  echo BAR=other >> .env
(env) pydantic 05ms ➤  cat .env 
FOO=whatever
BAR=other
(env) pydantic 08ms ➤  export FOO=different
(env) pydantic 06ms ➤  ipython

Python 3.7.4
import asyncio, base64, math, hashlib, json, os, pickle, random, re, secrets, sys, time
from datetime import datetime, date, timedelta, timezone
from pathlib import Path

autoreload enabled, use `%autoreload 0` to disable

In [1]: from dotenv import load_dotenv

In [2]: load_dotenv()
Out[2]: True

In [3]: import os

In [4]: os.getenv('FOO')
Out[4]: 'different'

In [5]: os.getenv('BAR')
Out[5]: 'other'

@samuelcolvin
Copy link
Member

samuelcolvin commented Nov 26, 2019

In an actual environment, there can be many different places / programs / scripts etc. modifying your vars.

That's what the env variable prefix is for.

@samuelcolvin
Copy link
Member

Hi @acnebs, are you still okay to finish this?

@acnebs
Copy link
Contributor Author

acnebs commented Dec 9, 2019

@samuelcolvin Yep, just been a bit busy over the past couple weeks. Should be able to get to it soon.

@sm-Fifteen
Copy link

Any news about that PR? I'm pretty excided about getting rid of my custom .env config data extraction and casting logic so I can replace it with Pydantic instead.

@samuelcolvin
Copy link
Member

me too, if there's no progress I'll try and finish it myself when I get a chance.

@sm-Fifteen if you'd like to take over this, by all means create a new PR from this work. I'll make sure we give credit to both developers.

Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@acnebs
Copy link
Contributor Author

acnebs commented Jan 13, 2020

Sorry about the long delay, getting back to this now!

@sm-Fifteen
Copy link

sm-Fifteen commented Jan 13, 2020

Looking at the code more closely, shouldn't we just use python-dotenv (as an optional dependency) for the file parsing instead of making our own spin on it, possibly with its own quirks and issues? Because parsing those files correctly looks awfully tricky.

I just checked and python-dotenv's quoting behavior doesn't even match dotenv's since mismatched quotes (see this file) appear to assume the current line continues as a multiline string in the python lib, but the JS lib treats the lone quote as a literal character. I don't think the current logic implemented in this PR even handles mismatched quotes at all.

python-dotenv would at least ensure that the parsing is done consistently with other Python applications, without adding an extra maintenance burden on pydantic.

import dotenv
# A dict, which can then be used to populate BaseSettings
env_dict = dotenv.dotenv_values('.env')

EDIT: See theskumar/python-dotenv#170 for some of the differences between parsers.

@samuelcolvin
Copy link
Member

No. For reasons I think I explained above.

We can cover 99.9% of usage very easily, if you want more you should probably use yaml or toml.

@samuelcolvin
Copy link
Member

samuelcolvin commented Jan 19, 2020

upon reflection, I've decided to take on @sm-Fifteen's comment and switch to using python-dotenv. While I don't like the extra (optional) dependency for one use, I can't ignore the problems you discuss regarding quotes etc.

@acnebs I'm sorry we spent so long building a dotenv parser that's now not being used, but I really do think this is a better approach.

@samuelcolvin samuelcolvin merged commit 730d842 into pydantic:master Jan 19, 2020
@sm-Fifteen
Copy link

I'm not seeing tests for types other than strings, does the current implementation support other types of field, such as ints, floats, booleans, optionals and possibly JSON-formatted BaseModels?

@samuelcolvin
Copy link
Member

Yes of course, same as pydantic always does. Those values will be coerced to the field's type.

There is definitely at least one test which demonstrates this.

Lists etc. would require the Json type.

@sm-Fifteen
Copy link

Yeah, ok, thank you for confirming this. That pretty much covers what I needed this to do, then.

@acnebs
Copy link
Contributor Author

acnebs commented Jan 21, 2020

@samuelcolvin Sounds good. Thanks for finishing it up!

@layday
Copy link
Contributor

layday commented Jan 24, 2020

BaseSettings._build_values used to be part of the public interface. The docs said:

By default BaseSettings considers field values in the following priority ... This behaviour can be changed by overriding the _build_values method on BaseSettings.

Mention of this was removed in v1 and this PR breaks code which relied on _build_values to change the evaluation order.

@samuelcolvin
Copy link
Member

sorry about that I wasn't aware that _build_values had at one point been publicly documented.

PR welcome if you wish to update the release notes to include this.

@layday
Copy link
Contributor

layday commented Jan 24, 2020

Would you also accept a PR to allow customising the evaluation order in a future version?

@samuelcolvin
Copy link
Member

I guess, though I imagine it could be quite complex.

Quite related: I also want to allow multiple dot-env files with different priorities which are then merged, same as CRA.

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.

None yet

6 participants