[pve-devel] [PATCH pve-manager] api, ui: allow to remove subscription

[Thomas Lamprecht]

On 5/26/20 3:42 PM, Moayad Almalat wrote:
> Signed-off-by: Moayad Almalat <m.almalat at proxmox.com>
> ---

please add a changelog here, e.g.

changes from v2 -> v3:
* set returns property for API schema
* ...

FYI: you can add a "v2" easily by using the -v2 option in `git format-patch`
or `git send-email`.

>  PVE/API2/Subscription.pm          | 24 ++++++++++++++++++++++++
>  PVE/CLI/pvesubscription.pm        |  1 +
>  www/manager6/node/Subscription.js |  9 +++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/PVE/API2/Subscription.pm b/PVE/API2/Subscription.pm
> index 6657c00d..b0597cc2 100644
> --- a/PVE/API2/Subscription.pm
> +++ b/PVE/API2/Subscription.pm
> @@ -245,4 +245,28 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'delete',
> +    path => '',
> +    method => 'DELETE',
> +    permissions => {
> +	check => ['perm', '/nodes/{node}', [ 'Sys.Modify' ]],
> +    },
> +    description => "Delete subscription key.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +    	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	},
> +    },
> +    returns => { type => 'null'},
> +    code => sub {
> +	my $subscription_file = '/etc/subscription';
> +	die "Subscription file doesn't exist - $!\n" if ! -e $subscription_file;
> +	unlink $subscription_file if -e $subscription_file;

Twice making the same check makes not much sense, or?

I made two simple suggestions and you go a complete other direction, why?
So please, no die on file doesn't exists and check the return value of
unlink for failure. In other words, these two lines:

return if ! -e $subscription_file; # nothing to delete
unlink($subscription_file) or die "cannot delete subscription key: $!";

The rest of the patch looks now much better, thanks for that!


> +	return undef;
> +    }});
> +
>  1;