From 4fefab5d2a8717d87b440dd616842e27442d0a12 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 17 May 2024 14:10:54 +0200 Subject: [PATCH 1/3] fix(cli/rm/rm.go): using panic is not user friendly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I manually tested the cli and concluded that it's too easy to reach the panic caused by runtimex.Try0 over there and it would expose panics to users too frequently. Let's just convert those into normal error checks. Here is what we would previously get: ``` % ./ooniprobe rm ? Are you sure you wish to delete the result #0 [Use arrows to move, type to filter] true > false panic: Try0: interrupt [...] ``` So, here I just pressed ^D and got a panic. Ouch! After the diff, we have this: ``` % ./ooniprobe rm ? Are you sure you wish to delete the result #0 false ⨯ failure in main command error=canceled by user ``` and: ``` % ./ooniprobe rm --all ? Are you sure you wish to delete ALL results false ⨯ failure in main command error=canceled by user ``` It seems much better, isn't it? Part of the QA for https://github.com/ooni/probe/issues/2722 --- cmd/ooniprobe/internal/cli/rm/rm.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/ooniprobe/internal/cli/rm/rm.go b/cmd/ooniprobe/internal/cli/rm/rm.go index ab3409a02..39cd7e31c 100644 --- a/cmd/ooniprobe/internal/cli/rm/rm.go +++ b/cmd/ooniprobe/internal/cli/rm/rm.go @@ -9,7 +9,6 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/cli/root" "github.com/ooni/probe-cli/v3/internal/database" - "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/upper/db/v4" ) @@ -21,7 +20,9 @@ func deleteAll(d *database.Database, skipInteractive bool) error { Options: []string{"true", "false"}, Default: "false", } - runtimex.Try0(survey.AskOne(confirm, &answer, nil)) + if err := survey.AskOne(confirm, &answer, nil); err != nil { + return err + } if answer == "false" { return errors.New("canceled by user") } @@ -81,7 +82,9 @@ func init() { Options: []string{"true", "false"}, Default: "false", } - runtimex.Try0(survey.AskOne(confirm, &answer, nil)) + if err := survey.AskOne(confirm, &answer, nil); err != nil { + return err + } if answer == "false" { return errors.New("canceled by user") } From 26ceb42548fa34a1b7d683d13a1f1a41b6f6244c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 17 May 2024 14:42:41 +0200 Subject: [PATCH 2/3] x --- cmd/ooniprobe/internal/cli/rm/rm.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/cmd/ooniprobe/internal/cli/rm/rm.go b/cmd/ooniprobe/internal/cli/rm/rm.go index 39cd7e31c..b4c95a091 100644 --- a/cmd/ooniprobe/internal/cli/rm/rm.go +++ b/cmd/ooniprobe/internal/cli/rm/rm.go @@ -20,9 +20,7 @@ func deleteAll(d *database.Database, skipInteractive bool) error { Options: []string{"true", "false"}, Default: "false", } - if err := survey.AskOne(confirm, &answer, nil); err != nil { - return err - } + _ = survey.AskOne(confirm, &answer, nil) // no error checking: we rely on the default if answer == "false" { return errors.New("canceled by user") } @@ -82,9 +80,7 @@ func init() { Options: []string{"true", "false"}, Default: "false", } - if err := survey.AskOne(confirm, &answer, nil); err != nil { - return err - } + _ = survey.AskOne(confirm, &answer, nil) // no error checking: we rely on the default if answer == "false" { return errors.New("canceled by user") } From 88c3950785d09b535687e59de2d8d9cb753f24a5 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 17 May 2024 14:44:24 +0200 Subject: [PATCH 3/3] Apply suggestions from code review --- cmd/ooniprobe/internal/cli/rm/rm.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/ooniprobe/internal/cli/rm/rm.go b/cmd/ooniprobe/internal/cli/rm/rm.go index b4c95a091..79edbbce3 100644 --- a/cmd/ooniprobe/internal/cli/rm/rm.go +++ b/cmd/ooniprobe/internal/cli/rm/rm.go @@ -20,7 +20,7 @@ func deleteAll(d *database.Database, skipInteractive bool) error { Options: []string{"true", "false"}, Default: "false", } - _ = survey.AskOne(confirm, &answer, nil) // no error checking: we rely on the default + _ = survey.AskOne(confirm, &answer, nil) // no error checking: we rely on the default value if answer == "false" { return errors.New("canceled by user") } @@ -80,7 +80,7 @@ func init() { Options: []string{"true", "false"}, Default: "false", } - _ = survey.AskOne(confirm, &answer, nil) // no error checking: we rely on the default + _ = survey.AskOne(confirm, &answer, nil) // no error checking: we rely on the default value if answer == "false" { return errors.New("canceled by user") }