[pve-devel] [PATCH manager] gui: expose lxc features

[Thomas Lamprecht]

On 10/22/18 4:29 PM, Dominik Csapak wrote:
> but constrain editing to root at pam
> 

Code itself works and looks ok, some higher level remarks/nits inline.

.git/rebase-apply/patch:93: new blank line at EOF.

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> i am not sure if the gettexts are ok, or if we want something like this
> Ext.String.format(gettext('Allow {}'), gettext('filesystem types')), etc.

Initially, with reading only your code changes, not your commit message,
I had the same idea - which seems good for it.

Alternatively, just omit it completely, those are checkboxes which makes
it clear that it disables/enables something, and we actively removed
enable/allow sounding words from some checkboxes a bit ago (Emmanuel and
maybe even myself did, AFAIK)

> 
> also, would it make sense to have an example in the fstypes fields emptytext?
> e.g. 'nfs, smb' ?

If we do it this way, yes, I'd do so. Else a combobox with the most common used
may be more PVE GUI worthy? I.e., simpler and value/input checking included (at
least for the ones which are not JS-webconsole hacker ;-)

> 
>  www/manager6/Makefile            |  1 +
>  www/manager6/lxc/FeaturesEdit.js | 62 ++++++++++++++++++++++++++++++++++++++++
>  www/manager6/lxc/Options.js      |  6 ++++
>  3 files changed, 69 insertions(+)
>  create mode 100644 www/manager6/lxc/FeaturesEdit.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 68b5227b..cc2f7203 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -146,6 +146,7 @@ JSSRC= 				                 	\
>  	lxc/Summary.js					\
>  	lxc/Network.js					\
>  	lxc/Resources.js				\
> +	lxc/FeaturesEdit.js				\
>  	lxc/Options.js					\
>  	lxc/DNS.js					\
>  	lxc/Config.js					\
> diff --git a/www/manager6/lxc/FeaturesEdit.js b/www/manager6/lxc/FeaturesEdit.js
> new file mode 100644
> index 00000000..4fb1595a
> --- /dev/null
> +++ b/www/manager6/lxc/FeaturesEdit.js
> @@ -0,0 +1,62 @@
> +Ext.define('PVE.lxc.FeaturesInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pveLxcFeaturesInputPanel',
> +
> +    initComponent: function() {
> +	var me = this;
> +	me.items= [
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Allow keyctl'),
> +		name: 'keyctl'
> +	    },

may want to disable this on privileged CTs?

> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Allow nesting'),
> +		name: 'nesting'
> +	    },
> +	    {
> +		xtype: 'proxmoxtextfield',
> +		fieldLabel: gettext('Allow filesystem types'),
> +		name: 'mount'
> +	    }
> +	];
> +	me.callParent();
> +    },
> +
> +    onGetValues: function(values) {
> +	var featuresstring = PVE.Parser.printPropertyString(values, undefined);

no camelCase? Where's the JS spirit? ;-)
Do we have a style guide for JS? We build upon the Perl one but not everything
is the same here.

Just out of interest, not really having a problem with the var name..

> +	if (featuresstring == '') {
> +	    return { 'delete': 'features' };
> +	}
> +	return { features: featuresstring };
> +    },
> +
> +    setValues: function(values) {
> +	var features = values.features;
> +	if (features) {
> +	    var res = PVE.Parser.parsePropertyString(features, undefined);
> +	    this.callParent([res]);
> +	}
> +    }
> +});
> +
> +Ext.define('PVE.lxc.FeaturesEdit', {
> +    extend: 'Proxmox.window.Edit',
> +    xtype: 'pveLxcFeaturesEdit',
> +
> +    subject: gettext('Features'),

Hmm, they are features and the backend property names them also so,
but it feels a bit to generic, IMO. "Privileged Features" doesn't really
works either. But, as we always talk of "allow XY" something alike
privileges may not be to bad.

> +
> +    items: [{
> +	xtype: 'pveLxcFeaturesInputPanel'
> +    }],
> +
> +    initComponent : function() {
> +	var me = this;
> +
> +	me.callParent();
> +
> +	me.load();
> +    }
> +});
> +
> diff --git a/www/manager6/lxc/Options.js b/www/manager6/lxc/Options.js
> index 3a9959fe..e4a8d86b 100644
> --- a/www/manager6/lxc/Options.js
> +++ b/www/manager6/lxc/Options.js
> @@ -134,6 +134,12 @@ Ext.define('PVE.lxc.Options', {
>  		header: gettext('Unprivileged container'),
>  		renderer: Proxmox.Utils.format_boolean,
>  		defaultValue: 0
> +	    },
> +	    features: {
> +		header: gettext('Features'),
> +		defaultValue: Proxmox.Utils.noneText,
> +		editor: Proxmox.UserName === 'root at pam' ?
> +		    'PVE.lxc.FeaturesEdit' : undefined
>  	    }
>  	};
>  
>