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

Enable streamlined upgrades with local.py #408

Closed
wants to merge 1 commit into from
Closed

Enable streamlined upgrades with local.py #408

wants to merge 1 commit into from

Conversation

cpeel
Copy link

@cpeel cpeel commented Oct 7, 2022

Improve the k8s deployment experience by making override updates to settings/local.py rather than changing the entire settings/config.py, like was done in the helm templates referenced in #317. A single container can now be built for use in dev/stg/prd environments and use an updated helm manifest to override values that differ in each env. This also improves the dev & upgrade experience by having local settings not pollute code under source control.

Pros/cons of this approach:

  • Pro:
    • Backwards compatible (users can continue to make changes in config.py if they want).
    • Optional -- if there is no local.py everything continues to work.
    • Re-uses established pattern of using a python file that does not exist in the repo (eg secrets.py).
  • Cons:
    • Changes the suggested location of where configurations are set (local.py vs config.py) requiring several doc updates.
    • Users must copy/paste/change the settings from config.py into local.py.

Other approaches considered:

  • We could move to a config file or env-based approach. The challenge would be lots of changes to config.py to optionally read every variable from the file or env before falling back to a default/initial value. This approach is also challenging for representing non-string / non-numeric configuration settings (booleans, lists).
  • All of this is technically not necessary as users can just write config values into secret.py and have them picked up and override config.py. This hacky approach convolutes the separation of secrets vs configuration data.

Improve the upgrade experience by having users configure settings by
making override updates to settings/local.py rather than changing
settings/config.py. This improves the k8s deployment experience by
only setting values we want to override rather than the entire
config.py.
@vsoch
Copy link
Member

vsoch commented Oct 7, 2022

We could move to a config file or env-based approach. The challenge would be lots of changes to config.py to optionally read every variable from the file or env before falling back to a default/initial value. This approach is also challenging for representing non-string / non-numeric configuration settings (booleans, lists).

I think I actually really like this approach? I made the original sregistry a long time ago, but since then I've made (by default) most of my Django apps read everything in from either a config file or the environment. E.g., here is an example that allows loading in from a config.yaml or the environment:

https://github.com/spack/spack-monitor/blob/89acf94dc664b598d9f73292ae4d61fdccf3ac5b/spackmon/settings.py#L44-L55

And then the defaults could be set in multiple ways, e.g., here is an example (for a Django plugin) that has a defaults structure and then can override it (in our case it would be from a settings file or environment):

https://github.com/vsoch/django-river-ml/blob/67bea67770809e75323a68cac660f528e673f5d2/django_river_ml/settings.py#L31-L102

What do you think?

@vsoch
Copy link
Member

vsoch commented Oct 7, 2022

hey @cpeel ! Here is an idea to throw into the mix for envars: #409. Its a bit different than what I've done before (I wanted more control over types) but I think it could be a good start? If you want to test out the branch (I won't get to before the work day starts) we can iteratively work on it (if you think it is a good direction).

@cpeel
Copy link
Author

cpeel commented Oct 9, 2022

Closing in lieu of #409

@cpeel cpeel closed this Oct 9, 2022
@cpeel cpeel deleted the improved-local-configs branch October 9, 2022 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants