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

Providing only one nested value from environment variable #888

Closed
idmitrievsky opened this issue Oct 11, 2019 · 4 comments · Fixed by #891
Closed

Providing only one nested value from environment variable #888

idmitrievsky opened this issue Oct 11, 2019 · 4 comments · Fixed by #891

Comments

@idmitrievsky
Copy link
Contributor

@idmitrievsky idmitrievsky commented Oct 11, 2019

Feature Request

  • OS: macOS
  • Python version import sys; print(sys.version): 3.6.8 (default, Jun 24 2019, 16:49:13) [GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.46.4)]
  • Pydantic version import pydantic; print(pydantic.VERSION): 1.0b2

Hello, great work on the library! I can't find the reason, why environment variables don't work in nested settings. I have a class

class Credentials(pydantic.BaseModel):
    host: str
    user: str
    database: str
    password: pydantic.SecretStr

class Settings(pydantic.BaseSettings):
    db: Credentials

    class Config:
        env_prefix = 'APP_'

and I want to specify password as an environment variable

export APP_DB='{"password": ""}'

but pass other fields in as values in code. Right now BaseSettings does simple top-level merging of dictionaries that doesn't consider nested values

def _build_values(self, init_kwargs: Dict[str, Any]) -> Dict[str, Any]:
    return {**self._build_environ(), **init_kwargs}

but dictionaries could be merged recursively, so users could provide values for any of their settings in environment variables.

What do you think, would it be useful?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Oct 11, 2019

Yeah this seems like a good feature request to me.

I've never tried nesting BaseSettings classes but it seems totally reasonable to me and I don't see any reason it shouldn't work like this. (But maybe @samuelcolvin does?)

Could you produce a failing test case or two as a starting point for implementation work?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

@samuelcolvin samuelcolvin commented Oct 11, 2019

Sounds good to me.

Do you want to use dot notation e.g. db.host?

Also should this work for dictionaries too?

This isn't required for v1 since it would be entirely backwards compatible.

@idmitrievsky

This comment has been minimized.

Copy link
Contributor Author

@idmitrievsky idmitrievsky commented Oct 11, 2019

It's great to hear!

I think the only logic that needs to change is merging of self._build_environ() and init_kwargs dictionaries, so I don't see why it wouldn't work for dictionaries.

Should I start a merge request with the changes and tests or would you like to tackle this one yourselves?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Oct 11, 2019

@idmitrievsky It would be great if you could start a pull request for this. Your approach to the logic sounds right to me (though admittedly there could be some edge cases I'm not considering).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.