From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Aaron Lauterer <a.lauterer@proxmox.com>
Subject: Re: [pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s
Date: Fri, 18 Dec 2020 17:38:46 +0100 [thread overview]
Message-ID: <2bbe1229-fc2c-bb8e-1883-256d34b53579@proxmox.com> (raw)
In-Reply-To: <20201218160016.32301-1-a.lauterer@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;
>
prev parent reply other threads:[~2020-12-18 16:38 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-18 16:00 Aaron Lauterer
2020-12-18 16:38 ` Thomas Lamprecht [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2bbe1229-fc2c-bb8e-1883-256d34b53579@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox