[pve-devel] [PATCH pve-manager] First beta of FreeNAS storage plugin

[Dominik Csapak]

On 06/13/2017 09:01 AM, Michael Rasmussen wrote:
> On Mon, 12 Jun 2017 14:18:59 +0200
> Dominik Csapak <d.csapak at proxmox.com> wrote:
>
>> i just gave this patch a quick glance, here a few remarks:
>>
>> a bit nitpicky, but your indentation needs fixing, the FreeNASEdit.js seems mostly ok, but nearly every other file you touched has wrong indentation
>>
> The reason is that all the *.js files uses a mix of either tabs for
> indentation or spaces, in some cases used intertwined so until somebody
> normalizes all *.js files it seems the only safe way is to stick
> exclusively with spaces as indentation and instruct your editor to
> leave tabs untouched for not changes text.
>

not really, we try to also use the same indentation as with perl
see https://pve.proxmox.com/wiki/Perl_Style_Guide#Indentation
(one indentation level is 4 spaces, every 8 spaces are converted using tabs)

there are some files which do not use this style, but we try to fix
this as we editing those files (also patches are welcome :P )

>>> +		xtype: 'textfield',
>>> +		name: 'password',
>>> +		emptyText: '',
>>> +        inputType: 'password',
>>> +		fieldLabel: gettext('Password'),
>>> +		allowBlank: false
>>> +	    },
>>
>> not really a gui problem, but it is probably a bad idea to save/load passwords from/to a textfield, at least do not load and insert it in the field, but leave it empty
>>
> How would you suggest fixing this? If you don't enter something then
> the password will be change if the user presses save (I am not a
> javascript guru;-)
>

in the onGetValues function you could do something like this:

if (values.password === '') {
	delete values.password;
}

this only sends the password to the api when it is non empty

but a better way for the whole plugin would probably be a credentials
file with limited read access (so only root can read it)

>>> +Ext.define('PVE.storage.FreeNASEdit', {
>>> +    extend: 'PVE.window.Edit',
>>> +
>>> +    initComponent : function() {
>>> +	var me = this;
>>> +
>>> +	me.isCreate = !me.storageId;
>>> +
>>> +	if (me.isCreate) {
>>> +            me.url = '/api2/extjs/storage';
>>> +            me.method = 'POST';
>>> +        } else {
>>> +            me.url = '/api2/extjs/storage/' + me.storageId;
>>> +            me.method = 'PUT';
>>> +        }
>>> +
>>> +	var ipanel = Ext.create('PVE.storage.FreeNASInputPanel', {
>>> +	    isCreate: me.isCreate,
>>> +	    storageId: me.storageId
>>> +	});
>>> +
>>> +	Ext.apply(me, {
>>> +            subject: 'FreeNAS Storage',
>>
>> you add the storage type to format_storage_type, but here you do not use it, is there a reason?
>>
> I don't get this. Please elaborate?
>

see for example the code in www/manager6/storage/ZFSPoolEdit.js

-----8<-----
Ext.apply(me, {
	subject: PVE.Utils.format_storage_type('zfspool'),
	isAdd: true,
----->8-----

This is also something in our code that could use some patches ;)