setup_notification command #2

annashamray · 2024-02-09T16:22:30Z

fixes #1

codecov-commenter · 2024-02-20T14:13:21Z

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

sergei-maertens

I think it makes more sense to run all phases of a step and only then move to the next step, so that you can sort out dependencies between steps.

Now the order of steps is irrelevant since you first check all pre-requisites, then run all configurations and then the self tests but that breaks if you have a step that requires another step to be completed first

sergei-maertens · 2024-02-27T09:35:22Z

django_setup_configuration/configuration.py

+        if prerequisites are missing
+        """
+        missing = [
+            var for var in self.required_settings if not getattr(settings, var, None)


What if a valid setting value is None? Is that a case to consider?

How should I interpret these required settings? Can you give an example?

Hmm I can't make up an example when we need None as a valid setting for configuration. But False can definitely be a valid one, so I'll refactor it.

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

I guess if it can be None, its not required?

sergei-maertens · 2024-02-27T09:37:27Z

django_setup_configuration/configuration.py

+        if not self.enable_setting:
+            return True
+
+        return getattr(settings, self.enable_setting, True)


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?

I use django settings to set defaults when it's possible. For example we can have a default for SELECTIELIJST_API_ROOT

sergei-maertens · 2024-02-27T09:38:14Z

django_setup_configuration/exceptions.py

+
+class ConfigurationRunFailed(ConfigurationException):
+    """
+    Raise an error then configuration process was faulty


s/then/when?

sergei-maertens · 2024-02-27T09:38:52Z

django_setup_configuration/management/commands/setup_configuration.py

@@ -0,0 +1,117 @@
+from collections import OrderedDict


Python dicts retain insertion order, you generally don't need OrderedDict anymore

TIL! Changed to dict

sergei-maertens · 2024-02-27T10:09:48Z

django_setup_configuration/management/commands/setup_configuration.py

+
+class Command(BaseCommand):
+    help = (
+        "Bootstrap the initial Open Zaak configuration. "


Probably should remove Open Zaak from this docstring

django_setup_configuration/management/commands/setup_configuration.py

Co-authored-by: Sergei Maertens <[email protected]>

annashamray

To be honest I haven't considered case when one configuration step can be dependent on the other.
I thought of step.validate_requirements function more like a check that all required environment variables are provided. This is why I wanted first to check prerequisites of all the steps and give a full list of missing variables to the user.

I can do it step by step, but it can be annoying for the user.

Or I can divide this check into two parts: check required env vars and check other dependencies, and the latter can be done step by step.

annashamray · 2024-02-27T13:22:38Z

django_setup_configuration/configuration.py

+        if prerequisites are missing
+        """
+        missing = [
+            var for var in self.required_settings if not getattr(settings, var, None)


Hmm I can't make up an example when we need None as a valid setting for configuration. But False can definitely be a valid one, so I'll refactor it.

annashamray · 2024-02-27T13:25:34Z

django_setup_configuration/configuration.py

+        if prerequisites are missing
+        """
+        missing = [
+            var for var in self.required_settings if not getattr(settings, var, None)


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

annashamray · 2024-02-27T13:27:49Z

django_setup_configuration/configuration.py

+        if not self.enable_setting:
+            return True
+
+        return getattr(settings, self.enable_setting, True)


I use django settings to set defaults when it's possible. For example we can have a default for SELECTIELIJST_API_ROOT

annashamray · 2024-02-27T13:45:57Z

django_setup_configuration/exceptions.py

+
+class ConfigurationRunFailed(ConfigurationException):
+    """
+    Raise an error then configuration process was faulty


annashamray · 2024-02-27T13:51:53Z

django_setup_configuration/management/commands/setup_configuration.py

@@ -0,0 +1,117 @@
+from collections import OrderedDict


TIL! Changed to dict

annashamray · 2024-02-27T13:52:01Z

django_setup_configuration/management/commands/setup_configuration.py

+
+class Command(BaseCommand):
+    help = (
+        "Bootstrap the initial Open Zaak configuration. "


annashamray · 2024-02-27T14:04:51Z

@sergei-maertens thank you for the review!

joeribekker

Just needs Django 4.2 support so it won't hold us back in migrating projects.

annashamray · 2024-03-05T16:37:36Z

@joeribekker Django 4.2 is supported now

annashamray added 2 commits February 9, 2024 17:22

✨ [#1] first draft of setup_notification command

de81826

💚 [#1] fix gh actions

a68eabb

annashamray force-pushed the feature/1-config-command branch from d6a2708 to a68eabb Compare February 9, 2024 16:34

💚 [#1] fix doc build

fdba99b

annashamray marked this pull request as draft February 9, 2024 16:43

🐛 [#1] fix management command

3ac524b

annashamray added 4 commits February 20, 2024 15:32

💚 [#1] comment out non-existent links in the docs

261a73e

✨ [#1] small improvement of the package code and description

2cc9bae

✅ [#1] a create testapp and add tests

6c047d7

📝 [#1] update docs

bda38bb

annashamray marked this pull request as ready for review February 21, 2024 16:05

✨ [#1] add option to disable selftests in the command

a47235e

annashamray requested review from joeribekker and sergei-maertens February 26, 2024 11:27

sergei-maertens reviewed Feb 27, 2024

View reviewed changes

Apply suggestions from code review

16d3330

Co-authored-by: Sergei Maertens <[email protected]>

annashamray commented Feb 27, 2024

View reviewed changes

👌 [#1] proccess PR feedback

67dabde

👌 [#1] explicitly list invalid values for required settings

674ea6a

joeribekker approved these changes Mar 5, 2024

View reviewed changes

annashamray added 2 commits March 5, 2024 17:31

🔧 [#1] support Django 4.2

723a014

🔧 [#1] fix gh action name

c17c48a

annashamray mentioned this pull request Mar 5, 2024

As developer I want to have support for Django 4.2 #3

Closed

annashamray requested a review from stevenbal March 14, 2024 08:44

stevenbal approved these changes Mar 14, 2024

View reviewed changes

annashamray merged commit e4559f7 into main Mar 21, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setup_notification command #2

setup_notification command #2

annashamray commented Feb 9, 2024 •

edited

Loading

codecov-commenter commented Feb 20, 2024

sergei-maertens left a comment

sergei-maertens Feb 27, 2024

sergei-maertens Feb 27, 2024

annashamray Feb 27, 2024

annashamray Feb 27, 2024

joeribekker Mar 5, 2024

sergei-maertens Feb 27, 2024

annashamray Feb 27, 2024

sergei-maertens Feb 27, 2024

annashamray Feb 27, 2024

sergei-maertens Feb 27, 2024

annashamray Feb 27, 2024

sergei-maertens Feb 27, 2024

annashamray Feb 27, 2024

annashamray left a comment

annashamray Feb 27, 2024

annashamray Feb 27, 2024

annashamray Feb 27, 2024

annashamray Feb 27, 2024

annashamray Feb 27, 2024

annashamray Feb 27, 2024

annashamray commented Feb 27, 2024

joeribekker left a comment

annashamray commented Mar 5, 2024

setup_notification command #2

setup_notification command #2

Conversation

annashamray commented Feb 9, 2024 • edited Loading

codecov-commenter commented Feb 20, 2024

Welcome to Codecov 🎉

sergei-maertens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annashamray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annashamray commented Feb 27, 2024

joeribekker left a comment

Choose a reason for hiding this comment

annashamray commented Mar 5, 2024

annashamray commented Feb 9, 2024 •

edited

Loading