Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fix] improve liqoctl uninstall error message #2825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Fix] improve liqoctl uninstall error message #2825

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/liqoctl/uninstall/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ func (o *Options) Run(ctx context.Context) error {

s := o.Printer.StartSpinner("Running pre-uninstall checks")
if err := utils.PreUninstall(ctx, o.CRClient); err != nil {
s.Fail("Pre-uninstall checks failed: ", output.PrettyErr(err))
errMsg := fmt.Sprintf("Pre-uninstall checks failed: %s\n"+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we checking here also for offloaded namespaces? if so, I would add the command to unoffload namespaces too

"You can disable the active peerings with other clusters with the 'liqoctl unpeer' command.\n"+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say "You must tear down the active peerings..." instead of "You can disable the active peering...."
Saying "you can", it means (to me) that it's just a suggestion, not a mandatory requirement to proceed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that the errors might not be related to the active peerings, but as @aleoli said, there might be also the possibility that there still is an offloaded namespace or other resources to remove. So I wouldn't always print to "You must tear down the active peerings..." because this is not always the case.
We might add at the end of the error message something like:

  • To disable active peering launch liqoctl X
  • To unoffload a namespace launch liqoctl Y

Actually, the ideal solution would be changing the returned error message to suggest a solution for each of the returned error, but since this is a shared function, I wouldn't suggest to use liqoctl when the user is not actually using it. So we should add a logic to understand when we are using liqoctl and print the suggestion. However, I'm not sure if it is worth it.
What do you guys think? @aleoli @fra98 @cheina97

Maybe in future, it would be nice having a flag like --force that automatically removes everything that prevents Liqo from being uninstalled.

"Check 'liqoctl unpeer --help' for further information.", output.PrettyErr(err))
s.Fail(errMsg)
return err
}
s.Success("Pre-uninstall checks passed")
Expand Down