public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Daniel Kral" <d.kral@proxmox.com>
To: "Dominik Rusovac" <d.rusovac@proxmox.com>, <pve-devel@lists.proxmox.com>
Subject: Re: [PATCH manager] ui: ha: add disarm/re-arm button
Date: Tue, 14 Apr 2026 15:30:29 +0200	[thread overview]
Message-ID: <DHSWXKSN8XXC.3HHSOBZ41QXXS@proxmox.com> (raw)
In-Reply-To: <20260414115156.188522-1-d.rusovac@proxmox.com>

Nice work, looks great!

I added some inline comments, but functionality-wise this works as
expected already.

On Tue Apr 14, 2026 at 1:51 PM CEST, Dominik Rusovac wrote:
> The button to disarm HA in either of the resource modes ('freeze' or
> 'ignore') is disabled as long as HA is disarmed. Analogously, the
> re-arming button is disabled as long as HA is not disarmed.
>
> The icons ('unlink' and 'link') are chosen to emphasize that
> "Disarm HA" and "Re-arm HA" are complements. There may be more
> suitable pairs of icons though.
>
> Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
> ---
>  www/manager6/ha/Status.js     | 106 ++++++++++++++++++++++++++++++++++
>  www/manager6/ha/StatusView.js |   8 +++
>  2 files changed, 114 insertions(+)
>
> diff --git a/www/manager6/ha/Status.js b/www/manager6/ha/Status.js
> index b0b0feb9..d955452d 100644
> --- a/www/manager6/ha/Status.js
> +++ b/www/manager6/ha/Status.js
> @@ -8,6 +8,107 @@ Ext.define('PVE.ha.Status', {
>          align: 'stretch',
>      },
>  
> +    viewModel: {
> +        data: {
> +            haDisarmed: false,
> +        },
> +    },
> +
> +    controller: {
> +        xclass: 'Ext.app.ViewController',
> +
> +        check_ha_status: function (isDisarmed) {

new method names should use cameCase according to our JS Style Guide [0]

> +            let vm = this.getViewModel();
> +            vm.set('haDisarmed', isDisarmed);
> +        },
> +
> +        disarm: function (comp) {

nit: maybe s/comp/menuItem/ would make it a little clearer where the
     information is coming from?

nit: maybe s/disarm/handleDisarmButton/ would also make it clearer where
     it is supposed to be used (instead of a generic method here)

> +            let me = this;
> +            let view = me.getView();
> +            let mode = comp.mode;
> +
> +            Ext.Msg.confirm(
> +                gettext('Confirm'),
> +                mode === 'freeze'
> +                    ? gettext("Are you sure you want to disarm HA with resource mode 'freeze'?")
> +                    : gettext("Are you sure you want to disarm HA with resource mode 'ignore'?"),

nit: the translation keys could be deduplicated with something like

    Ext.Msg.confirm(
        gettext('Confirm'),
        Ext.String.format(
            gettext("Are you sure you want to disarm HA with resource mode '{0}'?"),
            mode
        )
        ...
    );

Feel free to restructure the code snippet if you consider using it.

I also removed the mode === 'freeze' condition with just emplacing the
content of `mode`, but it would be nicer to use the same translation
keys as for the menu items (i.e., gettext('Freeze') and
gettext('Ignore')).

> +                function (btn) {
> +                    if (btn !== 'yes') {
> +                        return;
> +                    }
> +                    Proxmox.Utils.API2Request({
> +                        url: '/cluster/ha/status/disarm-ha',
> +                        params: { 'resource-mode': mode },
> +                        method: 'POST',
> +                        waitMsgTarget: view,
> +                        failure: (response) => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> +                    });
> +                },
> +            );
> +        },
> +
> +        rearm: function () {

nit: maybe s/rearm/handleRearmButton/ here as well?

> +            let me = this;
> +            let view = me.getView();
> +
> +            Ext.Msg.confirm(
> +                gettext('Confirm'),
> +                gettext('Are you sure you want to re-arm HA?'),
> +                function (btn) {
> +                    if (btn !== 'yes') {
> +                        return;
> +                    }
> +                    Proxmox.Utils.API2Request({
> +                        url: '/cluster/ha/status/arm-ha',
> +                        params: {},

nit: the params is not needed here

> +                        method: 'POST',
> +                        waitMsgTarget: view,
> +                        failure: (response) => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> +                    });
> +                },
> +            );
> +        },
> +    },
> +
> +    dockedItems: [
> +        {
> +            xtype: 'toolbar',
> +            dock: 'top',
> +            items: [
> +                {
> +                    text: gettext('Disarm HA'),
> +                    iconCls: 'fa fa-unlink',
> +                    bind: {
> +                        disabled: '{haDisarmed}',
> +                    },
> +                    menu: [
> +                        {
> +                            text: gettext('Freeze'),
> +                            iconCls: 'fa fa-snowflake-o',
> +                            mode: 'freeze',
> +                            handler: 'disarm',
> +                        },
> +                        {
> +                            text: gettext('Ignore'),
> +                            iconCls: 'fa fa-eye-slash',
> +                            mode: 'ignore',
> +                            handler: 'disarm',
> +                        },
> +                    ],
> +                },
> +                {
> +                    text: gettext('Re-arm HA'),

nit: wondering whether this should just be called 'Arm HA', but no hard
     feelings about this. If we go for that, should be changed for the
     other instances as well.

> +                    iconCls: 'fa fa-link',
> +                    bind: {
> +                        disabled: '{!haDisarmed}',
> +                    },
> +                    handler: 'rearm',
> +                },
> +            ],
> +        },
> +    ],
> +
> +

extra new-line that is removed with a `make -C www/manager6 tidy`

>      initComponent: function () {
>          var me = this;
>  
> @@ -30,6 +131,11 @@ Ext.define('PVE.ha.Status', {
>                  border: 0,
>                  collapsible: true,
>                  padding: '0 0 20 0',
> +                listeners: {
> +                    hastatuschange: function (isDisarmed) {
> +                        me.getController().check_ha_status(isDisarmed);
> +                    },
> +                },
>              },
>              {
>                  xtype: 'pveHAResourcesView',

[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing




  reply	other threads:[~2026-04-14 13:29 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-14 11:51 Dominik Rusovac
2026-04-14 13:30 ` Daniel Kral [this message]
2026-04-14 13:49   ` Dominik Rusovac
2026-04-15  6:44 ` Dominik Rusovac

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=DHSWXKSN8XXC.3HHSOBZ41QXXS@proxmox.com \
    --to=d.kral@proxmox.com \
    --cc=d.rusovac@proxmox.com \
    --cc=pve-devel@lists.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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal