sorry for the delay, a few comments inline.
Am 22/02/2023 um 15:23 schrieb Max Carrara:
> diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
> index afcc67e0..eaf7918e 100755
> --- a/PVE/CLI/pveceph.pm
> +++ b/PVE/CLI/pveceph.pm
> @@ -157,6 +157,19 @@ __PACKAGE__->register_method ({
> } else {
> die "unsupported ceph version: $cephver";
> }
> +
> + if (-t STDOUT && !$param->{version}) {
> + my $title_case_cephver = $cephver =~ s/(.)(.*?)/\U$1\L$2/r ;
You can simply use ucfirst($cephver)
https://perldoc.perl.org/functions/ucfirst
> +
> + print "This will install Ceph $title_case_cephver.\n" .
> + "Are you sure you want to proceed (yes/no)? ";
you can go for 100cc and for line continuations please place the continuation
dot,
and normally also the whitespace (but no hard feelings on the latter), on the
next line.
Here
print "This will install Ceph ". ucfirst($cephver) ." - continue (y/N)? "
(using uppercase N to signal the default)
> +
> + my $answer = <STDIN>;
this might not be defined, e.g., if the user hits CTRL + D, closing the stdin
stream.
> + my $valid = ($answer =~ m/^\s*yes\s*$/i) ? 1 : 0;
No need to transform a truthy expression to another truthy one, if you want to
enforce
that the value gets reduced to perl's canonical boolean true/false values you
can use
double negation !!, but not necessary here as we need to check for defined-ness
anyway.
Also, nit: $valid is a bit odd/confusing here, as either is a valid answer,
would name
for what it actually checks, which can be as simple as $continue.
I'd also accept a simple y here:
my $continue = defined($answer) && $answer =~ m/^\s*y(?:es)?\s*$/i;
> +
> + die "Aborting installation.\n" if !$valid;
This error reads a bit like we (as in pveceph) are aborting the installation,
but actually
the user was, so I'd go for something like:
"User aborted installation\n"
"Aborting installation as requested\n"
A small, not to relevant and almost philosophical question is also if we want
to return a
negative exit code in this case or if it is actually also an exit 0 – not sure
from top of
my head how other CLI setup wizards are handling such things.
> + }
> +
> PVE::Tools::file_set_contents("/etc/apt/sources.list.d/ceph.list",
> $repolist);
>
> my $supported_re = join('|', $supported_ceph_versions->@*);
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
