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

[pve-devel] [PATCH manager v2] 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>
---

v2 of this little patch incorporates the given feedback[0] (thanks btw!)

Regarding exit codes: Even though it's not too important, I think we
should stick with a non-zero exit code here; at least from what I've
encountered, installation wizards or similar will only exit with 0
if the installation succeeded *or* if the thing to be installed is
already installed. That way subsequent commands are only executed if
the things to be installed are actually there, as further commands
*might* already depend on them, e.g.:

  apt-get install -y neofetch && neofetch

  pveceph install --version quincy && ceph status

This is mostly philosophical, but I've personally found that behaviour
useful in some situations.


[0]: https://lists.proxmox.com/pipermail/pve-devel/2023-July/057993.html


 PVE/CLI/pveceph.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
index b47f8cc1..8cff04c5 100755
--- a/PVE/CLI/pveceph.pm
+++ b/PVE/CLI/pveceph.pm
@@ -176,6 +176,16 @@ __PACKAGE__->register_method ({
 	} else {
 	    die "unsupported ceph version: $cephver";
 	}
+
+	if (-t STDOUT && !$param->{version}) {
+	    print "This will install Ceph " . ucfirst($cephver) . " - continue (y/N)? ";
+
+	    my $answer = <STDIN>;
+	    my $continue = defined($answer) && $answer =~ m/^\s*y(?:es)?\s*$/i;
+
+	    die "Aborting installation as requested\n" if !$continue;
+	}
+
 	PVE::Tools::file_set_contents("/etc/apt/sources.list.d/ceph.list", $repolist);
 
 	my $supported_re = join('|', $supported_ceph_versions->@*);
-- 
2.39.2

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

[Thomas Lamprecht]

Am 05/07/2023 um 20:02 schrieb 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>
> ---
> 
> v2 of this little patch incorporates the given feedback[0] (thanks btw!)
> 
> Regarding exit codes: Even though it's not too important, I think we
> should stick with a non-zero exit code here; at least from what I've
> encountered, installation wizards or similar will only exit with 0
> if the installation succeeded *or* if the thing to be installed is
> already installed. That way subsequent commands are only executed if
> the things to be installed are actually there, as further commands
> *might* already depend on them, e.g.:
> 
>   apt-get install -y neofetch && neofetch
> 
>   pveceph install --version quincy && ceph status
> 

besides that those example would not really change if the first command
exits with 0 even if not installed, as the second would simply fail leaving
the result of the boolean expression the same, the question wasn't about
non-interactive batch-mode, there it definitively is correct to exit with
non-zereo, what I did is questioning the *interactive* abort by user choice.

While one might reach the same conclusion also for that too, above
argumentation is IMO not enough to justify that (on its own).

But FWIW, "apt purge" or "apt full-upgrade" also exit with non-zero if one
answers the confirmation with no (albeit, just for completeness sake, there
the default answer is Y).

> This is mostly philosophical, but I've personally found that behaviour
> useful in some situations.
> > 
> [0]: https://lists.proxmox.com/pipermail/pve-devel/2023-July/057993.html
> 
> 
>  PVE/CLI/pveceph.pm | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
>

applied, thanks!

Bonus points if you add checking the broadcasted ceph-versions using
PVE::Ceph::Services::get_ceph_versions() and if there are any and (all of?) them
match the default one, just continue.

And respective then for the "version got passed explicitly but mismatches with
what seems to be installed on other nodes" case you could also ask for confirmation
if on a TTY (no TTY one is likely to stem from the web UI, where the user got that
info already anyway, and installing newer version while an older one is still in
use can be fine too, so wouldn't ever hard-block that case).