-
Notifications
You must be signed in to change notification settings - Fork 0
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
setup_notification command #2
Changes from 9 commits
de81826
a68eabb
fdba99b
3ac524b
261a73e
2cc9bae
6c047d7
bda38bb
a47235e
16d3330
67dabde
674ea6a
723a014
c17c48a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,4 +30,4 @@ jobs: | |
run: pip install tox | ||
- run: tox | ||
env: | ||
TOXENV: $ | ||
TOXENV: ${{ matrix.toxenv }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
from abc import ABC, abstractmethod | ||
|
||
from django.conf import settings | ||
|
||
from .exceptions import PrerequisiteFailed | ||
|
||
|
||
class BaseConfigurationStep(ABC): | ||
verbose_name: str | ||
required_settings: list[str] = [] | ||
enable_setting: str = "" | ||
|
||
def __repr__(self): | ||
return self.verbose_name | ||
|
||
def validate_requirements(self) -> None: | ||
""" | ||
check prerequisites of the configuration | ||
|
||
:raises: :class: `django_setup_configuration.exceptions.PrerequisiteFailed` | ||
if prerequisites are missing | ||
""" | ||
missing = [ | ||
var for var in self.required_settings if not getattr(settings, var, None) | ||
] | ||
if missing: | ||
raise PrerequisiteFailed( | ||
f"{', '.join(missing)} settings should be provided" | ||
) | ||
|
||
def is_enabled(self) -> bool: | ||
""" | ||
Hook to switch on and off the configuration step from env vars | ||
|
||
By default all steps are enabled | ||
""" | ||
if not self.enable_setting: | ||
return True | ||
|
||
return getattr(settings, self.enable_setting, True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to do this via a Django setting step? Why can't we directly point to the name of an envvar and skip having to define a Django setting that reads the envvar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I use django settings to set defaults when it's possible. For example we can have a default for |
||
|
||
@abstractmethod | ||
def is_configured(self) -> bool: | ||
""" | ||
Check that the configuration is already done with current env variables | ||
""" | ||
... | ||
|
||
@abstractmethod | ||
def configure(self) -> None: | ||
""" | ||
Run the configuration step. | ||
|
||
:raises: :class: `django_setup_configuration.exceptions.ConfigurationRunFailed` | ||
if the configuration has an error | ||
""" | ||
... | ||
|
||
@abstractmethod | ||
def test_configuration(self) -> None: | ||
""" | ||
Test that the configuration works as expected | ||
|
||
:raises: :class:`openzaak.config.bootstrap.exceptions.SelfTestFailure` | ||
if the configuration aspect was found to be faulty. | ||
""" | ||
... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
class ConfigurationException(Exception): | ||
""" | ||
Base exception for configuration steps | ||
""" | ||
|
||
|
||
class PrerequisiteFailed(ConfigurationException): | ||
""" | ||
Raise an error then configuration step can't be started | ||
""" | ||
|
||
|
||
class ConfigurationRunFailed(ConfigurationException): | ||
""" | ||
Raise an error then configuration process was faulty | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/then/when? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
""" | ||
|
||
|
||
class SelfTestFailed(ConfigurationException): | ||
""" | ||
Raise an error for failed configuration self-tests. | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
from collections import OrderedDict | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python dicts retain insertion order, you generally don't need OrderedDict anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL! Changed to |
||
|
||
from django.conf import settings | ||
from django.core.management import BaseCommand, CommandError | ||
from django.db import transaction | ||
from django.utils.module_loading import import_string | ||
|
||
from ...configuration import BaseConfigurationStep | ||
from ...exceptions import ConfigurationRunFailed, PrerequisiteFailed, SelfTestFailed | ||
|
||
|
||
class ErrorDict(OrderedDict): | ||
""" | ||
small helper to display errors | ||
""" | ||
|
||
def as_text(self) -> str: | ||
output = [f"{k}: {v}" for k, v in self.items()] | ||
return "\n".join(output) | ||
|
||
|
||
class Command(BaseCommand): | ||
help = ( | ||
"Bootstrap the initial Open Zaak configuration. " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should remove Open Zaak from this docstring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed :) |
||
"This command is run only in non-interactive mode with settings " | ||
"configured mainly via environment variables." | ||
) | ||
output_transaction = True | ||
|
||
def add_arguments(self, parser): | ||
parser.add_argument( | ||
"--overwrite", | ||
action="store_true", | ||
help=( | ||
"Overwrite the existing configuration. Should be used if some " | ||
"of the env variables have been changed." | ||
), | ||
) | ||
parser.add_argument( | ||
"--no-selftest", | ||
action="store_true", | ||
dest="skip_selftest", | ||
help=( | ||
"Skip checking if configuration is successful. Use it if you " | ||
"run this command in the init container before the web app is started" | ||
), | ||
) | ||
|
||
@transaction.atomic | ||
def handle(self, **options): | ||
overwrite: bool = options["overwrite"] | ||
skip_selftest: bool = options["skip_selftest"] | ||
|
||
errors = ErrorDict() | ||
steps: list[BaseConfigurationStep] = [ | ||
import_string(path)() for path in settings.SETUP_CONFIGURATION_STEPS | ||
] | ||
enabled_steps = [step for step in steps if step.is_enabled()] | ||
|
||
if not enabled_steps: | ||
self.stdout.write( | ||
"There are no enabled configuration steps. " | ||
"Configuration can't be set up" | ||
) | ||
return | ||
|
||
self.stdout.write( | ||
f"Configuration would be set up with following steps: {enabled_steps}" | ||
annashamray marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
# 1. Check prerequisites of all steps | ||
for step in enabled_steps: | ||
try: | ||
step.validate_requirements() | ||
except PrerequisiteFailed as exc: | ||
errors[step] = exc | ||
|
||
if errors: | ||
raise CommandError( | ||
f"Prerequisites for configuration are not fulfilled: {errors.as_text()}" | ||
) | ||
|
||
# 2. Configure steps | ||
configured_steps = [] | ||
for step in enabled_steps: | ||
if not overwrite and step.is_configured(): | ||
self.stdout.write( | ||
f"Step {step} is skipped, because the configuration already exists." | ||
) | ||
continue | ||
else: | ||
self.stdout.write(f"Configuring {step}...") | ||
try: | ||
step.configure() | ||
except ConfigurationRunFailed as exc: | ||
raise CommandError(f"Could not configure step {step}") from exc | ||
else: | ||
self.stdout.write(f"{step} is successfully configured") | ||
configured_steps.append(step) | ||
|
||
# 3. Test configuration | ||
if skip_selftest: | ||
self.stdout.write("Selftest is skipped.") | ||
|
||
else: | ||
for step in configured_steps: | ||
try: | ||
step.test_configuration() | ||
except SelfTestFailed as exc: | ||
errors[step] = exc | ||
|
||
if errors: | ||
raise CommandError( | ||
f"Configuration test failed with errors: {errors.as_text()}" | ||
) | ||
|
||
self.stdout.write(self.style.SUCCESS("Instance configuration completed.")) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,17 +5,17 @@ build-backend = "setuptools.build_meta" | |
[project] | ||
name = "django_setup_configuration" | ||
version = "0.1.0" | ||
description = "TODO" | ||
description = "Pluggable configuration setup used with the django management command" | ||
authors = [ | ||
{name = "Maykin Media", email = "[email protected]"} | ||
] | ||
readme = "README.rst" | ||
license = {file = "LICENSE"} | ||
keywords = ["TODO"] | ||
keywords = ["Django", "Configuration"] | ||
classifiers = [ | ||
"Development Status :: 3 - Alpha", | ||
"Framework :: Django", | ||
"Framework :: Django :: 4.2", | ||
"Framework :: Django :: 3.2", | ||
"Intended Audience :: Developers", | ||
"Operating System :: Unix", | ||
"Operating System :: MacOS", | ||
|
@@ -27,19 +27,21 @@ classifiers = [ | |
] | ||
requires-python = ">=3.10" | ||
dependencies = [ | ||
"django>=4.2" | ||
"django>=3.2" | ||
] | ||
|
||
[project.urls] | ||
Homepage = "https://github.com/maykinmedia/django_setup_configuration" | ||
Documentation = "http://django_setup_configuration.readthedocs.io/en/latest/" | ||
"Bug Tracker" = "https://github.com/maykinmedia/django_setup_configuration/issues" | ||
"Source Code" = "https://github.com/maykinmedia/django_setup_configuration" | ||
Homepage = "https://github.com/maykinmedia/django-setup-configuration" | ||
Documentation = "http://django-setup-configuration.readthedocs.io/en/latest/" | ||
"Bug Tracker" = "https://github.com/maykinmedia/django-setup-configuration/issues" | ||
"Source Code" = "https://github.com/maykinmedia/django-setup-configuration" | ||
|
||
[project.optional-dependencies] | ||
tests = [ | ||
"pytest", | ||
"pytest-django", | ||
"pytest-mock", | ||
"furl", | ||
"tox", | ||
"isort", | ||
"black", | ||
|
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.
What if a valid setting value is
None
? Is that a case to consider?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.
How should I interpret these required settings? Can you give an example?
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.
Hmm I can't make up an example when we need
None
as a valid setting for configuration. ButFalse
can definitely be a valid one, so I'll refactor it.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.
You can find an example in Open Zaak - https://github.com/open-zaak/open-zaak/blob/4b870e709f1c08d53464eb6b76bc7ace11b2d87a/src/openzaak/config/bootstrap/demo.py#L29
Here we configure a demo user. So if settings with demo credentials are missing, then we raise
PrerequisiteFailed
If we want to skip this step we can set
DEMO_CONFIG_ENABLE = False
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.
I guess if it can be None, its not required?