[pve-devel] [PATCH manager] fix #4364: pveceph: add confirmation dialogue for ceph installation

[Thomas Lamprecht]

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->@*);