Allow runtime settings overrides #70

PeterJCLaw · 2022-08-06T20:57:42Z

Summary

This builds on #62 to introduce support for Django settings being overridden at runtime. While true changes to settings at runtime are not supported, this aims to simplify testing (django.test.override_settings should now work) and allow for settings architectures which do not pin down settings values immediately.

Consumers must ensure that settings do not change once the system has started however as this could lead to unpredictable behaviours in response. (For example it is possible that queues could have incorrect numbers of workers assigned)

I've rebased this on top of #66 so the tests that introduces help validate that extra-config loading works as expected and updated those tests to work with this new approach.

Code review

I suggest starting with the changes to app_settings.py, then looking at utils.py and test_extra_config.py.
The remainder of the changes are small adaptations to account for the slightly changed internal API and moving tests to use override_settings now that works.

Co-authored-by: Peter Law <[email protected]>

Introduce an long-name adapter which facades away the name munging logic and allows re-use of this both for Django settings and custom settings. This simplifies AppSettings which can now focus on just layering the settings. While this doesn't gain any type checking on the settings from the layers, that was never present anyway.

The previous approach of reloading the settings module worked ok when the module itself was what was imported & mutated, however with the move to a layered approach that's no longer the case.

This slightly changes the behaviour of the mock_workers helpers, however the default is an empty dictionary so this feels like a reasonable trade-off for using the more standard override_settings util.

lirsacc · 2023-06-14T14:39:20Z

django_lightweight_queue/app_settings.py


-# Allow per-queue overrides of the backend.
-BACKEND_OVERRIDES = setting('BACKEND_OVERRIDES', {})  # type: Mapping[QueueName, str]
+class AppSettings:


Given this is all internal, should't this inherit from Settings? This would avoid having to use cast in django_lightweight_queue/utils.py.

I could see some concern about having the app_settings value being more explicitly mutable by adding layers, but it already is in practice (even if the typing says no) so probably not a concern here? If it is I reckon AppSettings could be made immutable and adding a layer just returns a new instance instead of doing interior mutability.

Scratch that the point in utils is to mutate the global settings so yeah I think making this an explicit part of the API is probably better.

Sorry it's taken a while to get back to this. Are you suggesting that add_layer should be part of the public interface? I had avoided doing that since the consequences of calling it arbitrarily aren't well defined. That said, this is meant to be internal to this package anyway, so maybe it's ok? I'll do that and add a comment clarifying that this file is internal API.

lirsacc · 2023-06-14T14:43:00Z

django_lightweight_queue/utils.py

-    for name in override_names:
-        short_name = name[len(constants.SETTING_NAME_PREFIX):]
-        setattr(app_settings, short_name, getattr(extra_settings, name))
+    cast(AppSettings, app_settings).add_layer(LongNameAdapter(extra_settings))


See comment in app_settings.py - I reckon we can make this work more cleanly without a cast.

lirsacc · 2023-06-14T15:01:52Z

Consumers must ensure that settings do not change once the system has started however as this could lead to unpredictable behaviours in response. (For example it is possible that queues could have incorrect numbers of workers assigned)

This feels unlikely to be a huge problem but one idea to make this safe:

Make app_settings.app_settings a context var always accessed through the context, not as a constant
Add a locked() or other way to make it immutable (not sure of the API, but at it's core this would be making a copy of the django settings at a point in time.
Wrap all code qualifying as the system has started (mostly the management commands? or some layer just below) with a nested context setting the immutable version. Any calls to override_settings below that would essentially noop.

The caveat is that for when someone really wants to do this then we're back to override_settings not working as expected.

PeterJCLaw · 2023-06-15T22:00:27Z

Consumers must ensure that settings do not change once the system has started however as this could lead to unpredictable behaviours in response. (For example it is possible that queues could have incorrect numbers of workers assigned)

This feels unlikely to be a huge problem but one idea to make this safe:

Make app_settings.app_settings a context var always accessed through the context, not as a constant

Add a locked() or other way to make it immutable (not sure of the API, but at it's core this would be making a copy of the django settings at a point in time.

Wrap all code qualifying as the system has started (mostly the management commands? or some layer just below) with a nested context setting the immutable version. Any calls to override_settings below that would essentially noop.

The caveat is that for when someone really wants to do this then we're back to override_settings not working as expected.

So the issue here really is that calls to override_settings merely modify one of the layers, in a manner which this app has no control over. (Direct modification of settings.LIGHTWEIGHT_QUEUE_WHATEVER behaves similarly).

It's not that changes could be made and that might break things, but rather that changes could be made and we have no reasonable way to detect them and update the system to reflect them. This was already a problem (e.g: the WORKERS dict could already have been modified at runtime, with potentially interesting effects), there's just slightly more nuance to it now.

I don't feel I understand how adding contextvars or freezing the settings would help here?

This avoids needing to cast the settings by instead documenting the expected bounds of the API offered.

itsthejoker and others added 18 commits August 6, 2022 21:21

resolve conflicts

f40154f

make all settings changeable at runtime

7cce4c4

make all settings changeable at runtime, try 2

548cb6e

fix missing type hint and revert name to app_settings

8fe3951

revert version number update

4601ca3

run isort

fe81a32

move readme changes to own PR

caecc02

appease mypy with appropriate blood-based rituals

58663f7

revert mock to use .patch.dict()

cd8dc9e

Refactor for layer approach

dcbeb01

add type hints for getattr

5ae6b62

fix support for old python

0b64c18

refactor shortening correctly

0b47d54

re-add accidentally-dropped comment

be04a45

Update django_lightweight_queue/app_settings.py

30676c6

Co-authored-by: Peter Law <[email protected]>

Update django_lightweight_queue/app_settings.py

5464e66

Co-authored-by: Peter Law <[email protected]>

Update the extra-config tests for the new way that settings behave

ccd9992

The previous approach of reloading the settings module worked ok when the module itself was what was imported & mutated, however with the move to a layered approach that's no longer the case.

PeterJCLaw changed the base branch from master to extra-config-use-importlib August 6, 2022 20:57

PeterJCLaw force-pushed the allow-runtime-settings-overrides branch from 442a4d7 to 154b929 Compare August 6, 2022 21:07

Use override_settings in tests where we can now do so

31eb204

This slightly changes the behaviour of the mock_workers helpers, however the default is an empty dictionary so this feels like a reasonable trade-off for using the more standard override_settings util.

PeterJCLaw force-pushed the allow-runtime-settings-overrides branch from 154b929 to 31eb204 Compare August 6, 2022 21:10

Wrap longish line for clarity

f86a29f

Base automatically changed from extra-config-use-importlib to master August 15, 2022 15:15

Merge branch 'master' into allow-runtime-settings-overrides

f1e12d6

PeterJCLaw marked this pull request as ready for review October 17, 2022 10:24

PeterJCLaw requested a review from aaron7 October 17, 2022 12:51

itsthejoker mentioned this pull request Dec 9, 2022

docker localdev AlexandriaILS/Alexandria#51

Open

6 tasks

Merge branch 'master' into allow-runtime-settings-overrides

5f5acbe

PeterJCLaw-mns force-pushed the allow-runtime-settings-overrides branch from 16b78f0 to 5f5acbe Compare January 27, 2023 16:36

Merge branch 'master' into allow-runtime-settings-overrides

c8e1502

lirsacc approved these changes Jun 14, 2023

View reviewed changes

Clarify API boundaries and remove cast

80d2fd4

This avoids needing to cast the settings by instead documenting the expected bounds of the API offered.

PeterJCLaw-mns merged commit b235120 into master Dec 14, 2023
20 checks passed

PeterJCLaw-mns deleted the allow-runtime-settings-overrides branch December 14, 2023 14:47

PeterJCLaw-mns mentioned this pull request Dec 14, 2023

Expand Settings to handle different Django Architectures #62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow runtime settings overrides #70

Allow runtime settings overrides #70

PeterJCLaw commented Aug 6, 2022 •

edited

Loading

lirsacc Jun 14, 2023

PeterJCLaw Dec 12, 2023

PeterJCLaw Dec 12, 2023

lirsacc Jun 14, 2023

PeterJCLaw Dec 12, 2023

lirsacc commented Jun 14, 2023

PeterJCLaw commented Jun 15, 2023

Allow runtime settings overrides #70

Allow runtime settings overrides #70

Conversation

PeterJCLaw commented Aug 6, 2022 • edited Loading

Summary

Code review

lirsacc Jun 14, 2023

Choose a reason for hiding this comment

PeterJCLaw Dec 12, 2023

Choose a reason for hiding this comment

PeterJCLaw Dec 12, 2023

Choose a reason for hiding this comment

lirsacc Jun 14, 2023

Choose a reason for hiding this comment

PeterJCLaw Dec 12, 2023

Choose a reason for hiding this comment

lirsacc commented Jun 14, 2023

PeterJCLaw commented Jun 15, 2023

PeterJCLaw commented Aug 6, 2022 •

edited

Loading