Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

[RFC] Remove custom multierrors package #426

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

thaJeztah
Copy link
Member

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

@@ -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:"))
Copy link
Member Author

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)

Copy link
Member Author

@thaJeztah thaJeztah Aug 5, 2020

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

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

@rumpl rumpl Aug 5, 2020

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?

@rumpl
Copy link
Contributor

rumpl commented Aug 5, 2020

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?

cli/cmd/context/rm.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member Author

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 :/

Ah, right; that was the first approach I took, but instead of keeping the multierror package, I took the approach from the go-multierror example in their readme;

if errs != nil {
	errs.ErrorFormat = listFormatFunc
}

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.

How is the output now?

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

3 errors occurred:
	* Error: cannot delete current context
	* Error: cannot delete current context
	* Error: cannot delete current context

(so there will be a difference, as it adds the 3 errors occurred:)

@thaJeztah thaJeztah force-pushed the simplify_multierror branch from dc0c79b to 89d6401 Compare August 5, 2020 10:31
Copy link
Contributor

@rumpl rumpl left a 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.

@thaJeztah
Copy link
Member Author

Let me know if you want the format to be updated, then I can take the other approach instead

@rumpl
Copy link
Contributor

rumpl commented Aug 5, 2020

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

@thaJeztah thaJeztah force-pushed the simplify_multierror branch from 89d6401 to 15ca0e7 Compare August 11, 2020 15:02
Copy link
Contributor

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

LGTM

@rumpl
Copy link
Contributor

rumpl commented Aug 11, 2020

@thaJeztah could you please rebase with main?

@thaJeztah thaJeztah force-pushed the simplify_multierror branch from 15ca0e7 to 8d755ae Compare August 11, 2020 15:23
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]>
@thaJeztah thaJeztah force-pushed the simplify_multierror branch from 8d755ae to c862d78 Compare August 11, 2020 15:25
@thaJeztah
Copy link
Member Author

Rebased 👍

@rumpl
Copy link
Contributor

rumpl commented Aug 11, 2020

Thank you

@rumpl rumpl merged commit 2b9da84 into docker-archive:main Aug 11, 2020
@thaJeztah thaJeztah deleted the simplify_multierror branch August 11, 2020 15:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants