From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Daniel Kral <d.kral@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [RFC PATCH common] section config: correctly handle required options in write_config
Date: Thu, 12 Mar 2026 13:54:29 +0100 [thread overview]
Message-ID: <mknpn6iozyda5w5hcjj7ma6jvstcw6bizwtg5vmhfyyn5zrddo@qgpeovyucavu> (raw)
In-Reply-To: <DH0SRK112AT3.3AZ6Y8WBAYNT3@proxmox.com>
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
next prev parent reply other threads:[~2026-03-12 12:55 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-11 12:26 Daniel Kral
2026-03-12 9:47 ` Max R. Carrara
2026-03-12 12:19 ` Daniel Kral
2026-03-12 12:54 ` Wolfgang Bumiller [this message]
2026-03-12 13:06 ` Wolfgang Bumiller
2026-03-12 13:25 ` Daniel Kral
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mknpn6iozyda5w5hcjj7ma6jvstcw6bizwtg5vmhfyyn5zrddo@qgpeovyucavu \
--to=w.bumiller@proxmox.com \
--cc=d.kral@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox