comments inline
> On June 21, 2018 at 9:25 AM René Jochum <[email protected]> wrote:
>
>
> Signed-off-by: René Jochum <[email protected]>
> ---
> 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
_______________________________________________
pve-devel mailing list
[email protected]
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
