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

Merging settings makes it almost impossible to fully override settings #2284

Closed
alexdutton opened this issue Jul 5, 2021 · 3 comments · Fixed by #2294
Closed

Merging settings makes it almost impossible to fully override settings #2284

alexdutton opened this issue Jul 5, 2021 · 3 comments · Fixed by #2294
Assignees
Labels
bug Something's not working

Comments

@alexdutton
Copy link
Contributor

Describe the bug

In load_janeway_settings the settings referred to by DJANGO_SETTINGS_MODULE are merged into the base core.janeway_global_settings.

The method for merging involves appending to any existing sequence settings, and applying new keys to existing dictionaries. This means it's almost impossible to completely override sequence- or dict-based settings, and this behaviour is also potentially counter-intuitive.

For example, my attempt to insert a new middleware class into the existing Janeway default middleware list results in the default middleware being included twice:

from core import janeway_global_settings

MIDDLEWARE_CLASSES = list(janeway_global_settings.MIDDLEWARE_CLASSES)
MIDDLEWARE_CLASSES.insert(
    MIDDLEWARE_CLASSES.index("django.middleware.security.SecurityMiddleware") + 1,
    "whitenoise.middleware.WhiteNoiseMiddleware",
)

Janeway version

This is on master, currently 7739efb.

Expected behavior

One of three things:

  • A setting in DJANGO_SETTINGS_MODULE completely overrides the corresponding setting in global_janeway_settings, but the current custom configure_settings() behaviour continues, or
  • The current configure_settings() approach is removed, and from core.global_janeway_settings import * can be used in DJANGO_SETTINGS_MODULE to the same effect, making things more explicit, with no backwards-compatibility, or
  • load_janeway_settings could detect whether global_janeway_settings is *-imported into DJANGO_SETTINGS_MODULE — as the new preferred approach — and disable the settings-merging behavour. This would provide backwards-compatibility for the current behaviour.
@alexdutton alexdutton added the bug Something's not working label Jul 5, 2021
@mauromsl
Copy link
Member

mauromsl commented Jul 8, 2021

I think the best approach is a combination of A and B, by moving the list of settings to be merged from the base installation to the actual settings file.. let me draft a PR

@ajrbyers ajrbyers added this to the v1.4.1 milestone Oct 26, 2021
@mauromsl mauromsl self-assigned this Nov 11, 2021
@ajrbyers ajrbyers added this to v1.4.1 Nov 18, 2021
@ajrbyers ajrbyers modified the milestones: v1.4.1, 1.4.2 Nov 18, 2021
@ajrbyers ajrbyers removed this from v1.4.1 Nov 18, 2021
@ajrbyers ajrbyers added this to v1.4.2 Nov 18, 2021
@joemull joemull moved this to Todo in v1.4.2 Feb 3, 2022
@ajrbyers ajrbyers added this to v1.4.3 Jun 9, 2022
@ajrbyers ajrbyers moved this to Todo in v1.4.3 Jun 9, 2022
@ajrbyers ajrbyers removed this from v1.4.2 Jun 9, 2022
@mauromsl mauromsl moved this from Todo to PR Submitted in v1.4.3 Sep 7, 2022
@ajrbyers ajrbyers removed this from the 1.4.2 milestone Sep 21, 2022
@ajrbyers ajrbyers removed this from v1.4.3 Sep 21, 2022
@ajrbyers ajrbyers moved this to Todo in Old v1.6 Sep 21, 2022
@joemull joemull removed this from Old v1.6 Sep 22, 2022
@joemull joemull moved this to Todo in v1.5 Torres Sep 22, 2022
@ajrbyers ajrbyers removed this from v1.5 Torres Mar 27, 2023
@ajrbyers
Copy link
Member

I've re-assigned this to v1.5.1

@ajrbyers ajrbyers moved this to Todo in v1.5.1 Archer May 9, 2023
@ajrbyers ajrbyers added this to Old v1.6 Jun 8, 2023
@ajrbyers ajrbyers removed this from v1.5.1 Archer Jun 8, 2023
@github-project-automation github-project-automation bot moved this to Todo in Old v1.6 Jun 8, 2023
@mauromsl mauromsl removed this from Old v1.6 Feb 7, 2024
@mauromsl mauromsl added this to v1.5.2 Feb 7, 2024
@mauromsl mauromsl moved this to In Progress in v1.5.2 Feb 7, 2024
mauromsl added a commit that referenced this issue Mar 12, 2024
@mauromsl
Copy link
Member

Closed by #2294

@github-project-automation github-project-automation bot moved this from In Progress to Done in v1.5.2 Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something's not working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants