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
