* Re: [pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s
2020-12-18 16:00 [pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s Aaron Lauterer
@ 2020-12-18 16:38 ` Thomas Lamprecht
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2020-12-18 16:38 UTC (permalink / raw)
To: Proxmox VE development discussion, Aaron Lauterer
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;
>
^ permalink raw reply [flat|nested] 2+ messages in thread