Re: [pve-devel] [PATCH manager] ui: rbd: cephfs: add keyring/secret field for external clusters

[Fabian Ebner <f.ebner@proxmox.com>]

Am 24.01.22 um 16:26 schrieb Aaron Lauterer:
> 
> 
> On 1/24/22 13:54, Fabian Ebner wrote:
>> Am 26.11.21 um 17:44 schrieb Aaron Lauterer:
>>> Manual switching of xtype because binding 'hidden' does not work with
>>> pmxDisplayEditField.
>>>
>>
>> Except for two style nits:
>>
>> Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
>> Tested-by: Fabian Ebner <f.ebner@proxmox.com>
>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>> ---
>>>   www/manager6/storage/CephFSEdit.js | 39 ++++++++++++++++++++----------
>>>   www/manager6/storage/RBDEdit.js    | 11 +++++++++
>>>   2 files changed, 37 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/www/manager6/storage/CephFSEdit.js 
>>> b/www/manager6/storage/CephFSEdit.js
>>> index 92fdfe63..89459ba8 100644
>>> --- a/www/manager6/storage/CephFSEdit.js
>>> +++ b/www/manager6/storage/CephFSEdit.js
>>> @@ -101,20 +101,33 @@ Ext.define('PVE.storage.CephFSInputPanel', {
>>>           },
>>>       ];
>>> -    me.columnB = [{
>>> -        xtype: 'proxmoxcheckbox',
>>> -        name: 'pveceph',
>>> -        reference: 'pvecephRef',
>>> -        bind: {
>>> -        disabled: '{!pvecephPossible}',
>>> -        value: '{pveceph}',
>>> +    me.columnB = [
>>> +        {
>>> +        xtype: me.isCreate ? 'textfield' : 'displayfield',
>>> +        name: 'keyring',
>>> +        fieldLabel: 'Secret',
>>
>> Should there be a gettext or is this enough of a technical term in 
>> this context?
> 
> That was what I thought. But some time has passed since, and I am not 
> sure anymore if we should not place it in a gettext.
> 

Ok, so since .keyring and .secret are also the file extensions, it's 
likely better to not use gettext. But if we were to put "Secret" in 
gettext, I'd rather use "Secret Key".

>>
>>> +        value: me.isCreate? '' : '***********',
>>
>> Style nit: no space before '?' and eslint doesn't seem to catch it ;)
> 
> Thanks for catching that :)
> 
>>
>>> +        allowBlank: false,
>>> +        bind: {
>>> +            hidden: '{pveceph}',
>>> +            disabled: '{pveceph}',
>>> +        },
>>> +        },
>>> +        {
>>> +        xtype: 'proxmoxcheckbox',
>>> +        name: 'pveceph',
>>> +        reference: 'pvecephRef',
>>> +        bind: {
>>> +            disabled: '{!pvecephPossible}',
>>> +            value: '{pveceph}',
>>> +        },
>>> +        checked: true,
>>> +        uncheckedValue: 0,
>>> +        submitValue: false,
>>> +        hidden: !me.isCreate,
>>> +        boxLabel: gettext('Use Proxmox VE managed hyper-converged 
>>> cephFS'),
>>>           },
>>> -        checked: true,
>>> -        uncheckedValue: 0,
>>> -        submitValue: false,
>>> -        hidden: !me.isCreate,
>>> -        boxLabel: gettext('Use Proxmox VE managed hyper-converged 
>>> cephFS'),
>>> -    }];
>>> +    ];
>>>       me.callParent();
>>>       },
>>> diff --git a/www/manager6/storage/RBDEdit.js 
>>> b/www/manager6/storage/RBDEdit.js
>>> index 35568b98..3dcfea20 100644
>>> --- a/www/manager6/storage/RBDEdit.js
>>> +++ b/www/manager6/storage/RBDEdit.js
>>> @@ -201,6 +201,17 @@ Ext.define('PVE.storage.RBDInputPanel', {
>>>       ];
>>>       me.columnB = [
>>> +        {
>>> +        xtype: me.isCreate ? 'textarea' : 'displayfield',
>>> +        name: 'keyring',
>>> +        fieldLabel: 'Keyring',
>>
>> Same question here
> 
> Here I rather stick with the keyring as a technical term that is used 
> for Ceph throughout. But then again, there might be a case I did not 
> think of and putting it in a gettext is the sensible thing to do.
> 
>>
>>> +        value: me.isCreate? '' : '***********',
>>
>> and style nit here.
>>
>>> +        allowBlank: false,
>>> +        bind: {
>>> +            hidden: '{pveceph}',
>>> +            disabled: '{pveceph}',
>>> +        },
>>> +        },
>>>           {
>>>           xtype: 'proxmoxcheckbox',
>>>           name: 'pveceph',