Re: [pve-devel] [PATCH manager 2/2] lxc: show IPs in summary view

[Dominik Csapak <d.csapak@proxmox.com>]

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