* [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit @ 2023-10-10 6:39 Markus Frank 2023-10-10 6:39 ` [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList Markus Frank 2023-10-10 8:43 ` [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit Lukas Wagner 0 siblings, 2 replies; 6+ messages in thread From: Markus Frank @ 2023-10-10 6:39 UTC (permalink / raw) To: pbs-devel add commandline option for api function: DELETE /api2/json/nodes/{node}/disks/directory/{name} $ proxmox-backup-manager disk fs delete <datastoreid> Signed-off-by: Markus Frank <m.frank@proxmox.com> --- src/bin/proxmox_backup_manager/disk.rs | 29 ++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs index c3259b65..73cb95e6 100644 --- a/src/bin/proxmox_backup_manager/disk.rs +++ b/src/bin/proxmox_backup_manager/disk.rs @@ -317,6 +317,31 @@ 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; + let _result = match info.handler { + ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?, + _ => unreachable!(), + }; + + Ok(Value::Null) +} + pub fn filesystem_commands() -> CommandLineInterface { let cmd_def = CliCommandMap::new() .insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORE_MOUNTS)) @@ -325,6 +350,10 @@ pub fn filesystem_commands() -> CommandLineInterface { CliCommand::new(&API_METHOD_CREATE_DATASTORE_DISK) .arg_param(&["name"]) .completion_cb("disk", complete_disk_name), + ).insert( + "delete", + CliCommand::new(&API_METHOD_DELETE_DATASTORE_DISK) + .arg_param(&["name"]), ); cmd_def.into() -- 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList 2023-10-10 6:39 [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit Markus Frank @ 2023-10-10 6:39 ` Markus Frank 2023-10-10 8:43 ` Lukas Wagner 2023-10-10 8:43 ` [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit Lukas Wagner 1 sibling, 1 reply; 6+ messages in thread From: Markus Frank @ 2023-10-10 6:39 UTC (permalink / raw) To: pbs-devel With this patch it is possible to remove systemd mount units via the webui. Signed-off-by: Markus Frank <m.frank@proxmox.com> --- www/DirectoryList.js | 79 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/www/DirectoryList.js b/www/DirectoryList.js index 00531fd0..53aa76cc 100644 --- a/www/DirectoryList.js +++ b/www/DirectoryList.js @@ -7,6 +7,15 @@ Ext.define('PBS.admin.Directorylist', { emptyText: gettext('No Mount-Units found'), + viewModel: { + data: { + path: '', + }, + formulas: { + dirName: (get) => get('path')?.replace('/mnt/datastore/', '') || undefined, + }, + }, + controller: { xclass: 'Ext.app.ViewController', @@ -21,6 +30,27 @@ Ext.define('PBS.admin.Directorylist', { }).show(); }, + removeDirectory: function() { + let me = this; + let vm = me.getViewModel(); + + const dirName = vm.get('dirName'); + + if (!dirName) { + throw "no directory name specified"; + } + + Ext.create('Proxmox.window.SafeDestroy', { + url: `/nodes/localhost/disks/directory/${dirName}`, + item: { id: dirName }, + showProgress: true, + taskName: 'dirremove', + listeners: { + destroy: () => me.reload(), + }, + }).show(); + }, + reload: function() { let me = this; let store = me.getView().getStore(); @@ -49,6 +79,45 @@ Ext.define('PBS.admin.Directorylist', { text: gettext('Create') + ': Directory', handler: 'createDirectory', }, + '->', + { + xtype: 'tbtext', + data: { + dirName: undefined, + }, + bind: { + data: { + dirName: "{dirName}", + }, + }, + tpl: [ + '<tpl if="dirName">', + gettext('Directory') + ' {dirName}:', + '<tpl else>', + Ext.String.format(gettext('No {0} selected'), gettext('directory')), + '</tpl>', + ], + }, + { + text: gettext('More'), + iconCls: 'fa fa-bars', + disabled: true, + bind: { + disabled: '{!dirName}', + }, + menu: [ + { + text: gettext('Remove'), + itemId: 'remove', + iconCls: 'fa fa-fw fa-trash-o', + handler: 'removeDirectory', + disabled: true, + bind: { + disabled: '{!dirName}', + }, + }, + ], + }, ], columns: [ @@ -79,6 +148,16 @@ Ext.define('PBS.admin.Directorylist', { }, ], + listeners: { + activate: "reload", + selectionchange: function(model, selected) { + let me = this; + let vm = me.getViewModel(); + + vm.set('path', selected[0]?.data.path || ''); + }, + }, + store: { fields: ['path', 'device', 'filesystem', 'options', 'unitfile'], proxy: { -- 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList 2023-10-10 6:39 ` [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList Markus Frank @ 2023-10-10 8:43 ` Lukas Wagner 2023-10-10 9:05 ` Markus Frank 0 siblings, 1 reply; 6+ messages in thread From: Lukas Wagner @ 2023-10-10 8:43 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Markus Frank Comments inline. On 10/10/23 08:39, Markus Frank wrote: > With this patch it is possible to remove systemd mount units via the webui. > > Signed-off-by: Markus Frank <m.frank@proxmox.com> > --- > www/DirectoryList.js | 79 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/www/DirectoryList.js b/www/DirectoryList.js > index 00531fd0..53aa76cc 100644 > --- a/www/DirectoryList.js > +++ b/www/DirectoryList.js > @@ -7,6 +7,15 @@ Ext.define('PBS.admin.Directorylist', { > > emptyText: gettext('No Mount-Units found'), > > + viewModel: { > + data: { > + path: '', > + }, > + formulas: { > + dirName: (get) => get('path')?.replace('/mnt/datastore/', '') || undefined, > + }, > + }, > + > controller: { > xclass: 'Ext.app.ViewController', > > @@ -21,6 +30,27 @@ Ext.define('PBS.admin.Directorylist', { > }).show(); > }, > > + removeDirectory: function() { > + let me = this; > + let vm = me.getViewModel(); > + > + const dirName = vm.get('dirName'); Nit: As per our JS style guidelines I'd use 'let' here. For objects, 'const' only prohibits reassignemnt, but not modification of the actual object. > + > + if (!dirName) { > + throw "no directory name specified"; > + } > + > + Ext.create('Proxmox.window.SafeDestroy', { > + url: `/nodes/localhost/disks/directory/${dirName}`, > + item: { id: dirName }, > + showProgress: true, > + taskName: 'dirremove', > + listeners: { > + destroy: () => me.reload(), > + }, > + }).show(); > + }, > + > reload: function() { > let me = this; > let store = me.getView().getStore(); > @@ -49,6 +79,45 @@ Ext.define('PBS.admin.Directorylist', { > text: gettext('Create') + ': Directory', > handler: 'createDirectory', > }, > + '->', > + { > + xtype: 'tbtext', > + data: { > + dirName: undefined, > + }, > + bind: { > + data: { > + dirName: "{dirName}", > + }, > + }, > + tpl: [ > + '<tpl if="dirName">', > + gettext('Directory') + ' {dirName}:', > + '<tpl else>', > + Ext.String.format(gettext('No {0} selected'), gettext('directory')), > + '</tpl>', > + ], > + }, Indentation is off here (only spaces). > + { > + text: gettext('More'), > + iconCls: 'fa fa-bars', > + disabled: true, > + bind: { > + disabled: '{!dirName}', > + }, > + menu: [ > + { > + text: gettext('Remove'), > + itemId: 'remove', > + iconCls: 'fa fa-fw fa-trash-o', > + handler: 'removeDirectory', > + disabled: true, > + bind: { > + disabled: '{!dirName}', > + }, > + }, > + ], > + }, > ], > > columns: [ > @@ -79,6 +148,16 @@ Ext.define('PBS.admin.Directorylist', { > }, > ], > > + listeners: { > + activate: "reload", > + selectionchange: function(model, selected) { > + let me = this; > + let vm = me.getViewModel(); > + > + vm.set('path', selected[0]?.data.path || ''); > + }, > + }, > + > store: { > fields: ['path', 'device', 'filesystem', 'options', 'unitfile'], > proxy: { A high-level comment about the UI changes: In most places in our UI, we have a dedicated 'Remove' button right next to the 'Add/Create' and 'Edit' button: [Add] [Edit] [Remove] We don't need 'Edit' here, but why not just add the button right next to the 'Create: Directory' button? What's the rationale of moving the remove button to a menu on the right-hand side? -- - Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList 2023-10-10 8:43 ` Lukas Wagner @ 2023-10-10 9:05 ` Markus Frank 2023-10-10 9:13 ` Lukas Wagner 0 siblings, 1 reply; 6+ messages in thread From: Markus Frank @ 2023-10-10 9:05 UTC (permalink / raw) To: Lukas Wagner, Proxmox Backup Server development discussion Thanks for the review. Comments inline On 10/10/23 10:43, Lukas Wagner wrote: > Comments inline. > > On 10/10/23 08:39, Markus Frank wrote: >> With this patch it is possible to remove systemd mount units via the webui. >> >> Signed-off-by: Markus Frank <m.frank@proxmox.com> >> --- >> www/DirectoryList.js | 79 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 79 insertions(+) >> >> diff --git a/www/DirectoryList.js b/www/DirectoryList.js >> index 00531fd0..53aa76cc 100644 >> --- a/www/DirectoryList.js >> +++ b/www/DirectoryList.js >> @@ -7,6 +7,15 @@ Ext.define('PBS.admin.Directorylist', { >> emptyText: gettext('No Mount-Units found'), >> + viewModel: { >> + data: { >> + path: '', >> + }, >> + formulas: { >> + dirName: (get) => get('path')?.replace('/mnt/datastore/', '') || undefined, >> + }, >> + }, >> + >> controller: { >> xclass: 'Ext.app.ViewController', >> @@ -21,6 +30,27 @@ Ext.define('PBS.admin.Directorylist', { >> }).show(); >> }, >> + removeDirectory: function() { >> + let me = this; >> + let vm = me.getViewModel(); >> + >> + const dirName = vm.get('dirName'); > Nit: As per our JS style guidelines I'd use 'let' here. For objects, > 'const' only prohibits reassignemnt, but not modification of the > actual object. Okay > >> + >> + if (!dirName) { >> + throw "no directory name specified"; >> + } >> + >> + Ext.create('Proxmox.window.SafeDestroy', { >> + url: `/nodes/localhost/disks/directory/${dirName}`, >> + item: { id: dirName }, >> + showProgress: true, >> + taskName: 'dirremove', >> + listeners: { >> + destroy: () => me.reload(), >> + }, >> + }).show(); >> + }, >> + >> reload: function() { >> let me = this; >> let store = me.getView().getStore(); >> @@ -49,6 +79,45 @@ Ext.define('PBS.admin.Directorylist', { >> text: gettext('Create') + ': Directory', >> handler: 'createDirectory', >> }, >> + '->', >> + { >> + xtype: 'tbtext', >> + data: { >> + dirName: undefined, >> + }, >> + bind: { >> + data: { >> + dirName: "{dirName}", >> + }, >> + }, >> + tpl: [ >> + '<tpl if="dirName">', >> + gettext('Directory') + ' {dirName}:', >> + '<tpl else>', >> + Ext.String.format(gettext('No {0} selected'), gettext('directory')), >> + '</tpl>', >> + ], >> + }, > Indentation is off here (only spaces). Okay > >> + { >> + text: gettext('More'), >> + iconCls: 'fa fa-bars', >> + disabled: true, >> + bind: { >> + disabled: '{!dirName}', >> + }, >> + menu: [ >> + { >> + text: gettext('Remove'), >> + itemId: 'remove', >> + iconCls: 'fa fa-fw fa-trash-o', >> + handler: 'removeDirectory', >> + disabled: true, >> + bind: { >> + disabled: '{!dirName}', >> + }, >> + }, >> + ], >> + }, >> ], >> columns: [ >> @@ -79,6 +148,16 @@ Ext.define('PBS.admin.Directorylist', { >> }, >> ], >> + listeners: { >> + activate: "reload", >> + selectionchange: function(model, selected) { >> + let me = this; >> + let vm = me.getViewModel(); >> + >> + vm.set('path', selected[0]?.data.path || ''); >> + }, >> + }, >> + >> store: { >> fields: ['path', 'device', 'filesystem', 'options', 'unitfile'], >> proxy: { > > A high-level comment about the UI changes: > In most places in our UI, we have a dedicated 'Remove' button right next > to the 'Add/Create' and 'Edit' button: > > [Add] [Edit] [Remove] > > We don't need 'Edit' here, but why not just add the button right next > to the 'Create: Directory' button? > What's the rationale of moving the remove button to a menu on the > right-hand side? Because I think it would be better if it looks the same in pve & pbs. I think the reason for the menu in pve is that it is more difficult to accidentally remove a directory. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList 2023-10-10 9:05 ` Markus Frank @ 2023-10-10 9:13 ` Lukas Wagner 0 siblings, 0 replies; 6+ messages in thread From: Lukas Wagner @ 2023-10-10 9:13 UTC (permalink / raw) To: Markus Frank, Proxmox Backup Server development discussion On 10/10/23 11:05, Markus Frank wrote: >>> store: { >>> fields: ['path', 'device', 'filesystem', 'options', 'unitfile'], >>> proxy: { >> >> A high-level comment about the UI changes: >> In most places in our UI, we have a dedicated 'Remove' button right next >> to the 'Add/Create' and 'Edit' button: >> >> [Add] [Edit] [Remove] >> >> We don't need 'Edit' here, but why not just add the button right next >> to the 'Create: Directory' button? >> What's the rationale of moving the remove button to a menu on the >> right-hand side? > Because I think it would be better if it looks the same in pve & pbs. > I think the reason for the menu in pve is that it is more difficult to > accidentally remove a directory. Ah yes, you are right, I didn't compare it to PVE. In that case it's probably fine and you can keep it that way. -- - Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit 2023-10-10 6:39 [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit Markus Frank 2023-10-10 6:39 ` [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList Markus Frank @ 2023-10-10 8:43 ` Lukas Wagner 1 sibling, 0 replies; 6+ messages in thread From: Lukas Wagner @ 2023-10-10 8:43 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Markus Frank 1/2 looks good to me, 2/2 might need a v2. Consider patch 1/2: Tested-by: Lukas Wagner <l.wagner@proxmox.com> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com> On 10/10/23 08:39, Markus Frank wrote: > add commandline option for api function: > DELETE /api2/json/nodes/{node}/disks/directory/{name} > > $ proxmox-backup-manager disk fs delete <datastoreid> > > Signed-off-by: Markus Frank <m.frank@proxmox.com> > --- > src/bin/proxmox_backup_manager/disk.rs | 29 ++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs > index c3259b65..73cb95e6 100644 > --- a/src/bin/proxmox_backup_manager/disk.rs > +++ b/src/bin/proxmox_backup_manager/disk.rs > @@ -317,6 +317,31 @@ 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; > + let _result = match info.handler { > + ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?, > + _ => unreachable!(), > + }; > + > + Ok(Value::Null) > +} > + > pub fn filesystem_commands() -> CommandLineInterface { > let cmd_def = CliCommandMap::new() > .insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORE_MOUNTS)) > @@ -325,6 +350,10 @@ pub fn filesystem_commands() -> CommandLineInterface { > CliCommand::new(&API_METHOD_CREATE_DATASTORE_DISK) > .arg_param(&["name"]) > .completion_cb("disk", complete_disk_name), > + ).insert( > + "delete", > + CliCommand::new(&API_METHOD_DELETE_DATASTORE_DISK) > + .arg_param(&["name"]), > ); > > cmd_def.into() -- - Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-10 9:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-10 6:39 [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit Markus Frank 2023-10-10 6:39 ` [pbs-devel] [PATCH proxmox-backup 2/2] ui: add Remove button for DirectoryList Markus Frank 2023-10-10 8:43 ` Lukas Wagner 2023-10-10 9:05 ` Markus Frank 2023-10-10 9:13 ` Lukas Wagner 2023-10-10 8:43 ` [pbs-devel] [PATCH proxmox-backup 1/2] cli: add option to remove systemd mount unit Lukas Wagner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox