[pve-devel] [PATCH manager] gui: lxc/Network: add a virtual 'none' option

[Dominik Csapak]

On 2/5/20 10:13 AM, Thomas Lamprecht wrote:
> Am 2/5/20 um 9:37 AM schrieb Dominik Csapak:
>> sometimes, if users do not want a ipv4/6 address, they set the network mode
>> to 'dhcp' instead of 'static' (with no ip set), in believe
>> this will result in no ip
>>
>> in reality, often no ipv6 dhcp server exists in the environment, and
>> the container start stalls for ~5min trying to get a ipv6
>>
>> to improve ux, add the option 'none', which functionally is the same
>> as having selected 'static' with no ip set, and simultaniously
>> make the ip required
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>> this panel is a good candidate for a rewrite using viemodel/controller,
>> but this change is small and the gains of a rewrite not big enough,
>> so i left it as is for now, but if someone wants to get some extjs experience,
>> this could be a good exercise
>>
> 
> The basic idea is OK, but do not make none the new default! CTs normally do
> want a network, our  main use case is system CTs after all, no network no fun

yes this makes sense of course...

> 
>>   www/manager6/lxc/Network.js | 51 ++++++++++++++++++++++++++-----------
>>   1 file changed, 36 insertions(+), 15 deletions(-)
>>
>> diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
>> index be40a8fa..4afcd09b 100644
>> --- a/www/manager6/lxc/Network.js
>> +++ b/www/manager6/lxc/Network.js
>> @@ -36,10 +36,10 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   
>>   	var newdata = {};
>>   
>> -	if (values.ipv6mode !== 'static') {
>> +	if (values.ipv6mode !== 'static' && values.ipv6mode !== 'none') {
>>   	    values.ip6 = values.ipv6mode;
>>   	}
>> -	if (values.ipv4mode !== 'static') {
>> +	if (values.ipv4mode !== 'static' && values.ipv4mode !== 'none') {
>>   	    values.ip = values.ipv4mode;
>>   	}
>>   	newdata[id] = PVE.Parser.printLxcNetwork(values);
>> @@ -161,6 +161,7 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   	    cdata.ip = '';
>>   	    cdata.gw = '';
>>   	}
>> +	var none4 = (!cdata.ip && !cdata.gw && !dhcp4);
>>   
>>   	var auto6 = (cdata.ip6 === 'auto');
>>   	var dhcp6 = (cdata.ip6 === 'dhcp');
>> @@ -168,26 +169,36 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   	    cdata.ip6 = '';
>>   	    cdata.gw6 = '';
>>   	}
>> +
>> +	var none6 = (!cdata.ip6 && !cdata.gw6 && !auto6 && !dhcp6);
> 
> something more telling could be nice, we do not need to exten the ip4/ip6
> namescheme to all related variables, especially if they're not API params
> (and thus fixed). Maybe:
> 
> noIPv6addr
> noIPv4addr
> 

ok

>>   	
>>   	me.column2 = [
>> +	    {
>> +		xtype: 'label',
>> +		text: 'IPv4:', // do not localize
>> +	    },
>>   	    {
>>   		layout: {
>>   		    type: 'hbox',
>>   		    align: 'middle'
>>   		},
>>   		border: false,
>> -		margin: '0 0 5 0',
>> +		margin: '5 0 5 0',
>>   		items: [
>>   		    {
>> -			xtype: 'label',
>> -			text: 'IPv4:' // do not localize
> useless comment

i just copied it, but yes it doesn't add anything...

>> +			xtype: 'radiofield',
>> +			boxLabel: Proxmox.Utils.noneText,
>> +			name: 'ipv4mode',
>> +			inputValue: 'none',
>> +			checked: none4,
>> +			margin: '0 0 0 10',
>>   		    },
>>   		    {
>>   			xtype: 'radiofield',
>>   			boxLabel: gettext('Static'),
>>   			name: 'ipv4mode',
>>   			inputValue: 'static',
>> -			checked: !dhcp4,
>> +			checked: !(dhcp4 || none4),
>>   			margin: '0 0 0 10',
>>   			listeners: {
>>   			    change: function(cb, value) {
>> @@ -211,7 +222,8 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   		name: 'ip',
>>   		vtype: 'IPCIDRAddress',
>>   		value: cdata.ip,
>> -		disabled: dhcp4,
>> +		allowBlank: false,
>> +		disabled: dhcp4 || none4,
>>   		fieldLabel: 'IPv4/CIDR' // do not localize
>>   	    },
>>   	    {
>> @@ -219,14 +231,18 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   		name: 'gw',
>>   		value: cdata.gw,
>>   		vtype: 'IPAddress',
>> -		disabled: dhcp4,
>> +		disabled: dhcp4 || none4,
>>   		fieldLabel: gettext('Gateway') + ' (IPv4)',
>>   		margin: '0 0 3 0' // override bottom margin to account for the menuseparator
>>   	    },
>>   	    {
>>   		xtype: 'menuseparator',
>>   		height: '3',
>> -		margin: '0'
>> +		margin: '0 0 5 0',
>> +	    },
>> +	    {
>> +		xtype: 'label',
>> +		text: 'IPv6:' // do not localize
>>   	    },
>>   	    {
>>   		layout: {
>> @@ -234,18 +250,22 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   		    align: 'middle'
>>   		},
>>   		border: false,
>> -		margin: '0 0 5 0',
>> +		margin: '5 0 5 0',
>>   		items: [
>>   		    {
>> -			xtype: 'label',
>> -			text: 'IPv6:' // do not localize
> nit: useless comment
>> +			xtype: 'radiofield',
>> +			boxLabel: Proxmox.Utils.noneText,
> 
> noneText is not capitalized, stands out in a ugly way.

mhmm.. do we want to add a new gettext for this?
we use that already in some contexts where it could be capitalized...

is there a good way to capitalize this automatically?
(with regards to all languages)

> 
>> +			name: 'ipv6mode',
>> +			inputValue: 'none',
>> +			checked: none6,
>> +			margin: '0 0 0 10',
>>   		    },
>>   		    {
>>   			xtype: 'radiofield',
>>   			boxLabel: gettext('Static'),
>>   			name: 'ipv6mode',
>>   			inputValue: 'static',
>> -			checked: !(auto6 || dhcp6),
>> +			checked: !(auto6 || dhcp6 || none6),
>>   			margin: '0 0 0 10',
>>   			listeners: {
>>   			    change: function(cb, value) {
>> @@ -277,7 +297,8 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   		name: 'ip6',
>>   		value: cdata.ip6,
>>   		vtype: 'IP6CIDRAddress',
>> -		disabled: (dhcp6 || auto6),
>> +		allowBlank: false,
>> +		disabled: (dhcp6 || auto6 || none6),
>>   		fieldLabel: 'IPv6/CIDR' // do not localize
>>   	    },
>>   	    {
>> @@ -285,7 +306,7 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>>   		name: 'gw6',
>>   		vtype: 'IP6Address',
>>   		value: cdata.gw6,
>> -		disabled: (dhcp6 || auto6),
>> +		disabled: (dhcp6 || auto6 || none6),
>>   		fieldLabel: gettext('Gateway') + ' (IPv6)'
>>   	    }
>>   	];
>>
>