Re: [pve-devel] [PATCH manager] Improve storage selection on restore

2020-05-06 Thread Fabian Ebner 

On 5/5/20 1:40 PM, Thomas Lamprecht wrote:

On 5/5/20 1:20 PM, Fabian Ebner wrote:

Previously, the blank '' would be passed along and lead to a
parameter verfication failure.

For LXC the default behavior in the backend is to use 'local' as
the storage, so disallow blank and auto-select the first storage
supporting 'rootdir' instead.

For QEMU the default behavior in the backend is to use the
original layout from the backup configuration file, which
makes sense to use as the default in the GUI as well.

Signed-off-by: Fabian Ebner 
---
  www/manager6/window/Restore.js | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
index 3f9ad3bc..09c8a997 100644
--- a/www/manager6/window/Restore.js
+++ b/www/manager6/window/Restore.js
@@ -24,7 +24,11 @@ Ext.define('PVE.window.Restore', {
value: '',
fieldLabel: gettext('Storage'),
storageContent: (me.vmtype === 'lxc') ? 'rootdir' : 'images',
-   allowBlank: true
+   // when restoring a container without specifying a storage, the 
backend defaults
+   // to 'local', which is unintuitive and 'rootdir' might not even be 
allowed on it
+   allowBlank: (me.vmtype === 'lxc') ? false : true,
+   emptyText: (me.vmtype === 'lxc') ? '' : gettext('use old layout'),


1. We capitalize gettext strings normally, at least the first word
2. "Use old layout" is quite confusing, IMO, maybe "From Backup Configuration"
or "Backup Layout"



Yes, "From Backup Configuration" is better.


I mean in the longer run we want to probably stream line behavior between CT
and VM so that both try to use the ones from the backup config or even pass
a volume to storage map (as advanced feature).



+   autoSelect: (me.vmtype === 'lxc') ? true : false,


in general I like to avoid ternary operations if easily possible, for boolean
rvalues that's a must, me.vmtype === 'lxc' is already true or false ;-)




I thought it might make sense for readability in this case, because 
having the same expression four times in a row, one can read the first 
"column" as what happens for LXC and the second "column" as what happens 
for QEMU. But I can of course change it in v2.



});
  
  	var IDfield;

@@ -135,16 +139,15 @@ Ext.define('PVE.window.Restore', {
var submitBtn = Ext.create('Ext.Button', {
text: gettext('Restore'),
handler: function(){
-   var storage = storagesel.getValue();
var values = form.getValues();
  
  		var params = {

-   storage: storage,
vmid: me.vmid || values.vmid,
force: me.vmid ? 1 : 0
};
if (values.unique) { params.unique = 1; }
if (values.start) { params.start = 1; }
+   if (values.storage) { params.storage = values.storage; }
  
  		if (values.bwlimit !== undefined) {

params.bwlimit = values.bwlimit;





___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager] Improve storage selection on restore

2020-05-05 Thread Thomas Lamprecht 
On 5/5/20 1:20 PM, Fabian Ebner wrote:
> Previously, the blank '' would be passed along and lead to a
> parameter verfication failure.
> 
> For LXC the default behavior in the backend is to use 'local' as
> the storage, so disallow blank and auto-select the first storage
> supporting 'rootdir' instead.
> 
> For QEMU the default behavior in the backend is to use the
> original layout from the backup configuration file, which
> makes sense to use as the default in the GUI as well.
> 
> Signed-off-by: Fabian Ebner 
> ---
>  www/manager6/window/Restore.js | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
> index 3f9ad3bc..09c8a997 100644
> --- a/www/manager6/window/Restore.js
> +++ b/www/manager6/window/Restore.js
> @@ -24,7 +24,11 @@ Ext.define('PVE.window.Restore', {
>   value: '',
>   fieldLabel: gettext('Storage'),
>   storageContent: (me.vmtype === 'lxc') ? 'rootdir' : 'images',
> - allowBlank: true
> + // when restoring a container without specifying a storage, the 
> backend defaults
> + // to 'local', which is unintuitive and 'rootdir' might not even be 
> allowed on it
> + allowBlank: (me.vmtype === 'lxc') ? false : true,
> + emptyText: (me.vmtype === 'lxc') ? '' : gettext('use old layout'),

1. We capitalize gettext strings normally, at least the first word
2. "Use old layout" is quite confusing, IMO, maybe "From Backup Configuration"
   or "Backup Layout"

I mean in the longer run we want to probably stream line behavior between CT
and VM so that both try to use the ones from the backup config or even pass
a volume to storage map (as advanced feature).


> + autoSelect: (me.vmtype === 'lxc') ? true : false,

in general I like to avoid ternary operations if easily possible, for boolean
rvalues that's a must, me.vmtype === 'lxc' is already true or false ;-)


>   });
>  
>   var IDfield;
> @@ -135,16 +139,15 @@ Ext.define('PVE.window.Restore', {
>   var submitBtn = Ext.create('Ext.Button', {
>   text: gettext('Restore'),
>   handler: function(){
> - var storage = storagesel.getValue();
>   var values = form.getValues();
>  
>   var params = {
> - storage: storage,
>   vmid: me.vmid || values.vmid,
>   force: me.vmid ? 1 : 0
>   };
>   if (values.unique) { params.unique = 1; }
>   if (values.start) { params.start = 1; }
> + if (values.storage) { params.storage = values.storage; }
>  
>   if (values.bwlimit !== undefined) {
>   params.bwlimit = values.bwlimit;
> 


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager] Improve storage selection on restore

2020-05-05 Thread Fabian Ebner 
Previously, the blank '' would be passed along and lead to a
parameter verfication failure.

For LXC the default behavior in the backend is to use 'local' as
the storage, so disallow blank and auto-select the first storage
supporting 'rootdir' instead.

For QEMU the default behavior in the backend is to use the
original layout from the backup configuration file, which
makes sense to use as the default in the GUI as well.

Signed-off-by: Fabian Ebner 
---
 www/manager6/window/Restore.js | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
index 3f9ad3bc..09c8a997 100644
--- a/www/manager6/window/Restore.js
+++ b/www/manager6/window/Restore.js
@@ -24,7 +24,11 @@ Ext.define('PVE.window.Restore', {
value: '',
fieldLabel: gettext('Storage'),
storageContent: (me.vmtype === 'lxc') ? 'rootdir' : 'images',
-   allowBlank: true
+   // when restoring a container without specifying a storage, the 
backend defaults
+   // to 'local', which is unintuitive and 'rootdir' might not even be 
allowed on it
+   allowBlank: (me.vmtype === 'lxc') ? false : true,
+   emptyText: (me.vmtype === 'lxc') ? '' : gettext('use old layout'),
+   autoSelect: (me.vmtype === 'lxc') ? true : false,
});
 
var IDfield;
@@ -135,16 +139,15 @@ Ext.define('PVE.window.Restore', {
var submitBtn = Ext.create('Ext.Button', {
text: gettext('Restore'),
handler: function(){
-   var storage = storagesel.getValue();
var values = form.getValues();
 
var params = {
-   storage: storage,
vmid: me.vmid || values.vmid,
force: me.vmid ? 1 : 0
};
if (values.unique) { params.unique = 1; }
if (values.start) { params.start = 1; }
+   if (values.storage) { params.storage = values.storage; }
 
if (values.bwlimit !== undefined) {
params.bwlimit = values.bwlimit;
-- 
2.20.1


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel