From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 35ADA60C32
for <pbs-devel@lists.proxmox.com>; Fri, 14 Aug 2020 10:13:22 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 282061FB23
for <pbs-devel@lists.proxmox.com>; Fri, 14 Aug 2020 10:13:22 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[212.186.127.180])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 16E941FB15
for <pbs-devel@lists.proxmox.com>; Fri, 14 Aug 2020 10:13:18 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D1C88445FB
for <pbs-devel@lists.proxmox.com>; Fri, 14 Aug 2020 10:13:17 +0200 (CEST)
To: pbs-devel@lists.proxmox.com
References: <20200813105853.144386-1-h.laimer@proxmox.com>
<20200813105853.144386-3-h.laimer@proxmox.com>
From: Dominik Csapak <d.csapak@proxmox.com>
Message-ID: <9e05e98d-05a6-5370-8dcc-5b761c387f28@proxmox.com>
Date: Fri, 14 Aug 2020 10:13:13 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:80.0) Gecko/20100101
Thunderbird/80.0
MIME-Version: 1.0
In-Reply-To: <20200813105853.144386-3-h.laimer@proxmox.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.646 Adjusted score from AWL reputation of From: address
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.001 Looks like a legit reply (A)
RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/,
medium trust
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [disk.rs, result.data, proxmox.com, item.id]
Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 2/2] ui/cli: added support
for the removal of mount-units
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 14 Aug 2020 08:13:22 -0000
aside from dietmars comments, ui code looks mostly ok
some comments inline
On 8/13/20 12:58 PM, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/bin/proxmox_backup_manager/disk.rs | 29 +++-
> www/DirectoryList.js | 150 ++++++++++--------
> www/Makefile | 1 +
> www/window/SafeRemove.js | 209 +++++++++++++++++++++++++
> 4 files changed, 325 insertions(+), 64 deletions(-)
> create mode 100644 www/window/SafeRemove.js
>
> diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs
> index a93a6f6b..b7eec592 100644
> --- a/src/bin/proxmox_backup_manager/disk.rs
> +++ b/src/bin/proxmox_backup_manager/disk.rs
> @@ -319,6 +319,32 @@ async fn create_datastore_disk(
> Ok(Value::Null)
> }
>
> +#[api(
> + input: {
> + properties: {
> + name: {
> + schema: DATASTORE_SCHEMA,
> + },
> + },
> + },
> +)]
> +/// Remove a Filesystem mounted under '/mnt/datastore/<name>'.".
> +async fn delete_datastore_disk(
> + mut param: Value,
> + rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> +
> + param["node"] = "localhost".into();
> +
> + let info = &api2::node::disks::directory::API_METHOD_DELETE_DATASTORE_DISK;
> + match info.handler {
> + ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> + _ => unreachable!(),
> + };
> +
> + Ok(Value::Null)
> +}
> +
> pub fn filesystem_commands() -> CommandLineInterface {
>
> let cmd_def = CliCommandMap::new()
> @@ -327,7 +353,8 @@ pub fn filesystem_commands() -> CommandLineInterface {
> CliCommand::new(&API_METHOD_CREATE_DATASTORE_DISK)
> .arg_param(&["name"])
> .completion_cb("disk", complete_disk_name)
> - );
> + ).insert("remove", CliCommand::new(&API_METHOD_DELETE_DATASTORE_DISK)
> + .arg_param(&["name"]));
>
> cmd_def.into()
> }
> diff --git a/www/DirectoryList.js b/www/DirectoryList.js
> index 00531fd0..6ec91e6a 100644
> --- a/www/DirectoryList.js
> +++ b/www/DirectoryList.js
> @@ -8,83 +8,107 @@ Ext.define('PBS.admin.Directorylist', {
> emptyText: gettext('No Mount-Units found'),
>
> controller: {
> - xclass: 'Ext.app.ViewController',
> + xclass: 'Ext.app.ViewController',
>
> - createDirectory: function() {
> - let me = this;
> - Ext.create('PBS.window.CreateDirectory', {
> - listeners: {
> - destroy: function() {
> - me.reload();
> - },
> - },
> - }).show();
> - },
> + createDirectory: function () {
> + console.log(this);
i guess this is leftover?
> + let me = this;
> + Ext.create('PBS.window.CreateDirectory', {
> + listeners: {
> + destroy: function () {
> + me.reload();
> + },
> + },
> + }).show();
> + },
>
> - reload: function() {
> - let me = this;
> - let store = me.getView().getStore();
> - store.load();
> - store.sort();
> - },
> + removeDir: function () {
> + let me = this;
> + let view = me.getView();
> + let rec = view.getSelection();
> + let dialog = Ext.create('PBS.window.SafeDestroy', {
> + url: rec[0].data.path.replace(
> + '/mnt/datastore/',
> + '/nodes/localhost/disks/directory/'),
> + item: {type: 'Dir', id: rec[0].data.path.replace('/mnt/datastore/', '')},
that seems brittle... would it not be better to return the 'id' from the
api instead of modifying the path in the frontend?
> + note: 'Data and partitions on the disk will be left untouched.'
i think it would be better to put that text in a 'gettext' so it can be
translated
> + });
> + dialog.on('destroy', function() {
> + me.reload();
> + });
> + dialog.show();
i would here prefer the same codestyle as in createDirectory, so
Ext.create('', {
...
listeners: {
destroy: ...
},
}).show();
> + },
>
> - init: function(view) {
> - let me = this;
> - Proxmox.Utils.monStoreErrors(view, view.getStore(), true);
> - me.reload();
> - },
> - },
> + reload: function () {
> + let me = this;
> + let store = me.getView().getStore();
> + store.load();
> + store.sort();
> + },
>
> + init: function (view) {
> + let me = this;
> + Proxmox.Utils.monStoreErrors(view, view.getStore(), true);
> + me.reload();
> + },
> + },
>
> rootVisible: false,
> useArrows: true,
>
> tbar: [
> - {
> - text: gettext('Reload'),
> - iconCls: 'fa fa-refresh',
> - handler: 'reload',
> - },
> - {
> - text: gettext('Create') + ': Directory',
> - handler: 'createDirectory',
> - },
> + {
> + text: gettext('Reload'),
> + iconCls: 'fa fa-refresh',
> + handler: 'reload',
> + },
> + {
> + text: gettext('Create') + ': Directory',
> + handler: 'createDirectory',
> + },
> + {
> + xtype: 'proxmoxButton',
> + text: gettext('Remove'),
> + handler: 'removeDir',
> + disabled: true,
> + iconCls: 'fa fa-trash-o'
> + }
> ],
>
> columns: [
> - {
> - text: gettext('Path'),
> - dataIndex: 'path',
> - flex: 1,
> - },
> - {
> - header: gettext('Device'),
> - flex: 1,
> - dataIndex: 'device',
> - },
> - {
> - header: gettext('Filesystem'),
> - width: 100,
> - dataIndex: 'filesystem',
> - },
> - {
> - header: gettext('Options'),
> - width: 100,
> - dataIndex: 'options',
> - },
> - {
> - header: gettext('Unit File'),
> - hidden: true,
> - dataIndex: 'unitfile',
> - },
> + {
> + text: gettext('Path'),
> + dataIndex: 'path',
> + flex: 1,
> + },
> + {
> + header: gettext('Device'),
> + flex: 1,
> + dataIndex: 'device',
> + },
> + {
> + header: gettext('Filesystem'),
> + width: 100,
> + dataIndex: 'filesystem',
> + },
> + {
> + header: gettext('Options'),
> + width: 100,
> + dataIndex: 'options',
> + },
> + {
> + header: gettext('Unit File'),
> + hidden: true,
> + dataIndex: 'unitfile',
> + },
> ],
>
> store: {
> - fields: ['path', 'device', 'filesystem', 'options', 'unitfile'],
> - proxy: {
> - type: 'proxmox',
> - url: '/api2/json/nodes/localhost/disks/directory',
> - },
> - sorters: 'path',
> + fields: ['path', 'device', 'filesystem', 'options', 'unitfile'],
> + proxy: {
> + type: 'proxmox',
> + url: '/api2/json/nodes/localhost/disks/directory',
> + },
> + sorters: 'path',
> },
> });
> diff --git a/www/Makefile b/www/Makefile
> index edce8cb3..57db54ee 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -23,6 +23,7 @@ JSSRC= \
> window/SyncJobEdit.js \
> window/ACLEdit.js \
> window/DataStoreEdit.js \
> + window/SafeRemove.js \
> window/CreateDirectory.js \
> window/ZFSCreate.js \
> window/FileBrowser.js \
> diff --git a/www/window/SafeRemove.js b/www/window/SafeRemove.js
> new file mode 100644
> index 00000000..fdbefeae
> --- /dev/null
> +++ b/www/window/SafeRemove.js
> @@ -0,0 +1,209 @@
> +/* Popup a message window
> + * where the user has to manually enter the resource ID
> + * to enable the destroy button
> + * (mostly taken from PVE-Manager(git.proxmox.com/git/pve-manager.git))
> + */
i would rather prefer to refactor that safedestroy window into
the widget-toolkit
aside from handling the new type is there any reason
not to refactor and reuse?
i diffed over the two files and mostly
saw the different checkbox (which can be handled by showing/hiding the
correct one, or have the text and parameter as arguments)
and the note (which can also be shown/hidden by type i guess)
> +Ext.define('PBS.window.SafeDestroy', {
> + extend: 'Ext.window.Window',
> +
> + title: gettext('Confirm'),
> + modal: true,
> + buttonAlign: 'center',
> + bodyPadding: 10,
> + width: 450,
> + layout: {type: 'hbox'},
> + defaultFocus: 'confirmField',
> + showProgress: false,
> +
> + config: {
> + item: {
> + id: undefined,
> + type: undefined
> + },
> + url: undefined,
> + note: undefined,
> + params: {}
> + },
> +
> + getParams: function () {
> + var me = this;
> + var wipeCheckbox = me.lookupReference('wipeCheckbox');
> + if (wipeCheckbox.checked) {
> + me.params.wipe = 1;
> + }
> + if (Ext.Object.isEmpty(me.params)) {
> + return '';
> + }
> + return '?' + Ext.Object.toQueryString(me.params);
> + },
> +
> + controller: {
> +
> + xclass: 'Ext.app.ViewController',
> +
> + control: {
> + 'field[name=confirm]': {
> + change: function (f, value) {
> + var view = this.getView();
> + var removeButton = this.lookupReference('removeButton');
> + if (value === view.getItem().id.toString()) {
> + removeButton.enable();
> + } else {
> + removeButton.disable();
> + }
> + },
> + specialkey: function (field, event) {
> + var removeButton = this.lookupReference('removeButton');
> + if (!removeButton.isDisabled() && event.getKey() == event.ENTER) {
> + removeButton.fireEvent('click', removeButton, event);
> + }
> + }
> + },
> + 'button[reference=removeButton]': {
> + click: function () {
> + var view = this.getView();
> + Proxmox.Utils.API2Request({
> + url: view.getUrl() + view.getParams(),
> + method: 'DELETE',
> + waitMsgTarget: view,
> + failure: function (response, opts) {
> + view.close();
> + Ext.Msg.alert('Error', response.htmlStatus);
> + },
> + success: function (response, options) {
> + var hasProgressBar = view.showProgress &&
> + response.result.data ? true : false;
> +
> + if (hasProgressBar) {
> + // stay around so we can trigger our close events
> + // when background action is completed
> + view.hide();
> +
> + var upid = response.result.data;
> + var win = Ext.create('Proxmox.window.TaskProgress', {
> + upid: upid,
> + listeners: {
> + destroy: function () {
> + view.close();
> + }
> + }
> + });
> + win.show();
> + } else {
> + view.close();
> + }
> + }
> + });
> + }
> + }
> + }
> + },
> +
> + items: [
> + {
> + xtype: 'component',
> + cls: [Ext.baseCSSPrefix + 'message-box-icon',
> + Ext.baseCSSPrefix + 'message-box-warning',
> + Ext.baseCSSPrefix + 'dlg-icon']
> + },
> + {
> + xtype: 'container',
> + flex: 1,
> + layout: {
> + type: 'vbox',
> + align: 'stretch'
> + },
> + items: [
> + {
> + xtype: 'component',
> + reference: 'messageCmp'
> + },
> + {
> + itemId: 'confirmField',
> + reference: 'confirmField',
> + xtype: 'textfield',
> + name: 'confirm',
> + labelWidth: 300,
> + hideTrigger: true,
> + allowBlank: false
> + },
> + {
> + xtype: 'proxmoxcheckbox',
> + name: 'wipe',
> + reference: 'wipeCheckbox',
> + boxLabel: gettext('Wipe'),
> + checked: false,
> + autoEl: {
> + tag: 'div',
> + 'data-qtip': gettext('Wipe disk after the removal of mountpoint')
> + }
> + },
> + {
> + xtype: 'container',
> + flex: 1,
> + layout: {
> + type: 'vbox',
> + align: 'middle'
> + },
> + height: 25,
> + items: [
> + {
> + xtype: 'component',
> + reference: 'noteCmp'
> + },
> + ]
> + },
> + ]
> + }
> + ],
> + buttons: [
> + {
> + reference: 'removeButton',
> + text: gettext('Remove'),
> + disabled: true
> + }
> + ],
> +
> + initComponent: function () {
> + var me = this;
> +
> + me.callParent();
> +
> + var item = me.getItem();
> +
> + if (!Ext.isDefined(item.id)) {
> + throw "no ID specified";
> + }
> +
> + if (!Ext.isDefined(item.type)) {
> + throw "no Disk type specified";
> + }
> +
> + var messageCmp = me.lookupReference('messageCmp');
> + var noteCmp = me.lookupReference('noteCmp');
> + var msg;
> +
> + if (item.type === 'Dir') {
> + msg = `Directory ${item.id} - Remove`
> + } else {
> + throw "unknown item type specified";
> + }
> +
> + if(me.getNote()) {
> + noteCmp.setHtml(`<small>${me.getNote()}</small>`);
> + }
> +
> + messageCmp.setHtml(msg);
> +
> + if (item.type === 'Dir') {
> + let wipeCheckbox = me.lookupReference('wipeCheckbox');
> + wipeCheckbox.setDisabled(true);
> + wipeCheckbox.setHidden(true);
> + }
> +
> + var confirmField = me.lookupReference('confirmField');
> + msg = gettext('Please enter the ID to confirm') +
> + ' (' + item.id + ')';
> + confirmField.setFieldLabel(msg);
> + }
> +});
>