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 loading settings from outside of the postgresqleu tree #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mwanner
Copy link

@mwanner mwanner commented Nov 24, 2024

To ease building docker images and prevent having to maintain an
in-tree file for local settings, allow a pgeu_system_settings
module anywhere in the PYTHONPATH for configuration.

In addition, also allow overrides to be applied after loading the
skin.

To ease building docker images and prevent having to maintain an
in-tree file for local settings, allow a pgeu_system_settings
module anywhere in the PYTHONPATH for configuration.

In addition, also allow overrides to be applied after loading the
skin.
@mhagander
Copy link
Member

I like this idea in general. In fact, I think something like this can be used to significantly simplify some of the hacks around the deployment we do on pginfra as well! But I think the ordering s wrong :)

Today we have:

  1. local_settings
  2. skin_settings
  3. skin_local_settings (which I think are similar to your override ones)

I think what you're adding is basically "site settings", so shouldn't they be loaded before local_settings?

In a lot of ways, local_settings should be loaded last. The reason they're not loaded last toay is that local_settings is where you define the SKIN_DIRECTORY. So there's kind of a chicken and egg situation there. I think your idea of splitting it into two makes a lot of sense -- but it should then also be done for local_settings, as in it should have a similar override:ability.

So I'm thinking maybe something like this (open for discussions):

  1. pgeu_system_settings module
  2. local_settings.py (needs to be early to set skin settings)
  3. if skin then skin_settings
  4. if skin then skin_local_settings
  5. pgeu_system_override_settings module
  6. local_settings_override.py

Or something like that. If seems wrong that "system settings" would override "local settings"...

@mwanner
Copy link
Author

mwanner commented Nov 27, 2024

Thanks for your consideration.

So I'm thinking maybe something like this (open for discussions):

1. pgeu_system_settings module

2. local_settings.py (needs to be early to set skin settings)

3. if skin then skin_settings

4. if skin then skin_local_settings

5. pgeu_system_override_settings module

6. local_settings_override.py

I agree and like this order. I'll try to adjust the PR accordingly.

Or something like that. If seems wrong that "system settings" would override "local settings"...

This could be more of a naming issue. The entire project is called pgeu_system, so the system part is more of the original same (the python module is called postgresqleu, though, maybe I should use that instead). The intent for these was just to be override settings. E.g. to override ENABLE_PG_COMMUNITY_AUTH that a skin sets, but you may not want to enable for a local test setup during development.

Do we need two different override modules?

To avoid bike-shedding as much as possible, I tend to just s/pgeu_system/postgresqleu/ as the base name. And go with settings, local_settings, and override_settings. Times two, if we really need a local vs system wide variant for each.

@mhagander
Copy link
Member

Or something like that. If seems wrong that "system settings" would override "local settings"...

This could be more of a naming issue. The entire project is called pgeu_system, so the system part is more of the original same (the python module is called postgresqleu, though, maybe I should use that instead). The intent for these was just to be override settings. E.g. to override ENABLE_PG_COMMUNITY_AUTH that a skin sets, but you may not want to enable for a local test setup during development.

Oh. Gotcha. I did not realize it came from that :) With that it makes a lot more sense.

Do we need two different override modules?

Maybe? When deploying to multiple different servers (like we do for pginfra for example), having the module mentioned in point 1 above makes sense. It would be a good place to put things like logging configurations.

And then the usecase that you mention have use for the module in point 5?

To avoid bike-shedding as much as possible, I tend to just s/pgeu_system/postgresqleu/ as the base name. And go with settings, local_settings, and override_settings. Times two, if we really need a local vs system wide variant for each.

I don't think changing the base to postgresqleu is a good idea because for historical reasons that's the base name of the modules in the code itself. It's better to keep them apart.

So maybe pgeu_system_override is the right thing. And maybe make the other one pgeu_system_global_settings or something, to indicate it's loaded on top before the others?

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.

2 participants