[pve-devel] [PATCH pve-client v3] Add per remote configurable defaults

[René Jochum]

Thanks for looking over it.

On 2018-06-21 12:20, Dietmar Maurer wrote:
> comments inline
>
>> On June 21, 2018 at 9:25 AM René Jochum <r.jochum at proxmox.com> wrote:
>>
>>
>> Signed-off-by: René Jochum <r.jochum at proxmox.com>
>> ---
>> v2: Added defaults per remote.
>> v3: Simplified code by using "pveclient remote set" 
>>
>>  PVE/APIClient/Commands/lxc.pm | 16 ++++++++++++++--
>>  PVE/APIClient/Config.pm       | 29 +++++++++++++++++++++++------
>>  2 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/PVE/APIClient/Commands/lxc.pm b/PVE/APIClient/Commands/lxc.pm
>> index 3add2dd..0601453 100644
>> --- a/PVE/APIClient/Commands/lxc.pm
>> +++ b/PVE/APIClient/Commands/lxc.pm
>> @@ -428,7 +428,12 @@ __PACKAGE__->register_method ({
>>  	    '/nodes/{node}/lxc', 'POST', {
>>  		remote => get_standard_option('pveclient-remote-name'),
>>  		vmid => get_standard_option('pve-vmid'),
>> -		node => get_standard_option('pve-node'),
>> +		node => {
>> +		    description => "The cluster node name.",
>> +		    type => 'string', format => 'pve-node',
>> +		    optional => 1,
>> +		    default => 'localhost | default node'
>> +		}
> You can use something like:
>
>  node => get_standard_option('pve-node', {
> 		    optional => 1,
> 		    default => 'localhost | default node'
>  }),
kk, thanks.
>
>
>
>>  		quiet => {
>>  		    description => "Suppress log output.",
>>  		    type => 'boolean',
>> @@ -453,6 +458,13 @@ __PACKAGE__->register_method ({
>>  	my $background = PVE::APIClient::Tools::extract_param($param, 'background');
>>  
>>  	my $config = PVE::APIClient::Config->load();
>> +	my $remote_config = PVE::APIClient::Config->lookup_remote($config, $remote);
>> +
>> +	if (!$node) {
>> +	    $node = $remote_config->{default_node} // 'localhost';
>> +	}
>> +	$param->{storage} = $param->{storage} // $remote_config->{default_storage}
>> // 'local';
> Is 'local' really a good default (containers are disabled on 'local' per
> default)?
'local' was the only one that I could think of.
>
>> +
>>  	my $conn = PVE::APIClient::Config->remote_conn($config, $remote);
>>  
>>  	my $upid = $conn->post("/nodes/$node/lxc", $param);
>> @@ -496,7 +508,7 @@ __PACKAGE__->register_method ({
>>      }});
>>  
>>  our $cmddef = {
>> -    create => [ __PACKAGE__, 'create', ['remote', 'vmid', 'node']],
>> +    create => [ __PACKAGE__, 'create', ['remote', 'vmid']],
>>      destroy => [ __PACKAGE__, 'destroy', ['remote', 'vmid']],
>>      enter => [ __PACKAGE__, 'enter', ['remote', 'vmid']],
>>  };
>> diff --git a/PVE/APIClient/Config.pm b/PVE/APIClient/Config.pm
>> index a783ab3..73e8f5d 100644
>> --- a/PVE/APIClient/Config.pm
>> +++ b/PVE/APIClient/Config.pm
>> @@ -261,18 +261,36 @@ sub properties {
>>  	    optional => 1,
>>  	    maxLength => 4096,
>>  	},
>> +
>> +	default_node => {
>> +	    description => "The default Node to create guests on.",
>> +	    type => 'string',
>> +	    optional => 1,
>> +	    maxLength => 4096,
>> +	    default => 'localhost',
>> +	},
> Why don't you use the standard option definition here?
Overlooked that I can use it, will do.
>
>> +	default_storage => {
>> +	    description => "The default storage to create guests on.",
>> +	    type => 'string',
>> +	    optional => 1,
>> +	    maxLength => 4096,
>> +	    default => 'local',
>> +	},
> Why don't you use the standard option 'pve-storage-id' here?
Same as above.
>
>>      };
>>  }
>>  
>>  sub options {
>>      return {
>> -	name => { optional => 0 },
>> -	host => { optional => 0 },
>> +	name => { optional => 1 },
>> +	host => { optional => 0, fixed => 1 },
> unrelated change? - should go into a separate patch.
>
>>  	comment => { optional => 1 },
>> -	username => { optional => 0 },
>> +	username => { optional => 0, fixed => 1 },
> same here
>
>>  	password => { optional => 1 },
>> -	port => { optional => 1 },
>> -	fingerprint => { optional => 1 },
>> +	port => { optional => 1, fixed => 1 },
>> +	fingerprint => { optional => 0, fixed => 1 },
> why 'fixed'?
That makes the option none editable.
>
>> +
>> +	default_node => { optional => 1 },
>> +	default_storage => { optional => 1 },
>>     };
>>  }
>>  
>> @@ -295,7 +313,6 @@ sub type {
>>  
>>  sub options {
>>      return {
>> -	name => { optional => 1 },
>>  	username => { optional => 1 },
>>  	port => { optional => 1 },
>>     };
>> -- 
>> 2.11.0