From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id EB2421FF13F for ; Thu, 12 Mar 2026 13:55:09 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E392714ABD; Thu, 12 Mar 2026 13:55:04 +0100 (CET) Date: Thu, 12 Mar 2026 13:54:29 +0100 From: Wolfgang Bumiller To: Daniel Kral Subject: Re: [RFC PATCH common] section config: correctly handle required options in write_config Message-ID: References: <20260311122659.250421-1-d.kral@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773320034279 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.982 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 RCVD_IN_MSPIKE_H2 0.001 Average reputation (+2) RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.408 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.819 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.903 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: SJG4PH25ECAKSYNS6KGO5P2PD33OE6ZX X-Message-ID-Hash: SJG4PH25ECAKSYNS6KGO5P2PD33OE6ZX X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, Mar 12, 2026 at 01:19:53PM +0100, Daniel Kral wrote: > On Thu Mar 12, 2026 at 10:47 AM CET, Max R. Carrara wrote: > > Just had a look at your patch—nice that you spotted this! > > > > I think this fix here should be fine, but I think it would be nice if > > you added a test: Replicate the broken behavior first, and then fix it > > together with this patch here. So, basically a little bit of TDD. > > > > I'm not a big fan of TDD myself, but I think in this specific case it > > would be beneficial for us to have a test like that—it's also > > documentation at the same time. > > > > Perhaps a nice spot for that would be in the SectionConfig tests > > directory [1] as a separate script or something—see the other scripts > > for how the tests in there work. (ofc you can holler at me too!) > > Thanks for the quick feedback! > > Yeah, but for something integral as the section config adding some test > cases documenting the changes would be great, I'll do that for the v2! > > > > > Regarding JSONSchema: I think the definedness check itself is fine—an > > empty array is still "something" rather than nothing, so ... > > > > We could instead implement some of the other validation keywords [2] for > > arrays, like e.g. `minItems`. I think an array that's `required` and > > also has `minItems` set to `1` would allow you to express what you > > initially wanted where you fixed #7399 [0], right? > > > > I just hope that this is possible in your current JSONSchema—Wolfgang > > and I have spoken about some things related to this a (longer) while > > ago; I believe it was about empty strings / zeroes as well. *That* might > > be a bit more complicated to solve, though and might even require a > > JSONSchema v2 of some sorts, but I don't have all the details in my head > > at the moment. > > Thanks for the insights, I haven't looked too closely at the json schema > draft yet! > > You're right, even though it seems that the draft doesn't specify any > behavior regarding our `optional` property, my proposed change in our Our optional property is just an "inlined" version of *not* putting something in the enclosing object's `required` array. (IIRC we discussed the possibility of adding that and slowly moving over to using that as it would also make the perl<->rust conversion/interop easier in the long run. > json schema code would break assumptions around our code base and with > all the `min*` properties in mind it indeed would be inconsistent if > those `min*` properties default to `0`. So let's discard that idea. > > > > > [1]: https://git.proxmox.com/?p=pve-common.git;a=tree;f=test/SectionConfig;h=0967fb1eebf84b4ecad696901572fa7550d49941;hb=refs/heads/master > > [2]: https://json-schema.org/draft/2020-12/json-schema-validation#name-validation-keywords-for-arr