From: Matthias Heiserer <m.heiserer@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH manager] fix #2435: GUI: LXC summary: Add privileged status and os type
Date: Fri, 16 Sep 2022 14:13:33 +0200 [thread overview]
Message-ID: <90a45125-5a7d-afe3-f339-620b29ed98d7@proxmox.com> (raw)
In-Reply-To: <67918266-9afc-0830-8bf8-2e7399a4dc42@proxmox.com>
On 09.09.2022 08:20, Thomas Lamprecht wrote:
> Can you please split this into two commits, first the unpriv one and then the OS type one?
>
> Am 08/08/2022 um 13:48 schrieb Matthias Heiserer:
>> ---
>>
>> I'm a bit unsure about the icons, there are probably better ones, but I couldn't
>> think of/find any. One alternative for OS type would be the OS logos.
>
> That'd be nice(r) and make it stand out more. But, while we already got the Debian
> one available (currently used for the apt repo gui), you'd need to assemble the other ones
> yourself, ensuring to only pull the official ones with a compatible license.
>
> After a quick search I found a project[0] that would seem to provide the desired logos,
> we could pull out the for us relevant one and ship it in manager, or alternatively create
> a full new package with all logos and CSS files exposed, wouldn't be _that_ much more work
> and possible a bit cleaner.
>
> [0]: https://github.com/lukas-w/font-logos
>
Sure
>>
>> www/manager6/.lint-incremental | 0
>> www/manager6/panel/GuestStatusView.js | 44 +++++++++++++++++++++++++++
>> 2 files changed, 44 insertions(+)
>> create mode 100644 www/manager6/.lint-incremental
>>
>> diff --git a/www/manager6/.lint-incremental b/www/manager6/.lint-incremental
>> new file mode 100644
>> index 00000000..e69de29b
>> diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
>> index 8db1f492..96e9adea 100644
>> --- a/www/manager6/panel/GuestStatusView.js
>> +++ b/www/manager6/panel/GuestStatusView.js
>> @@ -11,6 +11,30 @@ Ext.define('PVE.panel.GuestStatusView', {
>> };
>> },
>>
>> + controller: {
>> + xclass: 'Ext.app.ViewController',
>> +
>> + init: function(view) {
>> + const nodename = view.pveSelNode.get("node");
>> + const id = view.pveSelNode.get("vmid");
>> + const me = this;
>> + Proxmox.Utils.API2Request({
>> + url: `/api2/extjs/nodes/${nodename}/lxc/${id}/config`,
>> + waitMsgTarget: view,
>
> I could swear we got the config already available somewhere in the hierarchy there,
> and checking the debug console's network view I saw a config load in the summary
> panel, naturally without your patch applied yet ;-), for the pmxNotes panel.
>
> I really want to avoid loading the same stuff in the same view multiple times.
>
> As we don't need the config often in the LXC panels (just Summary and Network,
> otherwise we use the slightly odd /pending endpoint) I'd not add it in the
> PVE.lxc.Config, but rather load it once in the pveGuestSummary
>
> Then you can pass it along to the notes and status panels for reuse on declaration.
> To avoid forced dependency bump and browser cache issues please keep the widget
> toolkits notes view's config load as fallback.
Correct me if I'm wrong, but we can't pass it to Network (because
different Tab), and passing to to pmxNotes would result in a circular
dependency.
I guess we can store it in some global object?
>
>
>> + method: 'GET',
>> + success: function(response) {
>
> fyi: if you only ever need to access child properties and you can be sure that they
> exist (normally fine in the "success" callback) you could use some destructuring
> here:
>
> success: ({ result }) => { ... },
>
> style wise you could naturally also do:
>
> success: ({ result: { data: { ostype, unprivileged } } }) => { ... }
>
> but it increases assumptions about the return type, as you do not check for
> definedness or use nullish coalescing below anyway it would stay at the same
> risk level w.r.t exceptions in this case, and as said, should be fine for the
> success callback (no hard feelings to either variant on my side)
>
In that case, I'll use the current format without destructuring, I feel
like it's more readable.
prev parent reply other threads:[~2022-09-16 12:21 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-08 11:48 Matthias Heiserer
2022-09-09 6:20 ` Thomas Lamprecht
2022-09-16 12:13 ` Matthias Heiserer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=90a45125-5a7d-afe3-f339-620b29ed98d7@proxmox.com \
--to=m.heiserer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox