From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id C09EF1FF17C
for <inbox@lore.proxmox.com>; Wed, 2 Apr 2025 12:41:12 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 495F3F85E;
Wed, 2 Apr 2025 12:41:01 +0200 (CEST)
Date: Wed, 2 Apr 2025 12:40:28 +0200
From: Gabriel Goller <g.goller@proxmox.com>
To: Christoph Heiss <c.heiss@proxmox.com>
Message-ID: <cmoxeoyq5pk6pgtbhjompa2jrpth6b3czju2o24etrl6mmmkgk@mdrogzcpvbqe>
Mail-Followup-To: Christoph Heiss <c.heiss@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250328171340.885413-1-g.goller@proxmox.com>
<20250328171340.885413-51-g.goller@proxmox.com>
<D8W26409CVS4.14VE1ODAGF54M@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <D8W26409CVS4.14VE1ODAGF54M@proxmox.com>
User-Agent: NeoMutt/20241002-35-39f9a6
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.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
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH pve-manager 6/7] fabrics: Add main FabricView
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On 02.04.2025 11:50, Christoph Heiss wrote:
>Some comments inline - did the review mostly in tandem with testing the
>UI, to get a better context.
>
>On Fri Mar 28, 2025 at 6:13 PM CET, Gabriel Goller wrote:
>[..]
>> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
>> index 74728c8320e9..68f7be8d6042 100644
>> --- a/www/manager6/dc/Config.js
>> +++ b/www/manager6/dc/Config.js
>> @@ -229,6 +229,14 @@ Ext.define('PVE.dc.Config', {
>> hidden: true,
>> iconCls: 'fa fa-shield',
>> itemId: 'sdnfirewall',
>> + },
>> + {
>> + xtype: 'pveSDNFabricView',
>> + groups: ['sdn'],
>> + title: gettext('Fabrics'),
>
>Do we really want to translate 'Fabrics'? Or rather just keep the name
>always at 'Fabrics' as the name of the concept/feature itself? Since it
>is a established term, after all. I'd keep it, IMHO.
>
>(But we seem to translate everything else here in the file, so really
>not sure.)
Hmm I mean it's probably the same in every language, but we also seem to
translate everything, even "VNET Firewall" and "IPAM", so IMO I'd leave
it just for the sake of consistency.
>> + hidden: true,
>> + iconCls: 'fa fa-road',
>> + itemId: 'sdnfabrics',
>> });
>> }
>>
>> diff --git a/www/manager6/sdn/FabricsView.js b/www/manager6/sdn/FabricsView.js
>> new file mode 100644
>> index 000000000000..0ef12defb1a8
>> --- /dev/null
>> +++ b/www/manager6/sdn/FabricsView.js
>[..]
>> +Ext.define('PVE.sdn.Fabric.View', {
>[..]
>> + {
>> + text: gettext('Protocol'),
>> + dataIndex: 'protocol',
>> + width: 100,
>> + renderer: function(value, metaData, rec) {
>> + if (rec.data.type !== 'fabric') {
>> + return "";
>> + }
>> +
>> + return PVE.Utils.render_sdn_pending(rec, value, 'protocol');
>> + },
>
>A mapping for the internal protocol name to a user-visible would be nice
>here, i.e. 'ospf' -> 'OSPF' and 'openfabric' -> 'OpenFabric'. Not much
>difference currently, but would look a bit better in the UI :)
Oh, that's a nice one, added this.
>> [snip]
>> + handler: 'addActionTreeColumn',
>> + getTip: (_v, _m, _rec) => gettext('Add'),
>
>nit: "Add Node" would be better here for node rows, if that's possible.
I agree.
>> + getClass: (_v, _m, { data }) => {
>> + if (data.type === 'fabric') {
>> + return 'fa fa-plus-circle';
>> + }
>> +
>> + return 'pmx-hidden';
>> + },
>> + isActionDisabled: (_v, _r, _c, _i, { data }) => data.type !== 'fabric',
>> + },
>> + {
>> + tooltip: gettext('Edit'),
>
>^ same as above
Here it's a bit different, because the Edit button is on Fabrics and
Nodes, so I'd have to add the getTip callback and format the type in it.
IMO this is not worth it.
>> + handler: 'editAction',
>> + getClass: (_v, _m, { data }) => {
>> + // the fabric type (openfabric, ospf, etc.) cannot be edited
>> + if (data.type) {
>> + return 'fa fa-pencil fa-fw';
>> + }
>> +
>> + return 'pmx-hidden';
>> + },
>> + isActionDisabled: (_v, _r, _c, _i, { data }) => !data.type,
>> + },
>> + {
>> + tooltip: gettext('Delete'),
>
>^ and same here as well
Same as my answer above.
>> + handler: 'deleteAction',
>> + getClass: (_v, _m, { data }) => {
>> + // the fabric type (openfabric, ospf, etc.) cannot be deleted
>> + if (data.type) {
>> + return 'fa critical fa-trash-o';
>> + }
>> +
>> + return 'pmx-hidden';
>> + },
>> + isActionDisabled: (_v, _r, _c, _i, { data }) => !data.type,
>> + },
>> + ],
>> + },
>> + {
>> + header: gettext('Interfaces'),
>> + width: 100,
>> + dataIndex: 'interface',
>> + renderer: function(value, metaData, rec) {
>> + let interfaces = rec.data.pending?.interface || rec.data.interface || [];
>
>nit: could be const (see [0] tho regarding rules for const/let)
>
>[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
Yep.
>> [snip]
>> + initComponent: function() {
>> + let me = this;
>> +
>> +
>> + let add_button = new Proxmox.button.Button({
>
>New variables should generally be camelCase [0].
>
>[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing
Fixed as well.
>> + text: gettext('Add Node'),
>> + handler: 'addActionTbar',
>> + disabled: true,
>> + });
>
>Since there is also an "add node" button in the "Action" column for each
>fabric, do we need a separate one in the top-bar as well?
We talked with Dominik off-list and he mentioned that in the
ResourceMapping the users didn't always understand what the button did or
how to add a "child". That's why we add both buttons.
>> +
>> + let set_add_button_status = function() {
>> + let selection = me.view.getSelection();
>> +
>> + if (selection.length === 0) {
>> + return;
>> + }
>> +
>> + add_button.setDisabled(selection[0].data.type !== 'fabric');
>> + };
>> +
>> + Ext.apply(me, {
>> + tbar: [
>> + {
>> + text: gettext('Add Fabric'),
>> + menu: [
>> + {
>> + text: gettext('OpenFabric'),
>
>'OpenFabric' also should not be translated, right? As its the name of
>the implementation/protocol.
Right.
>> + handler: 'openAddOpenFabricWindow',
>> + },
>> + {
>> + text: gettext('OSPF'),
>
>^ same here
Fixed it also.
>> [snip]
>> + controller: {
>> + xclass: 'Ext.app.ViewController',
>> +
>> + reload: function() {
>> + let me = this;
>> +
>> + Proxmox.Utils.API2Request({
>> + url: `/cluster/sdn/fabrics/?pending=1`,
>> + method: 'GET',
>> + success: function(response, opts) {
>> + let ospf = response.result.data.ospf;
>> + let openfabric = response.result.data.openfabric;
>> +
>> + // add some metadata so we can merge the objects later and still know the protocol/type
>> + ospf = ospf.map(x => {
>> + if (x.state && x.state === 'new') {
>> + Object.assign(x, x.pending);
>> + }
>> +
>> + if (x.ty === 'fabric') {
>> + return Object.assign(x, { protocol: "ospf", name: x.area });
>> + } else if (x.ty === 'node') {
>> + let id = x.node_id.split("_");
>> + return Object.assign(x,
>> + {
>> + protocol: "ospf",
>> + node: id[1],
>> + fabric: id[0],
>> + },
>> + );
>> + } else {
>> + return x;
>> + }
>> + });
>
>Perhaps factor this out a bit into a separate function, making the
>success() definition here a bit more succinct?
Done, created a few functions mapOpenFabric, mapOspf and mapTree.
>> [snip]
>^ and same here, moving it into a separate function instead of
>open-coding it here. Would make the whole bit here more readable/easier
>to understand.
See above.
>> + }
>> +
>> + Object.assign(fabric, {
>> + type: 'fabric',
>> + protocol: fabric.protocol,
>> + expanded: true,
>> + name: fabric.fabric_id || fabric.area,
>> + iconCls: 'fa fa-road x-fa-treepanel',
>> + });
>> +
>> + return fabric;
>> + });
>> +
>[..]
>> +
>> + addActionTreeColumn: function(_grid, _rI, _cI, _item, _e, rec) {
>> + let me = this;
>
>nit: never used, just drop it
Done.
>> +
>> + deleteAction: function(table, rI, cI, item, e, { data }) {
>> + let me = this;
>> + let view = me.getView();
>> +
>> + Ext.Msg.show({
>> + title: gettext('Confirm'),
>> + icon: Ext.Msg.WARNING,
>> + message: Ext.String.format(gettext('Are you sure you want to remove the fabric {0}?'), `${data.name}`),
>
>nit: unnecessary string interpolation
Oh, yes. Also added some quotes around the name, as it's more readable
that way.
>> + buttons: Ext.Msg.YESNO,
>> + defaultFocus: 'no',
>> + callback: function(btn) {
>> + if (btn !== 'yes') {
>> + return;
>> + }
>> +
>> + let url;
>> + if (data.type === "node") {
>> + url = `/cluster/sdn/fabrics/${data.protocol}/${data._fabric}/node/${data.name}`;
>> + } else if (data.type === "fabric") {
>> + url = `/cluster/sdn/fabrics/${data.protocol}/${data.name}`;
>> + } else {
>> + console.warn("deleteAction: missing type");
>> + }
>> +
>> + Proxmox.Utils.API2Request({
>> + url,
>> + method: 'DELETE',
>> + waitMsgTarget: view,
>> + failure: function(response, opts) {
>> + Ext.Msg.alert(gettext('Error'), response.htmlStatus);
>
>Could use `Proxmox.Utils.errorText` here instead of `gettext('Error')`.
Nice!
>> + },
>> + callback: me.reload.bind(me),
>
>nit: `() => me.reload()` is already used multiple times in this file, so
>use that here too for consistency? Think it's the preferred style
>anyway.
Right.
>> [snip]
Thanks for the review!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel