[pve-devel] [PATCH v2 manager] fix #2185: add option to change nfs version on gui

[Thomas Lamprecht]

Am 5/13/19 um 9:05 AM schrieb Dominik Csapak:
> On 5/13/19 8:23 AM, Thomas Lamprecht wrote:
>> Am 5/10/19 um 2:52 PM schrieb Oguz Bektas:
>>> this enables us to specify an nfs version while editing/creating an nfs
>>> mount. it used to default to vers=3 without the ability to change it in
>>> gui. now it supports: 3, 4, 4.1 and 4.2
>>>
>>> it should also be possible to add further options in the future (rsize,
>>> wsize, timeo, etc.) on this screen.
>>>
>>> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
>>> ---
>>>
>>> v1->v2:
>>> * add missing space after for loop
>>
>> may make sense to await a first review before resending for such trivial things
>>
>> But looks OK, in general.
>>
>>>
>>>
>>>
>>>   www/manager6/storage/NFSEdit.js | 43 +++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 39 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/www/manager6/storage/NFSEdit.js b/www/manager6/storage/NFSEdit.js
>>> index 9eaa8bc5..26a158a8 100644
>>> --- a/www/manager6/storage/NFSEdit.js
>>> +++ b/www/manager6/storage/NFSEdit.js
>>> @@ -65,15 +65,35 @@ Ext.define('PVE.storage.NFSInputPanel', {
>>>       onGetValues: function(values) {
>>>       var me = this;
>>>   -    if (me.isCreate) {
>>> -        // hack: for now we always create nvf v3
>>> -        // fixme: make this configurable
>>> -        values.options = 'vers=3';
>>> +    var i;
>>> +    for (i = 0; i < me.options.length; i++) {
>>> +        var match;
>>> +        var item = me.options[i];
>>> +        if (match = item.match(/^vers=(.*)$/)) {
>>
>> match is set but never used here
>> (I can remove this when applying, so no need to send v3 just for this)
>>
>>> +        me.options[i] = 'vers=' + values.nfsversion;
>>> +        delete values.nfsversion;
>>> +        }
> 
> here one case is missing, namely if me.options is not set/has no 'vers' entry, we have to add the version in all cases
> 
>>>       }
>>> +    values.options = me.options.join(',');
>>>         return me.callParent([values]);
>>>       },
>>>   +    setValues: function(values) {
>>> +    var me = this;
>>> +    if (values.options) {
>>> +        var res = values.options;
>>> +        me.options = values.options.split(',');
>>> +        me.options.forEach(function(item) {
>>> +        var match;
>>> +        if (match = item.match(/^vers=(.*)$/)) {
>>> +            values.nfsversion = match[1];
>>> +        }
>>> +        });
>>> +    }
>>> +    return me.callParent([values]);
>>> +    },
>>> +
>>>       initComponent : function() {
>>>       var me = this;
>>>   @@ -126,6 +146,21 @@ Ext.define('PVE.storage.NFSInputPanel', {
>>>           }
>>>       ];
>>>   +    me.advancedColumn1 = [
>>> +        {
>>> +        xtype: 'proxmoxKVComboBox',
>>> +        fieldLabel: gettext('NFS Version'),
>>> +        name: 'nfsversion',
>>> +        value: '3',
>>> +        comboItems: [
>>
>> do we want a __default__ in here?
> 
> mhmm, we could add one that amounts to no version option at all
> would it also make nicer if no version is in the options

exactly, that's what I had in mind. 

> 
> ['__default__',Proxmox.Utils.defaultText ],
> 
> ?
> 
> we have to handle that of course while setting/getting
> and maybe we want to keep the gui default of '3' ?

keeping it now, yes, but for 6.0 we could change it also,
not too sure, though. Any bigger disadvantage or advantage
in mind?

> although it would be weird to have a 'default' setting
> that is not the default?

would need to be visible, e.g., "Default (3)" or something like this,
else yes, it could be confusing to people.

> 
>>
>>> +            ['3', '3'],
>>> +            ['4', '4'],
>>> +            ['4.1', '4.1'],
>>> +            ['4.2', '4.2']
>>> +        ],
>>> +        }
>>> +    ];
>>> +
>>>       me.callParent();
>>>       }
>>>   });
>>>