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

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

[Max Carrara]

Displays a confirmation dialogue if the user didn't explicitly
provide a valid ceph version via the `--version` flag and if
stdout is connected to a tty.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 PVE/CLI/pveceph.pm | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Note: This was tested in a VM, with and without --version flag.

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 ;
+
+	    print "This will install Ceph $title_case_cephver.\n" .
+		"Are you sure you want to proceed (yes/no)? ";
+
+	    my $answer = <STDIN>;
+	    my $valid = ($answer =~ m/^\s*yes\s*$/i) ? 1 : 0;
+
+	    die "Aborting installation.\n" if !$valid;
+	}
+
 	PVE::Tools::file_set_contents("/etc/apt/sources.list.d/ceph.list", $repolist);
 
 	my $supported_re = join('|', $supported_ceph_versions->@*);
-- 
2.30.2

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

[Max Carrara]

On 2/22/23 15:23, Max Carrara wrote:
> Displays a confirmation dialogue if the user didn't explicitly
> provide a valid ceph version via the `--version` flag and if
> stdout is connected to a tty.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  PVE/CLI/pveceph.pm | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> Note: This was tested in a VM, with and without --version flag.
> 
> 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 ;
> +
> +	    print "This will install Ceph $title_case_cephver.\n" .
> +		"Are you sure you want to proceed (yes/no)? ";
> +
> +	    my $answer = <STDIN>;
> +	    my $valid = ($answer =~ m/^\s*yes\s*$/i) ? 1 : 0;
> +
> +	    die "Aborting installation.\n" if !$valid;
> +	}
> +
>  	PVE::Tools::file_set_contents("/etc/apt/sources.list.d/ceph.list", $repolist);
>  
>  	my $supported_re = join('|', $supported_ceph_versions->@*);

Ping - just applied this on master[0] and tested it.

Though, I'm not sure if this still necessary, as we only support
Quincy atm; but might become relevant once Reef is released, I suppose.

[0] as of commit 50bcf799d8435b794fe2a79a74aa6df6a1419292

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