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

[Dietmar Maurer]

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'
 }),



>  		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)?

> +
>  	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?

> +	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?

>      };
>  }
>  
>  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'?

> +
> +	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