Re: [pve-devel] [PATCH manager 2/2] Close #1295: Make apt notifications configurable

Re: [pve-devel] [PATCH manager 2/2] Close #1295: Make apt notifications configurable

[Dietmar Maurer]

> Is there a reason why we assume that users without subscription do not want
> such notifications?
> 
> As far as I see it, if we change it to
> > $dccfg->{notify_updates} // 1
> Then (until they change something)
> - users with active subscription should _continue_ to get notifications
> - enterprise repo configured but invalid subscription will continue to _not_
>   get mails (because pveupdate exits with error 100)

Can we instead notify them that their subscription is invalid?

Re: [pve-devel] [PATCH manager 2/2] Close #1295: Make apt notifications configurable

[Thomas Lamprecht]

On 08.04.21 11:21, Dominic Jäger wrote:
> On Wed, Apr 07, 2021 at 10:51:43AM +0200, Thomas Lamprecht wrote:
>> On 07.04.21 10:30, Dominic Jäger wrote:
>>> -# We assume that users with subscriptions want informations
>>> -# about new packages.
>>> -my $notify = ($info && $info->{status} eq 'Active') ? 1 : 0;
>>> +my $notify = $dccfg->{notify_updates} // 1;
>>
>> We may want to keep the default value the same, i.e.:
>>
>> my $notify = $dccfg->{notify_updates} // ($info && $info->{status} eq 'Active');
> 
> Is there a reason why we assume that users without subscription do not want
> such notifications?

For a production-class setup a subscription is highly recommended, so we
can assume that without such one it is a test/evaluation setup where we may
not want to get the mails...

> 
> As far as I see it, if we change it to
>> $dccfg->{notify_updates} // 1
> Then (until they change something)
> - users with active subscription should _continue_ to get notifications
> - enterprise repo configured but invalid subscription will continue to _not_
>   get mails (because pveupdate exits with error 100)

this is not only for enterprise repo or? What if user disable the
enterprise repo in test system. Just keep the default...

> 
> Then the only change is that users
> - without/invalid subscription and
> - with only no-subscription-repo configured
> will now suddenly get mails, but this is actually good?
> 
> We could also append a line "You can deactivate these notifications in the
> Datacenter options" to the mail.
> 
>>
>> (the following is actually meant for the pve-cluster patch):
>> I'd really prefer using a colon for new config property entries, and I 
can imagine
>> that there will be more such switches in the future, so maybe start out with a format
>> sting (like migration is there) and have something like:
>>
>> 'notify: package-updates=1'
>>
>> what do you think?
> Done :) So the mentioned
>> $dccfg->{notify_updates} // 1
> is actually
>> my $notify = $dccfg->{notify}->{package_updates} // 1;
> already.
> 

Re: [pve-devel] [PATCH manager 2/2] Close #1295: Make apt notifications configurable

[Dominic Jäger]

On Wed, Apr 07, 2021 at 10:51:43AM +0200, Thomas Lamprecht wrote:
> On 07.04.21 10:30, Dominic Jäger wrote:
> > -# We assume that users with subscriptions want informations
> > -# about new packages.
> > -my $notify = ($info && $info->{status} eq 'Active') ? 1 : 0;
> > +my $notify = $dccfg->{notify_updates} // 1;
> 
> We may want to keep the default value the same, i.e.:
> 
> my $notify = $dccfg->{notify_updates} // ($info && $info->{status} eq 'Active');

Is there a reason why we assume that users without subscription do not want
such notifications?

As far as I see it, if we change it to
> $dccfg->{notify_updates} // 1
Then (until they change something)
- users with active subscription should _continue_ to get notifications
- enterprise repo configured but invalid subscription will continue to _not_
  get mails (because pveupdate exits with error 100)

Then the only change is that users
- without/invalid subscription and
- with only no-subscription-repo configured
will now suddenly get mails, but this is actually good?

We could also append a line "You can deactivate these notifications in the
Datacenter options" to the mail.

> 
> (the following is actually meant for the pve-cluster patch):
> I'd really prefer using a colon for new config property entries, and I can imagine
> that there will be more such switches in the future, so maybe start out with a format
> sting (like migration is there) and have something like:
> 
> 'notify: package-updates=1'
> 
> what do you think?
Done :) So the mentioned
> $dccfg->{notify_updates} // 1
is actually
> my $notify = $dccfg->{notify}->{package_updates} // 1;
already.

Re: [pve-devel] [PATCH manager 2/2] Close #1295: Make apt notifications configurable

[Thomas Lamprecht]

On 07.04.21 10:30, Dominic Jäger wrote:
> This way all users, including subscription users, can decide if they want to
> receive notifications mails from pveupdate or not. Disabling notifications is
> desirable if available updates are monitored externally, for example.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
>  bin/pveupdate                 | 4 +---
>  www/manager6/dc/OptionView.js | 5 +++++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/bin/pveupdate b/bin/pveupdate
> index 99b52fe9..37e6e518 100755
> --- a/bin/pveupdate
> +++ b/bin/pveupdate
> @@ -50,9 +50,7 @@ if (my $err = $@) {
>  }
>  
>  my $info = PVE::INotify::read_file('subscription');
> -# We assume that users with subscriptions want informations
> -# about new packages.
> -my $notify = ($info && $info->{status} eq 'Active') ? 1 : 0;
> +my $notify = $dccfg->{notify_updates} // 1;

We may want to keep the default value the same, i.e.:

my $notify = $dccfg->{notify_updates} // ($info && $info->{status} eq 'Active');

or if it's important that the perl "boolean" is stricly 0 or 1 then:

my $notify_default = ($info && $info->{status} eq 'Active') ? 1 : 0;
my $notify = $dccfg->{notify_updates} // $notify_default;


(the following is actually meant for the pve-cluster patch):
I'd really prefer using a colon for new config property entries, and I can imagine
that there will be more such switches in the future, so maybe start out with a format
sting (like migration is there) and have something like:

'notify: package-updates=1'

what do you think?


>  eval { PVE::API2::APT->update_database({ node => $nodename, notify => $notify, quiet => 1 }); };
>  if (my $err = $@) {
>      syslog ('err', "update apt database failed: $err");
> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
> index 0e783cf4..b36acd07 100644
> --- a/www/manager6/dc/OptionView.js
> +++ b/www/manager6/dc/OptionView.js
> @@ -92,6 +92,11 @@ Ext.define('PVE.dc.OptionView', {
>  	    vtype: 'proxmoxMail',
>  	    defaultValue: 'root@$hostname',
>  	});
> +	me.add_boolean_row(
> +	    'notify_updates',
> +	    gettext('Notify about updates'),
> +	    { defaultValue: 1 },
> +	);
>  	me.add_text_row('mac_prefix', gettext('MAC address prefix'), {
>  	    deleteEmpty: true,
>  	    vtype: 'MacPrefix',
> 

[pve-devel] [PATCH manager 2/2] Close #1295: Make apt notifications configurable

[Dominic Jäger]

This way all users, including subscription users, can decide if they want to
receive notifications mails from pveupdate or not. Disabling notifications is
desirable if available updates are monitored externally, for example.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
 bin/pveupdate                 | 4 +---
 www/manager6/dc/OptionView.js | 5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/bin/pveupdate b/bin/pveupdate
index 99b52fe9..37e6e518 100755
--- a/bin/pveupdate
+++ b/bin/pveupdate
@@ -50,9 +50,7 @@ if (my $err = $@) {
 }
 
 my $info = PVE::INotify::read_file('subscription');
-# We assume that users with subscriptions want informations
-# about new packages.
-my $notify = ($info && $info->{status} eq 'Active') ? 1 : 0;
+my $notify = $dccfg->{notify_updates} // 1;
 eval { PVE::API2::APT->update_database({ node => $nodename, notify => $notify, quiet => 1 }); };
 if (my $err = $@) {
     syslog ('err', "update apt database failed: $err");
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index 0e783cf4..b36acd07 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -92,6 +92,11 @@ Ext.define('PVE.dc.OptionView', {
 	    vtype: 'proxmoxMail',
 	    defaultValue: 'root@$hostname',
 	});
+	me.add_boolean_row(
+	    'notify_updates',
+	    gettext('Notify about updates'),
+	    { defaultValue: 1 },
+	);
 	me.add_text_row('mac_prefix', gettext('MAC address prefix'), {
 	    deleteEmpty: true,
 	    vtype: 'MacPrefix',
-- 
2.20.1