-
Notifications
You must be signed in to change notification settings - Fork 255
[RFC] Remove custom multierrors package #426
Conversation
cli/cmd/context/rm.go
Outdated
@@ -58,17 +58,20 @@ func runRemove(ctx context.Context, args []string, force bool) error { | |||
if force { | |||
err := runUse(ctx, "default") | |||
if err != nil { | |||
errs = multierror.Append(errs, errors.New("cannot delete current context")) | |||
errs = multierror.Append(errs, multierror.Prefix(errors.New("cannot delete current context"), "Error:")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we produce an error in the --force
case, or only warn (and skip)? At least --force
should ignore none-existing contexts (and have a "zero" exit code)
There may be some incorrect behaviour in the old CLI in this respect (noticed some in docker volume rm
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I recall there was some discussion about this, and it should only be a "success" in the "not exists" situation; see docker/cli#394
docker volume create namedvol
docker volume rm namedvol nosuchvolume
# namedvol
# Error: No such volume: nosuchvolume
echo $?
# 1
docker volume create namedvol
docker volume rm --force namedvol nosuchvolume
# namedvol
# nosuchvolume
echo $?
# 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, the --force
shouldn't return 0 no matter what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (looking at the other discussion) it should still error if there's a failure to remove, but should ignore cases where the context doesn't exist, so perhaps removeContext
needs a force
argument so that it can ignore the "does not exist" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, any ideas to how we can avoid the boolean trap?
The custom mutlierror was there for a nicer output than the one the original package has, I wrote on this PR but no response yet :/ How is the output now? |
Ah, right; that was the first approach I took, but instead of keeping the
I sill think that would be a good alternative for now (so that we don't have to maintain our own multi error package). Given that it's currently only used in two locations, I don't think it'd be a huge issue to have those lines in those two locations.
With this, it should look something like: package main
import (
"fmt"
"github.com/hashicorp/go-multierror"
)
func main() {
fmt.Println(foo())
}
func foo() error {
var errs *multierror.Error
errs = multierror.Append(errs, multierror.Prefix(fmt.Errorf("cannot delete current context"), "Error:"))
errs = multierror.Append(errs, multierror.Prefix(fmt.Errorf("cannot delete current context"), "Error:"))
errs = multierror.Append(errs, multierror.Prefix(fmt.Errorf("cannot delete current context"), "Error:"))
return errs.ErrorOrNil()
} Which outputs https://play.golang.org/p/qlRMR2cF0AV
(so there will be a difference, as it adds the |
dc0c79b
to
89d6401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, it's not used that much so it's ok to just use hashicorp's mutlierror.
Let me know if you want the format to be updated, then I can take the other approach instead |
Yeah to think of it, let's define our list function right away that way the output doesn't change once the PR is merged over at go-multierror |
89d6401
to
15ca0e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@thaJeztah could you please rebase with main? |
15ca0e7
to
8d755ae
Compare
The hashicorp/go-multierror package provides functionality to do this. Replacing the custom package in favor of those (at the cost of some small duplication). Signed-off-by: Sebastiaan van Stijn <[email protected]>
8d755ae
to
c862d78
Compare
Rebased 👍 |
Thank you |
What I did
The hashicorp/go-multierror package provides functionality to do this. Replacing the custom package in favor of those (at the cost of some small duplication).
Removing the custom package will prevent our implementation to diverge from the upstream. Alternatively, we could fully implement our own "multi error" package (instead of using the hashicorp package), which could be a simplified version that only provides what we need, but we'd have to maintain that.
Also added a nil-check to prevent a potential panic if multierror was nil
Related issue
<-- If this is a bug fix, make sure your description includes "fixes #xxxx", or "closes #xxxx" -->
(not mandatory) A picture of a cute animal, if possible in relation with what you did