search

singularityhub / sregistry

server for storage and management of singularity images
https://singularityhub.github.io/sregistry
Mozilla Public License 2.0
103 stars 42 forks source link

Allowing setting config vars from environment #409

Closed vsoch closed 1 year ago

vsoch commented 1 year ago

This is an idea to address the same issue brought up in #408 - we want to be able to easily set variables in settings from the environment. This currently just includes values in config.py (and currently can be extended). I decided to explicitly list the names and types to have better control (e.g., instead of iterating over locals().

Signed-off-by: vsoch vsochat@stanford.edu

vsoch commented 1 year ago

@cpeel totally agree - let me rework this a bit (probably after work) to see if I can make it a neater solution. I don't like the redundancy either!

vsoch commented 1 year ago

@cpeel I've been considering this for some time - but it might bet time to migrate the settings folder into a single settings.py to make this more clear, generally. What do you think? I'm happy to take the first shot (and I'll refactor this PR along with it!)

cpeel commented 1 year ago

I'm not sure refactoring it into a single settings.py is required. The way you've done the importing via __init__.py is nice and it lets them all be "local" to settings.py but logically grouped. If you moved the overrides into __init__.py after the imports rather than in config.py that would ensure that the overrides apply to any file.

One way to decrease the redundancy in this approach might be to iterate over all env settings and pull out all of them with SREGISTRY_ prefixes. If there is a matching local variable, set it. If it is prefixed by 'ENABLEorDISABLE` treat it as a boolean. Otherwise try to cast it as an int, otherwise treat it as a string. This has the potential to be more error prone -- and I would need to review all of the existing settings to see if this would work -- but it's extensible and removes the redundancy. We'd still need to special-case the lists in the code, but those are actually pretty few. An idea anyway.

Another possible approach would be to have the settings in a JSON file that is specified via env variable (SREGISTRY_CONFIG_FILE). The JSON would allow the user to be explicit about the type and take care of things like lists, dicts, etc. So the override code would open and iterate over the JSON and set local settings to their JSON values. In k8s we'd just create the JSON file in the manifest and pass in the env variable to pick it up. The more I type the more I like that idea.

vsoch commented 1 year ago

Another possible approach would be to have the settings in a JSON file that is specified via env variable (SREGISTRY_CONFIG_FILE). The JSON would allow the user to be explicit about the type and take care of things like lists, dicts, etc. So the override code would open and iterate over the JSON and set local settings to their JSON values. In k8s we'd just create the JSON file in the manifest and pass in the env variable to pick it up. The more I type the more I like that idea.

I also like the idea of having this as an option, although not required. And I'm also leaning towards making everything very flexible - e.g., "provide the value directly in the file, via the config file, or environment - up to you!"

Is JSON preferred to YAML? I think (for configs like this) my preference would be YAML, but maybe my config-ometer sense is off. :laughing:

vsoch commented 1 year ago

okay will have a PR update for you this evening!

cpeel commented 1 year ago

Is JSON preferred to YAML? I think (for configs like this) my preference would be YAML, but maybe my config-ometer sense is off.

YAML is probably better (says my professional half -- my personal half thinks YAML is rubbish 😁 ).

vsoch commented 1 year ago

YAML is probably better (says my professional half -- my personal half thinks YAML is rubbish grin ).

Haha, I totally feel that!

So I just pushed a refactor that has a single settings.py (to give more confidence we find everything in one place) and it should work to define variables from:

https://github.com/singularityhub/sregistry/pull/409/commits/9b25c8cd26fa62c2f7af2d2866167b656d1bfd1b

I decided to still have tight control over variables and placed them into groups based on type, as oppose to a strategy that looks for any SREGISTRY_ prefix in the environment and then tries to guess. It's an improvement on my first shot because we remove the redundancy. If we want to expose more customization (e.g., in the various dicts in settings.py) that should be fairly easy to do.

The one change I'm still thinking about doing is not having any "special prefix" for secrets as I do now (e.g., SREGISTRY_SECRET_. I was thinking that might be useful for the developer - so they can see based on the name that something should be treated more carefully. I'm thinking maybe it makes sense to try out this branch for your use case, and we can tweak / change as you see issues arise?

vsoch commented 1 year ago

okay (barring no issues with building) another round of changes! https://github.com/singularityhub/sregistry/pull/409/commits/854b7ab9be83c44f4ded920d7509cc43ca155b1b

The only detail we have to be careful about now is that values that are expected to be in settings (e.g., GOOGLE_ANALYTICS that can still be set to undefined are provided as empty string "" and not None - Nones are assumed to be unset and won't be added to the settings locals, whereas an empty string will.

vsoch commented 1 year ago

Note to self: don't look back at old code, because it undoubtedly will look terrible and you'll want to change everything to make it look nicer! :sob: (true story, just now...) :laughing:

Thanks for the back and forth today @cpeel it was fun!

cpeel commented 1 year ago

Note to self: don't look back at old code, because it undoubtedly will look terrible and you'll want to change everything to make it look nicer! 😭 (true story, just now...) 😆

lol - I've been doing software development for 22 years and this only gets worse! On the plus side this means that we're becoming better software developers!

Thanks for the back and forth today @cpeel it was fun!

Same! Appreciate all of your work in this PR! I will give this a test run ASAP. Tomorrow is busy for me so might be Tuesday.

vsoch commented 1 year ago

@cpeel I'll get these final tweaks in tonight - I don't typically commit to personal projects during the work day!

vsoch commented 1 year ago

okay final two changes in! https://github.com/singularityhub/sregistry/pull/409/commits/b9335b6d7f29583579daccdbdc798419228daf5b