From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 05D931FF13F for ; Thu, 12 Mar 2026 13:20:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CC0A513AE7; Thu, 12 Mar 2026 13:20:29 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 12 Mar 2026 13:19:53 +0100 Message-Id: Subject: Re: [RFC PATCH common] section config: correctly handle required options in write_config From: "Daniel Kral" To: "Max R. Carrara" , X-Mailer: aerc 0.21.0-38-g7088c3642f2c-dirty References: <20260311122659.250421-1-d.kral@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773317958130 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.024 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: PLWKQYNCMOPIXMGF3NWDEYBBBVYOS2AG X-Message-ID-Hash: PLWKQYNCMOPIXMGF3NWDEYBBBVYOS2AG X-MailFrom: d.kral@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: Wolfgang Bumiller 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 10:47 AM CET, Max R. Carrara wrote: > Just had a look at your patch=E2=80=94nice 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=E2=80=94it'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=E2=80=94see the other scr= ipts > 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=E2=80= =94an > 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=E2=80=94Wolf= gang > 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 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=3Dpve-common.git;a=3Dtree;f=3Dtest/Sectio= nConfig;h=3D0967fb1eebf84b4ecad696901572fa7550d49941;hb=3Drefs/heads/master > [2]: https://json-schema.org/draft/2020-12/json-schema-validation#name-va= lidation-keywords-for-arr