[pve-devel] [PATCH manager v3] ui: storage: combine RBD external and hyperconverged add dialog

[Alwin Antreich]

On Thu, Jul 05, 2018 at 12:23:13PM +0200, Thomas Lamprecht wrote:
> On 7/5/18 11:29 AM, Alwin Antreich wrote:
> > Patch tested and works. Two small nits below.
> >
> 
> much thanks!
> 
> > On Tue, Jul 03, 2018 at 03:55:10PM +0200, Thomas Lamprecht wrote:
> >> Combine both dialogues. This not only helps to reuse code but also
> >> reduces storage choices from the Storage -> Add menu, and thus
> >> improves usability.
> >>
> >> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> >> ---
> >>
> >> changes v2 -> v3:
> >> * don't show pveceph checkbox when editing
> >> * address the issue that displayfields have no submitValue config getter/setter
> >>   and thus cannot bind on those - as the bind mixin handles binding keys with
> >>   null/undefined value poorly we need to actively diferrentiate between those
> >>   cases, thus add a small closure helper
> >>
> >>  www/manager6/Utils.js           |  11 +--
> >>  www/manager6/dc/StorageView.js  |   4 -
> >>  www/manager6/storage/RBDEdit.js | 148 ++++++++++++++++++++++----------
> >>  3 files changed, 104 insertions(+), 59 deletions(-)
> >>
> >>  [snip]
> >> diff --git a/www/manager6/storage/RBDEdit.js b/www/manager6/storage/RBDEdit.js
> >> index d26a6ac3..e862f036 100644
> >> --- a/www/manager6/storage/RBDEdit.js
> >> +++ b/www/manager6/storage/RBDEdit.js
> >>  [snip]
> >> +	    {
> >> +		xtype: me.isCreate ? 'textfield' : 'displayfield',
> >> +		name: 'username',
> >> +		bind: getBinds(false),
> >> +		value: 'admin',
> >> +		fieldLabel: gettext('User name'),
> >> +		allowBlank: true
> >> +	    }
> >> +	];
> > Could the 'admin' value be an emptyText?
> >
> 
> Maybe, but I want to actively send it to the backend, as we
> do not save an default there, and in-fact admin is not defined
> as default anywhere (schema-wise)...
> 
> How would an user then add an storage without cephx? Username is
> not required there, isn't it?
The username is not required when auth_supported none.

> 
> >>  
> >> -	// here value is an array,
> >> -	// while before it was a string
> >> -	/*jslint confusion: true*/
> >>  	me.column2 = [
> >>  	    {
> >>  		xtype: 'pveContentTypeSelector',
> >> @@ -68,14 +116,20 @@ Ext.define('PVE.storage.RBDInputPanel', {
> >>  		fieldLabel: 'KRBD'
> >>  	    }
> >>  	];
> >> -	/*jslint confusion: false*/
> >> +
> >> +	me.columnB = [{
> >> +	    xtype: 'proxmoxcheckbox',
> >> +	    name: 'pveceph',
> >> +	    bind : {
> >> +		value: '{pveceph}'
> >> +	    },
> >> +	    checked: true,
> >> +	    uncheckedValue: 0,
> >> +	    submitValue: false,
> >> +	    hidden: !me.isCreate,
> >> +	    boxLabel: gettext('Use Proxmox VE Hyperconverged managed ceph pool')
> > s/Hyperconverged/hyper-converged/g
> >
> 
> g flag not needed, single occurrence here 
> I seen it written in both ways, so I actually do not care to much
> which one - albeit lower case is surely better.
Yeah, muscle memory. ;)

The dictionary Pons shows it with the hyphen.

> 
> > Or maybe reverse the checkbox and use as boxLabel:
> > Use foreign ceph cluster || Use external ceph cluster
> >
> 
> I'll like to keep the "PVE managed" part, as you can have setups
> where ceph and PVE are on the same nodes, just not managed by PVE.
> foreign sounds really weird, IMO, and external may not be totally
> correct from a cluster POV.
Maybe some else has an idea. :) 

> 
> >> +	}];
> >>  
> >>  	me.callParent();
> >>      }
> >>  });
> >> -
> >> -Ext.define('PVE.storage.PVERBDInputPanel', {
> >> -    extend: 'PVE.storage.RBDInputPanel',
> >> -
> >> -    pveceph: 1
> >> -});