Re: [pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 18/12/2020 17:00, Aaron Lauterer wrote:
> Printing a lot of very detailed JSON output on the CLI is not very
> useful.
> 
> Printing the `ceph -s` overview is much more suited to give an overview
> of the ceph cluster status.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> @Thomas: I hope this is what you hand in mind when we did discuss this
> off list.
> 

yeah, this looks fine in general, a code style nit and a comment/questions about
the use of eval below.


>  PVE/CLI/pveceph.pm | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
> index 3d7bf2b1..e12a9338 100755
> --- a/PVE/CLI/pveceph.pm
> +++ b/PVE/CLI/pveceph.pm
> @@ -175,6 +175,20 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'status',
> +    path => 'status',
> +    method => 'GET',
> +    description => "Get Ceph Status.",
> +    parameters => {
> +	additionalProperties => 0,
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	eval { run_command(['ceph', '-s'], outfunc => sub { print "$_[0]\n" }, errfunc => sub { print STDERR "$_[0]\n" }) };

is the eval really required? silencing execution errors may not be ideal,
I'd either drop it completely or if deemed as required do a `warn $@ if $@;`

And this is a bit long for a single line, we adapted rules from perl to be
closer to the ones from rust, see 80 as soft-limit (i.e., not good if you reach that
(branch) indentation, but totally fine for messages or improved readability) and 100
as hard one.
https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings

So rather do:

run_command(
    [ceph', '-s'],
    outfunc => sub { print "$_[0]\n" },
    errfunc => sub { print STDERR "$_[0]\n" },
);

(with and without eval)

> +	return undef;
> +    }});
> +
>  our $cmddef = {
>      init => [ 'PVE::API2::Ceph', 'init', [], { node => $nodename } ],
>      pool => {
> @@ -229,11 +243,7 @@ our $cmddef = {
>      stop => [ 'PVE::API2::Ceph', 'stop', [], { node => $nodename }, $upid_exit],
>      install => [ __PACKAGE__, 'install', [] ],
>      purge => [  __PACKAGE__, 'purge', [] ],
> -    status => [ 'PVE::API2::Ceph', 'status', [], { node => $nodename }, sub {
> -	my $res = shift;
> -	my $json = JSON->new->allow_nonref;
> -	print $json->pretty->encode($res) . "\n";
> -    }],
> +    status => [ __PACKAGE__, 'status', []],
>  };
>  
>  1;
>