[pve-devel] [PATCH pve-manager] First beta of FreeNAS storage plugin
Dominik Csapak
d.csapak at proxmox.com
Mon Jun 12 14:18:59 CEST 2017
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
other comments inline
On 06/12/2017 03:19 AM, Michael Rasmussen wrote:
> Signed-off-by: Michael Rasmussen <mir at datanom.net>
> ---
> www/manager6/Makefile | 1 +
> www/manager6/Utils.js | 4 +-
> www/manager6/dc/StorageView.js | 15 +++-
> www/manager6/lxc/ResourceEdit.js | 6 +-
> www/manager6/qemu/Clone.js | 3 +-
> www/manager6/qemu/HDEdit.js | 3 +-
> www/manager6/qemu/HDEfi.js | 2 +-
> www/manager6/qemu/HDMove.js | 3 +-
> www/manager6/storage/FreeNASEdit.js | 152 ++++++++++++++++++++++++++++++++++++
> 9 files changed, 180 insertions(+), 9 deletions(-)
> create mode 100644 www/manager6/storage/FreeNASEdit.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 0c266ef..2a4489d 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -186,6 +186,7 @@ JSSRC= \
> storage/SheepdogEdit.js \
> storage/ZFSEdit.js \
> storage/ZFSPoolEdit.js \
> + storage/FreeNASEdit.js \
> ha/StatusView.js \
> ha/Status.js \
> ha/GroupSelector.js \
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index d1ada77..c6227d0 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -799,7 +799,9 @@ Ext.define('PVE.Utils', { utilities: {
> return 'iSCSIDirect';
> } else if (value === 'drbd') {
> return 'DRBD';
> - } else {
> + } else if (value === 'freenas') {
> + return 'FreeNAS';
> + } else {
> return PVE.Utils.unknownText;
> }
> },
> diff --git a/www/manager6/dc/StorageView.js b/www/manager6/dc/StorageView.js
> index 2940a98..c16d464 100644
> --- a/www/manager6/dc/StorageView.js
> +++ b/www/manager6/dc/StorageView.js
> @@ -58,7 +58,9 @@ Ext.define('PVE.dc.StorageView', {
> editor = 'PVE.storage.ZFSEdit';
> } else if (type === 'zfspool') {
> editor = 'PVE.storage.ZFSPoolEdit';
> - } else {
> + } else if (type === 'freenas') {
> + editor = 'PVE.storage.FreeNASEdit';
> + } else {
> return;
> }
> var win = Ext.create(editor, {
> @@ -191,7 +193,16 @@ Ext.define('PVE.dc.StorageView', {
> win.on('destroy', reload);
> win.show();
> }
> - }
> + },
> + {
> + text: PVE.Utils.format_storage_type('freenas'),
> + iconCls: 'fa fa-fw fa-building',
> + handler: function() {
> + var win = Ext.create('PVE.storage.FreeNASEdit', {});
> + win.on('destroy', reload);
> + win.show();
> + }
> + }
>
> /* the following type are conidered unstable
> * so we do not enable that on the GUI for now
> diff --git a/www/manager6/lxc/ResourceEdit.js b/www/manager6/lxc/ResourceEdit.js
> index 61a5539..8bc3e76 100644
> --- a/www/manager6/lxc/ResourceEdit.js
> +++ b/www/manager6/lxc/ResourceEdit.js
> @@ -387,7 +387,8 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
> }
> var rec = f.store.getById(value);
> if (rec.data.type === 'zfs' ||
> - rec.data.type === 'zfspool') {
> + rec.data.type === 'zfspool' ||
> + rec.data.type === 'freenas') {
> me.quota.setDisabled(true);
> me.quota.setValue(false);
> } else {
> @@ -407,7 +408,8 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
> rec.data.type === 'rbd' ||
> rec.data.type === 'sheepdog' ||
> rec.data.type === 'zfs' ||
> - rec.data.type === 'zfspool') {
> + rec.data.type === 'zfspool' ||
> + rec.data.type === 'freenas') {
> me.hdfilesel.setDisabled(true);
> me.hdfilesel.setVisible(false);
> me.hdsizesel.setDisabled(false);
> diff --git a/www/manager6/qemu/Clone.js b/www/manager6/qemu/Clone.js
> index c5dad4d..9de9777 100644
> --- a/www/manager6/qemu/Clone.js
> +++ b/www/manager6/qemu/Clone.js
> @@ -116,7 +116,8 @@ Ext.define('PVE.window.Clone', {
> rec.data.type === 'iscsi' ||
> rec.data.type === 'sheepdog' ||
> rec.data.type === 'zfs' ||
> - rec.data.type === 'zfspool'
> + rec.data.type === 'zfspool' ||
> + rec.data.type === 'freenas'
> ) {
> formatsel.setValue('raw');
> formatsel.setDisabled(true);
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index 0060394..6920c0d 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -46,7 +46,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
> rec.data.type === 'rbd' ||
> rec.data.type === 'sheepdog' ||
> rec.data.type === 'zfs' ||
> - rec.data.type === 'zfspool') {
> + rec.data.type === 'zfspool' ||
> + rec.data.type === 'freenas') {
> me.hdfilesel.setDisabled(true);
> me.hdfilesel.setVisible(false);
> me.formatsel.setValue('raw');
> diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
> index 45c9d70..7aa9608 100644
> --- a/www/manager6/qemu/HDEfi.js
> +++ b/www/manager6/qemu/HDEfi.js
> @@ -21,7 +21,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
> var me = this.getView();
> var rec = f.store.getById(value);
>
> - var rawArray = ['iscsi', 'lvm', 'lvmthin', 'drbd', 'rbd', 'sheepdog', 'zfs', 'zfspool'];
> + var rawArray = ['iscsi', 'lvm', 'lvmthin', 'drbd', 'rbd', 'sheepdog', 'zfs', 'zfspool', 'freenas'];
>
> me.hdfilesel.setDisabled(true);
> me.hdfilesel.setVisible(false);
> diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
> index 745c791..4e69aa7 100644
> --- a/www/manager6/qemu/HDMove.js
> +++ b/www/manager6/qemu/HDMove.js
> @@ -80,7 +80,8 @@ Ext.define('PVE.window.HDMove', {
> rec.data.type === 'rbd' ||
> rec.data.type === 'sheepdog' ||
> rec.data.type === 'zfs' ||
> - rec.data.type === 'zfspool'
> + rec.data.type === 'zfspool' ||
> + rec.data.type === 'freenas'
> ) {
> me.formatsel.setValue('raw');
> me.formatsel.setDisabled(true);
> diff --git a/www/manager6/storage/FreeNASEdit.js b/www/manager6/storage/FreeNASEdit.js
> new file mode 100644
> index 0000000..5cc9f28
> --- /dev/null
> +++ b/www/manager6/storage/FreeNASEdit.js
> @@ -0,0 +1,152 @@
> +Ext.define('PVE.storage.FreeNASInputPanel', {
> + extend: 'PVE.panel.InputPanel',
> +
> + onGetValues: function(values) {
> + var me = this;
> +
> + if (me.isCreate) {
> + values.type = 'freenas';
> + values.content = 'images';
seems 'rootdir' is missing, this way you can never create containers there?
> + } else {
> + delete values.storage;
> + }
> +
> + values.disable = values.enable ? 0 : 1;
> + delete values.enable;
> +
> + return values;
> + },
> +
> + initComponent : function() {
> + var me = this;
> +
> + me.column1 = [
> + {
> + xtype: me.isCreate ? 'textfield' : 'displayfield',
> + name: 'storage',
> + value: me.storageId || '',
> + fieldLabel: 'ID',
> + vtype: 'StorageId',
> + allowBlank: false
> + },
> + {
> + xtype: me.isCreate ? 'textfield' : 'displayfield',
> + name: 'portal',
> + value: '',
> + fieldLabel: gettext('Portal'),
> + allowBlank: false
> + },
> + {
> + xtype: me.isCreate ? 'textfield' : 'displayfield',
> + name: 'pool',
> + value: '',
> + fieldLabel: gettext('Pool'),
> + allowBlank: false
> + },
> + {
> + xtype: me.isCreate ? 'textfield' : 'displayfield',
> + name: 'portal_group',
> + value: '',
> + fieldLabel: gettext('Portal Group ID'),
> + allowBlank: false
> + },
> + {
> + xtype: me.isCreate ? 'textfield' : 'displayfield',
> + name: 'initiator_group',
> + value: '',
> + fieldLabel: gettext('Initiator Group ID'),
> + allowBlank: false
> + },
this trailing comma is an error in jslint
> + ];
> +
> + me.column2 = [
> + {
> + xtype: 'pvecheckbox',
> + name: 'enable',
> + checked: true,
> + uncheckedValue: 0,
> + fieldLabel: gettext('Enable')
> + },
> +/* {
> + xtype: 'textfield',
> + name: 'blocksize',
> + emptyText: '8k',
> + fieldLabel: gettext('Block Size'),
> + allowBlank: true
> + },*/
is this part commented out for a reason? if yes please add it,
otherwise, please remove the code
> + {
> + xtype: 'textfield',
> + name: 'username',
> + emptyText: 'root',
> + fieldLabel: gettext('Username'),
> + allowBlank: false
> + },
> + {
> + 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
also, again a trailing comma
> + ];
> +
> + if (me.isCreate || me.storageId !== 'local') {
> + me.column2.unshift({
> + xtype: 'pveNodeSelector',
> + name: 'nodes',
> + fieldLabel: gettext('Nodes'),
> + emptyText: gettext('All') + ' (' +
> + gettext('No restrictions') +')',
> + multiSelect: true,
> + autoSelect: false
> + });
> + }
> +
> + me.callParent();
> + }
> +});
> +
> +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?
> + isAdd: true,
> + items: [ ipanel ]
> + });
> +
> + me.callParent();
> +
> + if (!me.isCreate) {
> + me.load({
> + success: function(response, options) {
> + var values = response.result.data;
> + if (values.nodes) {
> + values.nodes = values.nodes.split(',');
> + }
> + values.enable = values.disable ? 0 : 1;
> + ipanel.setValues(values);
> + }
> + });
> + }
> + }
> +});
>
More information about the pve-devel
mailing list