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

Experiment: multi-environment configuration in Python #161

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jamesbursa
Copy link
Contributor

Ticket

Changes

  • Add new src.config module that defines config for each environment entirely in Python.
  • Modify startup code to load that config and pass it to each module that needs it.
  • Update tests to match (remove most monkeypatching of environ).

Context for reviewers

Start with default.py and local.py to understand how this works.

Goals of this approach:

  1. Developer friendly with minimal pitfalls - find out at test time if a config option is not set or invalid type for any environment
  2. Almost all settings are in python code instead of split between infrastructure (terraform / env vars) and code
  3. Still possible to override a setting from an env var if needed (for secrets or runtime overrides)
  4. Same source of settings when running in docker-compose, native, within IDE, or running the test suite
  5. Tests aren't accidentally affected by env vars that you have locally

Testing

Note: some tests are still broken.

Comment on lines 26 to 27
all_configs = src.config.load_all()
logger.info("loaded all", extra={"all_configs": all_configs})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code should move to a unit test, where it checks that each one is an instance of RootConfig.

Comment on lines 17 to 26
def default_config():
return RootConfig(
app=AppConfig(),
database=PostgresDBConfig(),
logging=LoggingConfig(
format="json",
level="INFO",
enable_audit=True,
),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this require any process that runs to be able to load all environment variables? For example, if I have a config for connecting to a separate service, and it's only used in a few select processes, would I put that here in the default config? Let's assume that I don't want to set defaults in the config and want it to error if any fields aren't set.

Comment on lines 1 to 16
#
# Configuration for local development environments.
#
# This file only contains overrides (differences) from the defaults in default.py.
#

import pydantic.types

from .. import default

config = default.default_config()

config.database.password = pydantic.types.SecretStr("secret123")
config.database.hide_sql_parameter_logs = False
config.logging.format = "human_readable"
config.logging.enable_audit = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be a way to have this check for a file named local_overrides.py and use that? I'm thinking about local testing where I want to tinker with environment variables, but not check them in - especially when working with passwords or keys to access services. We could add this local_overrides.py file to the .gitignore.

Adding something like this to the end of this file:

if os.path.exists("local_overrides.py"):
     importlib.import_module("local_overrides") # whatever the right path/params are

@@ -2,7 +2,7 @@


class AppConfig(PydanticBaseEnvConfig):
environment: str
environment: str = "unknown"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe environment: Optional[str] = None? That might be annoying with the linter though

continue

env_var_name = field.field_info.extra.get("env", prefix + name)
for key in (env_var_name, env_var_name.lower(), env_var_name.upper()):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to be stricter here and require a certain casing, avoid letting someone accidentally set the environment variable in lowercase and then not understand why it's not being overridden if it's also set in uppercase elsewhere.

Comment on lines -14 to +15
class PydanticBaseEnvConfig(BaseSettings):
class Config:
env_file = env_file
validate_assignment = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Could require a prefix option here too to provide some namespacing for the various configs, e.g., have the PostgresConfig environment variables all start with a PG_DB_ prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

(The implementation on PFML does this)

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.

3 participants