all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "Lukas Sichert" <l.sichert@proxmox.com>
To: "Dominik Csapak" <d.csapak@proxmox.com>, <pve-devel@lists.proxmox.com>
Subject: Re: [PATCH manager v2] ui: fabrics: skip unsupported interface properties on submit
Date: Tue, 14 Jul 2026 15:54:14 +0200	[thread overview]
Message-ID: <DJYCFCF8OV6O.28KC9JEZQCGCX@proxmox.com> (raw)
In-Reply-To: <15aa5e3d-a2ea-4483-8ca8-91a39aaecf7c@proxmox.com>

Thanks for looking into this. I will send a v3. Some comments inline:

On 2026-07-14 10:23, Dominik Csapak <d.csapak@proxmox.com> wrote:

> wanted to apply this already, but during testing i noticed
> that even without the patch, we don't have an ipv4 column here, so
> i was confused by the commit message.

You are right, the wording is very misleading here. The base panel
always adds an IPv4 column, but it gets removed with 'me.commonColumns =
me.commonColumns.filter((col) => col.dataIndex !== 'ip');' in
bgp/InterfacePanel.js, so it is not actually there anymore.

I still think it is cleaner to make IPv4 column support configurable,
just like IPv6 column support already is. That part is only a
behavior-preserving cleanup though.

> It turns out, while this patch indeed fixes the error, the rationale
> in the commmit message is not precise and patch seems to conflate two
> things here (correct me if i'm missing something here)
>
> the panel never got a field via the column, because those got
> filtered out via 'hasIpv6Support' or in the initComponent you removed
>
> so the stray 'ip' comes from the overall store, not the columns
> (which get set in 'setValue')
>
> so this patch does two things here:
>
> it moves the removal of the ip column from the initcomponent
> to a config (which is fine IMO, but does not change the actual
> behavior)
>
> and it adds a manual filter in 'getValue' to skip the ip and ip6 field
> depending on this config -> this is what actually fixes the issue
>
>
> i think this should be split into two patch that reflect these two
> changes, with the proper commit messages.
> (though we can do it in one commmit, but then the message must explain
> the issue properly)
>
> the code itself LGTM
>

It absolutely makes sense to do this in two patches. I will split this
into a behavior-preserving cleanup for the IPv4 column handling and a
separate fix for filtering unsupported `ip`/`ip6` properties in
`getValue()` for v3.





  reply	other threads:[~2026-07-14 13:54 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-03 15:55 [PATCH manager v2] ui: fabrics: skip unsupported interface properties on submit Lukas Sichert
2026-06-05  8:04 ` Hannes Laimer
2026-07-14  8:23 ` Dominik Csapak
2026-07-14 13:54   ` Lukas Sichert [this message]
2026-07-15  8:18 ` superseded: " Lukas Sichert

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=DJYCFCF8OV6O.28KC9JEZQCGCX@proxmox.com \
    --to=l.sichert@proxmox.com \
    --cc=d.csapak@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal