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 6DD4B1FF17C
for <inbox@lore.proxmox.com>; Wed, 2 Apr 2025 11:51:14 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id E4024E0FF;
Wed, 2 Apr 2025 11:51:01 +0200 (CEST)
Date: Wed, 02 Apr 2025 11:50:57 +0200
Message-Id: <D8W26409CVS4.14VE1ODAGF54M@proxmox.com>
From: "Christoph Heiss" <c.heiss@proxmox.com>
To: "Gabriel Goller" <g.goller@proxmox.com>
Mime-Version: 1.0
X-Mailer: aerc 0.20.1
References: <20250328171340.885413-1-g.goller@proxmox.com>
<20250328171340.885413-51-g.goller@proxmox.com>
In-Reply-To: <20250328171340.885413-51-g.goller@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.029 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_VALIDITY_CERTIFIED_BLOCKED 0.001 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.001 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.001 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
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [fabric.name, proxmox.com, data.name]
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-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
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.)
> + 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 :)
> + },
> + {
> + text: gettext('Loopback IP'),
> + dataIndex: 'router_id',
> + width: 150,
> + renderer: function(value, metaData, rec) {
> + if (rec.data.type === 'fabric') {
> + return PVE.Utils.render_sdn_pending(rec, rec.data.loopback_prefix, 'loopback_prefix');
> + }
> +
> + return PVE.Utils.render_sdn_pending(rec, value, 'router_id');
> + },
> + },
> + {
> + text: gettext('Action'),
> + xtype: 'actioncolumn',
> + dataIndex: 'text',
> + width: 100,
> + items: [
> + {
> + handler: 'addActionTreeColumn',
> + getTip: (_v, _m, _rec) => gettext('Add'),
nit: "Add Node" would be better here for node rows, if that's possible.
> + 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
> + 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
> + 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
> +
> + let names = interfaces.map((iface) => {
> + let properties = Proxmox.Utils.parsePropertyString(iface);
nit: same here, as well for some other things in this file
> + return properties.name;
> + });
> +
> + names.sort();
> + return names.join(", ");
> + },
> + },
[..]
> +
> + 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
> + 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?
> +
> + 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.
> + handler: 'openAddOpenFabricWindow',
> + },
> + {
> + text: gettext('OSPF'),
^ same here
> + handler: 'openAddOspfWindow',
> + },
> + ],
> + },
[..]
> + 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?
> + openfabric = openfabric.map(x => {
> + if (x.state && x.state === 'new') {
> + Object.assign(x, x.pending);
> + }
> +
> + if (x.ty === 'fabric') {
> + return Object.assign(x, { protocol: "openfabric", name: x.fabric_id });
> + } else if (x.ty === 'node') {
> + let id = x.node_id.split("_");
> + return Object.assign(x,
> + {
> + protocol: "openfabric",
> + node: id[1],
> + fabric: id[0],
> + },
> + );
> + } else {
> + return x;
> + }
> + });
^ same for the openfabric mapping
> +
> + let allFabrics = openfabric.concat(ospf);
> + let fabrics = allFabrics.filter(e => e.ty === "fabric").map((fabric) => {
> + if (!fabric.state || fabric.state !== 'deleted') {
> + fabric.children = allFabrics.filter(e => e.ty === "node")
> + .filter((node) =>
> + node.fabric === fabric.name && node.protocol === fabric.protocol)
> + .map((node) => {
> + Object.assign(node, {
> + leaf: true,
> + type: 'node',
> + iconCls: 'fa fa-desktop x-fa-treepanel',
> + name: node.node,
> + _fabric: fabric.name,
> + });
> +
> + return node;
> + });
^ 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.
> + }
> +
> + 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
> + this.addAction(rec);
> + },
> +
[..]
> +
> + 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
> + 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')`.
> + },
> + 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.
> + });
> + },
> + });
> + },
> +
> + openAddOpenFabricWindow: function() {
> + let me = this;
> +
> + let window = Ext.create('PVE.sdn.Fabric.OpenFabric.Fabric.Edit', {
> + autoShow: true,
> + autoLoad: false,
> + isCreate: true,
> + });
> +
> + window.on('destroy', () => me.reload());
> + },
> +
> + openAddOspfWindow: function() {
> + let me = this;
> +
> + let window = Ext.create('PVE.sdn.Fabric.Ospf.Fabric.Edit', {
> + autoShow: true,
> + autoLoad: false,
> + isCreate: true,
> + });
> +
> + window.on('destroy', () => me.reload());
> + },
> +
> + init: function(view) {
> + let me = this;
> + me.reload();
> + },
> + },
> +});
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel