Merge ~d0ugal/maas:release-notification-setting into maas:master

Proposed by Dougal Matthews
Status: Merged
Approved by: Dougal Matthews
Approved revision: f11665f214ab20b0f2cb7b874cdb99014255e7f2
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~d0ugal/maas:release-notification-setting
Merge into: maas:master
Diff against target: 33 lines (+11/-0)
2 files modified
src/maasserver/forms/settings.py (+10/-0)
src/maasserver/models/config.py (+1/-0)
Reviewer Review Type Date Requested Status
Lee Trager (community) Approve
MAAS Lander Needs Fixing
Review via email: mp+388137@code.launchpad.net

Commit message

Add the Release Notification configuration option

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b release-notification-setting lp:~d0ugal/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8030/console
COMMIT: 2977a161338b30ff052b353bac13d8ab138bb334

review: Needs Fixing
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b release-notification-setting lp:~d0ugal/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8031/console
COMMIT: bdd765e5f11bcb8aa0e3d17d27eb63e7d90676b2

review: Needs Fixing
Revision history for this message
Lee Trager (ltrager) wrote :

Why are we making this a configuration option? None of our other notifications are optional. The data will be coming from the MAAS streams so it will be downloaded regardless.

review: Needs Information
Revision history for this message
Kit Randel (blr) wrote :

That was the design agreed upon
https://app.zeplin.io/project/5efa65801b4d9162739308fc/screen/5f16b3b5311acb77fa0925d7

On Tue, Jul 28, 2020 at 12:38 PM Lee Trager <email address hidden>
wrote:

> Review: Needs Information
>
> Why are we making this a configuration option? None of our other
> notifications are optional. The data will be coming from the MAAS streams
> so it will be downloaded regardless.
> --
> https://code.launchpad.net/~d0ugal/maas/+git/maas/+merge/388137
> Your team MAAS Committers is subscribed to branch maas:master.
>
> Launchpad-Message-Rationale: Subscriber @maas-committers
> Launchpad-Message-For: maas-committers
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~maas-committers/maas/+git/maas:master
> Launchpad-Project: maas
>

--
Kit Randel (blr)
Canonical - Web Team

Revision history for this message
Kit Randel (blr) wrote :

Also described in the new release notification spec

Revision history for this message
Kit Randel (blr) wrote :

Dougal, do you have a followup branch planned to prevent sending release notifications if this config is enabled?

Revision history for this message
Dougal Matthews (d0ugal) wrote :

> Why are we making this a configuration option? None of our other notifications
> are optional. The data will be coming from the MAAS streams so it will be
> downloaded regardless.

Right, this is exactly as Kit mentioned. This is a requirement of the release notification work, we want to give admins the ability to disable it.

Revision history for this message
Dougal Matthews (d0ugal) wrote :

> Dougal, do you have a followup branch planned to prevent sending release
> notifications if this config is enabled?

I have a follow up branch which I am working on now that will use this config option and send or not send the notifications (nothing creates them yet). I wanted to get this change ahead of the rest so that any UI work is unblocked.

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b release-notification-setting lp:~d0ugal/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 4ee0c6ba7fc0959adaebaee3b42688fa0523c421

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b release-notification-setting lp:~d0ugal/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: ce001452d24899e5a2b9a33979d156ccb2efe114

review: Approve
Revision history for this message
Lee Trager (ltrager) wrote :

I understand you're using a list so future notification types can be toggled. The Config model has API and websocket code so new values should just work. As this is a new type you may need to modify those as well. I wonder if we should keep this as a simple boolean as that is what the design calls for. In the future we could create new boolean config values as well if need be.

review: Needs Information
Revision history for this message
Dougal Matthews (d0ugal) wrote :

> I understand you're using a list so future notification types can be toggled.
> The Config model has API and websocket code so new values should just work. As
> this is a new type you may need to modify those as well. I wonder if we should
> keep this as a simple boolean as that is what the design calls for. In the
> future we could create new boolean config values as well if need be.

Thanks, that is a valid concern. To save time I have switched back to a boolean for now.

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b release-notification-setting lp:~d0ugal/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8043/console
COMMIT: f11665f214ab20b0f2cb7b874cdb99014255e7f2

review: Needs Fixing
Revision history for this message
Lee Trager (ltrager) wrote :

LGTM!

review: Approve
618e33a... by Dougal Matthews

Blacken code

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/src/maasserver/forms/settings.py b/src/maasserver/forms/settings.py
index f03eb04..f2d5692 100644
--- a/src/maasserver/forms/settings.py
+++ b/src/maasserver/forms/settings.py
@@ -753,6 +753,16 @@ CONFIG_ITEMS = {
753 "min_value": 1,753 "min_value": 1,
754 },754 },
755 },755 },
756 "release_notifications": {
757 "default": True,
758 "form": forms.BooleanField,
759 "form_kwargs": {
760 "required": False,
761 "label": (
762 "Enable or disable notifications for new MAAS releases."
763 ),
764 },
765 },
756 "use_rack_proxy": {766 "use_rack_proxy": {
757 "default": True,767 "default": True,
758 "form": forms.BooleanField,768 "form": forms.BooleanField,
diff --git a/src/maasserver/models/config.py b/src/maasserver/models/config.py
index 2aa381d..b6e9d9e 100644
--- a/src/maasserver/models/config.py
+++ b/src/maasserver/models/config.py
@@ -105,6 +105,7 @@ def get_default_config():
105 "max_node_installation_results": 3,105 "max_node_installation_results": 3,
106 # Notifications.106 # Notifications.
107 "subnet_ip_exhaustion_threshold_count": 16,107 "subnet_ip_exhaustion_threshold_count": 16,
108 "release_notifications": True,
108 # Authentication.109 # Authentication.
109 "external_auth_url": "",110 "external_auth_url": "",
110 "external_auth_user": "",111 "external_auth_user": "",

Subscribers

People subscribed via source and target branches