public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: "Dominic Jäger" <d.jaeger@proxmox.com>,
	"Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-common] JSONSchema: Increase vmid option minimum to 100
Date: Tue, 15 Dec 2020 09:55:06 +0100	[thread overview]
Message-ID: <680c7504-9b6d-08f2-3b72-a8a2bcec16b6@proxmox.com> (raw)
In-Reply-To: <20201215081139.GA383434@mala.proxmox.com>

On 15.12.20 09:11, Dominic Jäger wrote:
> On Mon, Dec 14, 2020 at 01:33:25PM +0100, Fabian Grünbichler wrote:
>> I think the argument is that the associated format already enforces that 
>> the ID is >= 100, so that the API allows less has no practical effect 
>> except confusing users that read the man page/help output/api dump.
> 
> This is exactly what I meant.  But I'm not 100% sure if some schema magic could
> happen that circumvents the pve-vmid format, so I can certainly start
> 

FWIW; I never questioned that and this was 100% clear to me, but it still
does not help if we have some use here, even if only legacy or restore wise
or what not, touch such things can easily break things in a subtle way, and
only get noticed in months or even years, we had such stuff a few times already,
so I'd be really cautious here - adding the "actual enforced limit" into the
description can also help.

I mean, I really do not think this crucial limit slipped and was left as is
for years...

> On Mon, Dec 14, 2020 at 12:52:35PM +0100, Thomas Lamprecht wrote:
>> maybe you could take another look if you find
>> some case where values <100 are used for some special handling?
> 
> looking for this.
> 
>>>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
>>>> index 29ada5b..b4596d3 100644
>>>> --- a/src/PVE/JSONSchema.pm
>>>> +++ b/src/PVE/JSONSchema.pm
>>>> @@ -58,7 +58,7 @@ sub get_standard_option {
>>>>  register_standard_option('pve-vmid', {
>>>>      description => "The (unique) ID of the VM.",
>>>>      type => 'integer', format => 'pve-vmid',
>>>> -    minimum => 1
>>>> +    minimum => 100,
>>>>  });
>>>>  






      reply	other threads:[~2020-12-15  8:55 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-14 10:20 Dominic Jäger
2020-12-14 11:52 ` Thomas Lamprecht
2020-12-14 12:33   ` Fabian Grünbichler
2020-12-15  8:11     ` Dominic Jäger
2020-12-15  8:55       ` Thomas Lamprecht [this message]

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=680c7504-9b6d-08f2-3b72-a8a2bcec16b6@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=d.jaeger@proxmox.com \
    --cc=f.gruenbichler@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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal