-
Notifications
You must be signed in to change notification settings - Fork 6
Allow runtime settings overrides #70
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
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
f40154f
resolve conflicts
itsthejoker 7cce4c4
make all settings changeable at runtime
itsthejoker 548cb6e
make all settings changeable at runtime, try 2
itsthejoker 8fe3951
fix missing type hint and revert name to app_settings
itsthejoker 4601ca3
revert version number update
itsthejoker fe81a32
run isort
itsthejoker caecc02
move readme changes to own PR
itsthejoker 58663f7
appease mypy with appropriate blood-based rituals
itsthejoker cd8dc9e
revert mock to use .patch.dict()
itsthejoker dcbeb01
Refactor for layer approach
itsthejoker 5ae6b62
add type hints for getattr
itsthejoker 0b64c18
fix support for old python
itsthejoker 0b47d54
refactor shortening correctly
itsthejoker be04a45
re-add accidentally-dropped comment
itsthejoker 30676c6
Update django_lightweight_queue/app_settings.py
itsthejoker 5464e66
Update django_lightweight_queue/app_settings.py
itsthejoker 33554ef
Reinstate typing by adding a settings protocol for local use
PeterJCLaw ccd9992
Update the extra-config tests for the new way that settings behave
PeterJCLaw 31eb204
Use override_settings in tests where we can now do so
PeterJCLaw f86a29f
Wrap longish line for clarity
PeterJCLaw f1e12d6
Merge branch 'master' into allow-runtime-settings-overrides
PeterJCLaw 5f5acbe
Merge branch 'master' into allow-runtime-settings-overrides
PeterJCLaw c8e1502
Merge branch 'master' into allow-runtime-settings-overrides
PeterJCLaw 80d2fd4
Clarify API boundaries and remove cast
PeterJCLaw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,53 +1,103 @@ | ||
from typing import Dict, Union, Mapping, TypeVar, Callable, Optional, Sequence | ||
""" | ||
Internal API facade over Django settings. | ||
|
||
from django.conf import settings | ||
This provides a typed interface over Django settings as well as handling default | ||
values for settings not set by users and supporting the addition of overrides in | ||
some management commands. | ||
""" | ||
|
||
from typing import Any, Dict, List, Union, Callable, Optional, Sequence | ||
|
||
from typing_extensions import Protocol | ||
|
||
from django.conf import settings as django_settings | ||
|
||
from . import constants | ||
from .types import Logger, QueueName | ||
|
||
T = TypeVar('T') | ||
|
||
class LongNameAdapter: | ||
def __init__(self, target: Any) -> None: | ||
self.target = target | ||
|
||
def __getattr__(self, name: str) -> Any: | ||
return getattr(self.target, f'{constants.SETTING_NAME_PREFIX}{name}') | ||
|
||
|
||
class Settings(Protocol): | ||
WORKERS: Dict[QueueName, int] | ||
BACKEND: str | ||
LOGGER_FACTORY: Union[str, Callable[[str], Logger]] | ||
|
||
# Allow per-queue overrides of the backend. | ||
BACKEND_OVERRIDES: Dict[QueueName, str] | ||
MIDDLEWARE: Sequence[str] | ||
|
||
# Apps to ignore when looking for tasks. Apps must be specified as the dotted | ||
# name used in `INSTALLED_APPS`. This is expected to be useful when you need to | ||
# have a file called `tasks.py` within an app, but don't want | ||
# django-lightweight-queue to import that file. | ||
# Note: this _doesn't_ prevent tasks being registered from these apps. | ||
IGNORE_APPS: Sequence[str] | ||
|
||
REDIS_HOST: str | ||
REDIS_PORT: int | ||
REDIS_PASSWORD: Optional[str] | ||
REDIS_DATABASE: int | ||
REDIS_PREFIX: str | ||
|
||
ENABLE_PROMETHEUS: bool | ||
# Workers will export metrics on this port, and ports following it | ||
PROMETHEUS_START_PORT: int | ||
|
||
ATOMIC_JOBS: bool | ||
|
||
|
||
class LayeredSettings(Settings, Protocol): | ||
def add_layer(self, layer: Settings) -> None: | ||
... | ||
|
||
|
||
class Defaults(Settings): | ||
WORKERS: Dict[QueueName, int] = {} | ||
BACKEND = 'django_lightweight_queue.backends.synchronous.SynchronousBackend' | ||
LOGGER_FACTORY = 'logging.getLogger' | ||
|
||
BACKEND_OVERRIDES: Dict[QueueName, str] = {} | ||
MIDDLEWARE = ('django_lightweight_queue.middleware.logging.LoggingMiddleware',) | ||
|
||
IGNORE_APPS: Sequence[str] = () | ||
|
||
REDIS_HOST = '127.0.0.1' | ||
REDIS_PORT = 6379 | ||
REDIS_PASSWORD = None | ||
REDIS_DATABASE = 0 | ||
REDIS_PREFIX = "" | ||
|
||
def setting(suffix: str, default: T) -> T: | ||
attr_name = '{}{}'.format(constants.SETTING_NAME_PREFIX, suffix) | ||
return getattr(settings, attr_name, default) | ||
ENABLE_PROMETHEUS = False | ||
|
||
PROMETHEUS_START_PORT = 9300 | ||
|
||
WORKERS = setting('WORKERS', {}) # type: Dict[QueueName, int] | ||
BACKEND = setting( | ||
'BACKEND', | ||
'django_lightweight_queue.backends.synchronous.SynchronousBackend', | ||
) # type: str | ||
ATOMIC_JOBS = True | ||
|
||
LOGGER_FACTORY = setting( | ||
'LOGGER_FACTORY', | ||
'logging.getLogger', | ||
) # type: Union[str, Callable[[str], Logger]] | ||
|
||
# Allow per-queue overrides of the backend. | ||
BACKEND_OVERRIDES = setting('BACKEND_OVERRIDES', {}) # type: Mapping[QueueName, str] | ||
class AppSettings: | ||
def __init__(self, layers: List[Settings]) -> None: | ||
self._layers = layers | ||
|
||
MIDDLEWARE = setting('MIDDLEWARE', ( | ||
'django_lightweight_queue.middleware.logging.LoggingMiddleware', | ||
'django_lightweight_queue.middleware.transaction.TransactionMiddleware', | ||
)) # type: Sequence[str] | ||
def add_layer(self, layer: Settings) -> None: | ||
self._layers.append(layer) | ||
|
||
# Apps to ignore when looking for tasks. Apps must be specified as the dotted | ||
# name used in `INSTALLED_APPS`. This is expected to be useful when you need to | ||
# have a file called `tasks.py` within an app, but don't want | ||
# django-lightweight-queue to import that file. | ||
# Note: this _doesn't_ prevent tasks being registered from these apps. | ||
IGNORE_APPS = setting('IGNORE_APPS', ()) # type: Sequence[str] | ||
def __getattr__(self, name: str) -> Any: | ||
# reverse so that later layers override earlier ones | ||
for layer in reversed(self._layers): | ||
if hasattr(layer, name): | ||
return getattr(layer, name) | ||
|
||
# Backend-specific settings | ||
REDIS_HOST = setting('REDIS_HOST', '127.0.0.1') # type: str | ||
REDIS_PORT = setting('REDIS_PORT', 6379) # type: int | ||
REDIS_PASSWORD = setting('REDIS_PASSWORD', None) # type: Optional[str] | ||
REDIS_DATABASE = setting('REDIS_DATABASE', 0) # type: int | ||
REDIS_PREFIX = setting('REDIS_PREFIX', '') # type: str | ||
raise AttributeError(f"Sorry, '{name}' is not a valid setting.") | ||
|
||
ENABLE_PROMETHEUS = setting('ENABLE_PROMETHEUS', False) # type: bool | ||
# Workers will export metrics on this port, and ports following it | ||
PROMETHEUS_START_PORT = setting('PROMETHEUS_START_PORT', 9300) # type: int | ||
|
||
ATOMIC_JOBS = setting('ATOMIC_JOBS', True) # type: bool | ||
app_settings: LayeredSettings = AppSettings(layers=[ | ||
Defaults(), | ||
LongNameAdapter(django_settings), | ||
]) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
django_lightweight_queue/management/commands/queue_configuration.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is all internal, should't this inherit from
Settings
? This would avoid having to usecast
indjango_lightweight_queue/utils.py
.I could see some concern about having theapp_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 reckonAppSettings
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
80d2fd4