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

[Thomas Lamprecht]

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?

>>  
>> -	// 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.

> 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.

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