On 12/5/24 18:28, Gabriel Goller wrote:
On 04.12.2024 10:25, Dominik Csapak wrote:
high level comments/questions (i know they're not you're patches exactly, but
still):
* maybe it would be better to integrate this into the AgentIPView for vms?
AFAICS the code is very similar and probably just needs a few adaptions
to work there too (url,parsing, etc.)
I'm not opposed to have two components, but then we should at least have
a good reason in the commit message why this was not done, e.g.
the data structures are too different, or something like that
* IMHO we should keep the columns consistent between VMs and Containers,
So either we change the AgentIPView to name/mac/ipv4/ipv6 too
or we combine the ipv4/ipv6 here
Hmm I had a quick glance at the AgentIPView and while there is a lot of
the same code, I fear that I will have to do a lot of "if container do
this, otherwise do that".
wouldn't it also be possible to define some callbacks e.g. one for
parsing/assigning,
one for privilege checking etc. (depending on what's exactly different)
Although we could make one generic panel for both, removing all the
mentions of "Agent" in the name/title though.
Let me know what you prefer!
If the amount of if/else or callbacks, etc. is not immensely large,
i'd go for unifying them (and since the widget itself is not much
code to begin with, my guess is that it's just a few differences)
OFC if it turns out that some things must be done fundamentally different
between the two, you can keep it separated.
(Sometimes one has to try some options and look at the result
to see what's the better approach, so it's hard to give
a firm recommendation beforehand)
+ startIPStore: function(store, records, success) {
+ var me = this;
+ let state = store.getById('status');
+
+ me.running = state && state.data.value === 'running';
+
+ var caps = Ext.state.Manager.get('GuiCap');
+
+ if (!caps.vms['VM.Monitor']) {
the api call for getting the interfaces does not really need this permission?
the api only needs 'vm.audit' for this information, so this check should
reflect that
Ack.
+ var errorText = gettext("Requires '{0}' Privileges");
+ me.updateStatus(false, Ext.String.format(errorText, 'VM.Monitor'));
+ return;
+ }
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
