✨ Add ConfigurationStep for NotificationsConfig #19
Conversation
stevenbal
force-pushed
the
feature/setup-configuration
branch
from
91870a7 to
9647d84
Compare
| @@ -0,0 +1,5 @@ | |||
| notifications_config_enable: True | |||
There was a problem hiding this comment.
| notifications_config_enable: True | |
| notifications_config_enable: true |
| @@ -0,0 +1,6 @@ | |||
| notifications_config_enable: True | |||
There was a problem hiding this comment.
| notifications_config_enable: True | |
| notifications_config_enable: true |
| ] | ||
|
|
||
| The YAML file that is passed to ``setup_configuration`` must set the | ||
| ``notifications_config_enable`` flag to ``true`` to enable the step. All fields under ``notifications_config`` are optional. |
There was a problem hiding this comment.
Is there going to be a use-case where the notifications_api_service_identifier is not going to be defined? I'm unsure why an user would configure this without also defining a service. If I read it correctly the other options have no effect whenever that is the case.
There was a problem hiding this comment.
I initially thought that Open Notificaties didn't need to specify this, but it turns out that in order to be able to subscribe to the autorisaties channel, you do need to specify this.
I guess we could make it required to avoid confusion
There was a problem hiding this comment.
@swrichards would you agree making the notifications_api_service_identifier required (and thereby a Service) ?
There was a problem hiding this comment.
Let's do it.
I think more generally, if we find ourselves in a situation bordering on "nearly every field is optional", you can lean towards assuming more requirements, because the step can always be disabled by the user. The frame is assuming you wanted to enable this, we require a few things, which means we can bit a bit more liberal in requiring settings.
|
|
||
| notifications_config_enable: True | ||
| notifications_config: | ||
| notifications_api_service_identifier: notifs-api |
There was a problem hiding this comment.
I think it would make the documentation more complete if we also mention somewhere that this points to either a existing service or one that is going to be created through the zgw-consumers setup step (and is declared in the same yaml file).
There was a problem hiding this comment.
I agree, I think this is going to be done after the initial deadline. This is something that all projects will have to mention in their docs, so we should think of an easy way to do this without repeating ourselves. Sergei mentioned that using intersphinx to link to the library documentation (with a section written for devops) could be an option
|
@SonnyBA @stevenbal @Coperh I adopted this branch and added an extra step, and I also made the service required on the configuration model. Closing this in favor of #22 . |
Required for: