From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A2135E5C1 for ; Tue, 18 Jul 2023 15:58:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7DD9B1C1A5 for ; Tue, 18 Jul 2023 15:58:20 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 18 Jul 2023 15:58:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 34B5742D1F; Tue, 18 Jul 2023 15:58:19 +0200 (CEST) Message-ID: <93e5a6f5-a163-6008-b2d6-c27478827d05@proxmox.com> Date: Tue, 18 Jul 2023 15:58:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Lukas Wagner , Proxmox VE development discussion References: <20230717150051.710464-1-l.wagner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.016 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v3 many 00/66] fix #4156: introduce new notification system X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jul 2023 13:58:50 -0000 On 7/18/23 15:14, Lukas Wagner wrote: > On 7/18/23 14:34, Dominik Csapak wrote: >> gave the series a quick spin, review of the gui patches comes later ;) >> >> a few high level comments from a user perspective: >> >> * the node fencing/replication edit windows always shows the warning that it shouldn't be >>    disabled, that should imo only be there if i select 'never' ? >>    (conversely, the package update window never shows the warning...) > Good point, I'll try to make it only show up if 'never' is selected. > The warning came actually to be as an input from Aaron, so that user's don't turn > off stuff for critical events without knowing what they are doing. > I didn't add them to the package update notifications because there, the setting > already existed in datacenter config without a warning and it seemed less critical to me. > But yeah, I guess it would make sense to add it there as well. > >> * when we have this, we could remove the pacakge notify line from the datacenter options, no? > > Yes, you are right, forgot about that. > >> * imho having "Notification Targets" directly below "Notifications" is a bit redundant >>    (and wasting space), but it's just a minor thing > > True, I can probably remove that, since it's already clear from the menu on the left-hand side. > >> * the filter 'mode' is also not exposed on the gui (intentionally?)> * also the mode is not quite >> clear since only one filter can be active per target? >>    (or is it meant when there are multiple filter by means of notification groups?)> * what is a >> filter without a minimum severity? what does it do? >>    (does it make sense to allow such filters in the first place?) > > Filters will be extended in the future so that they can match on multiple properties. > Every notification has a severity as well as arbitrary metadata attached to it (e.g. could be > hostname, backup-job ID, etc.). > > In future, a filter could be like > > filter: foo >     mode and|or >     min-severity error >     match-property hostname=pali > > > So here, the mode determines if `min-severity` AND/OR `match-property` have to match. > That is also the reason why a filter without min-severity can exist. > > Also, I'm thinking of supporting 'sub-filters': > > filter: foo >     mode or >     sub-filter a >     sub-filter b > > filter: a >     mode and >     min-severity error >     match-property hostname=pali > > filter: b >     mode and >     min-severity info >     match-property hostname=haya > > `mode`, `invert-match` and `sub-filter` together basically would allow users to > create arbitrarily complex filter 'formulas'. > > > I already have patches laying around that implement the additional filter matchers, but > decided to not include them in the patch series yet. Before the new matchers are merged, > I would need to 'stabilize' the properties associate with every single notification event, > as at that point those become part of our public facing API. At the moment, the properties > are only an implementation detail that is used for rendering notification templates. > > This is also the reason why the filter implementation (filter.rs) is somewhat overkill > atm for _just_ severity filtering. Everything needed for these advanced features is already > in place - because I already implemented that stuff and cut it out later for the patch series. ah ok, so the mode is currently unused. one of my questions remains though, does it make sense to configure a filter without any filtering properties? i guess not really > >> * the backup edit window is rather tall at this point, and if we assume a minimum >>    screen size of 1280x720 there is not quite enough space when you subtract >>    the (typical) os bar and browser window decoration, maybe a 'notification' >>    tab would be better (we already have some tabs there anyway) > > Good point, will look into that. > >> * i found one bug, but not quite sure yet where it comes from exactly, >>    putting in emojis into a field (e.g. a comment or author) it's accepted, >>    but editing a different entry fails with: >> >> --->8--- >> could not serialize configuration: writing 'notifications.cfg' failed: detected unexpected control >> character in section 'testgroup' key 'comment' (500) >> ---8<--- >> >> not sure where the utf-8 info gets lost. (or we could limit all fields to ascii?) >> such a notification target still works AFAICT (but if set as e.g. the author it's >> probably the wrong value) >> >> (i used 😀 as a test) > > I will investigate, thanks! >> >> >> >> otherwise works AFAICT >> > > Thanks so much for giving this a spin! >