-
Notifications
You must be signed in to change notification settings - Fork 241
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.
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
Wrap some driver-level errors #73
Conversation
drivers/btrfs/btrfs.go
Outdated
@@ -27,8 +27,10 @@ import ( | |||
"github.com/containers/storage/pkg/idtools" | |||
"github.com/containers/storage/pkg/mount" | |||
"github.com/containers/storage/pkg/parsers" | |||
|
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.
Why did you add this space.
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.
At some point, various source files appear to have been grouping import statements into standard library, packages that are part of this repository, and vendored code, but it's inconsistent. I had started trying to clean that up, but it can wait.
drivers/zfs/zfs.go
Outdated
"github.com/containers/storage/drivers" | ||
"github.com/containers/storage/pkg/idtools" | ||
"github.com/containers/storage/pkg/mount" | ||
"github.com/containers/storage/pkg/parsers" | ||
|
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.
Nit remove space.
drivers/zfs/zfs_freebsd.go
Outdated
@@ -6,6 +6,8 @@ import ( | |||
"syscall" | |||
|
|||
"github.com/Sirupsen/logrus" | |||
"github.com/pkg/errors" | |||
|
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.
Don't add space.
drivers/zfs/zfs_freebsd.go
Outdated
@@ -18,7 +20,7 @@ func checkRootdirFs(rootdir string) error { | |||
// on FreeBSD buf.Fstypename contains ['z', 'f', 's', 0 ... ] | |||
if (buf.Fstypename[0] != 122) || (buf.Fstypename[1] != 102) || (buf.Fstypename[2] != 115) || (buf.Fstypename[3] != 0) { | |||
logrus.Debugf("[zfs] no zfs dataset found for rootdir '%s'", rootdir) | |||
return graphdriver.ErrPrerequisites | |||
return errors.Wrapf(graphdriver.ErrPrerequisites, "%q is not on a zfs filesystem", rootdir) |
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.
How come logrus and error message don't match?
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.
Copy+paste error, fixing.
drivers/zfs/zfs_linux.go
Outdated
@@ -5,6 +5,8 @@ import ( | |||
"syscall" | |||
|
|||
"github.com/Sirupsen/logrus" | |||
"github.com/pkg/errors" | |||
|
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.
Remove space
drivers/driver.go
Outdated
@@ -227,7 +227,8 @@ func New(root string, name string, options []string, uidMaps, gidMaps []idtools. | |||
|
|||
// isDriverNotSupported returns true if the error initializing | |||
// the graph driver is a non-supported error. | |||
func isDriverNotSupported(err error) bool { | |||
func isDriverNotSupported(werr error) bool { |
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.
Nit, I would leave interface as err and then use werr inside.
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.
Okay. Renaming werr
to cause
if we're going to change that line anyway.
drivers/driver_solaris.go
Outdated
@@ -56,7 +57,7 @@ func Mounted(fsType FsMagic, mountPath string) (bool, error) { | |||
(buf.f_basetype[3] != 0) { | |||
log.Debugf("[zfs] no zfs dataset found for rootdir '%s'", mountPath) | |||
C.free(unsafe.Pointer(buf)) | |||
return false, ErrPrerequisites | |||
return false, errors.Wrapf(graphdriver.ErrPrerequisites, "%q is not on a zfs filesystem", mountPath) |
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.
Shold log message and error match?
drivers/overlay/overlay.go
Outdated
@@ -26,7 +23,9 @@ import ( | |||
"github.com/containers/storage/pkg/parsers" | |||
"github.com/containers/storage/pkg/parsers/kernel" | |||
|
|||
"github.com/Sirupsen/logrus" |
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.
Remove newline above?
drivers/proxy.go
Outdated
"fmt" | ||
|
||
"github.com/pkg/errors" | ||
|
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.
Remove newline.
drivers/windows/windows.go
Outdated
@@ -23,14 +23,15 @@ import ( | |||
"github.com/Microsoft/go-winio/backuptar" | |||
"github.com/Microsoft/hcsshim" | |||
"github.com/Sirupsen/logrus" | |||
"github.com/vbatts/tar-split/tar/storage" | |||
|
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.
Remove newline.
Looks like this pull request replaces #70 |
Wrap graphdriver.{ErrNotSupported,ErrPrerequisites,ErrIncompatibleFS} errors in contexts using github.com/pkg/errors, and dig them out for comparison using errors.Cause(). Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Updated, undoing regrouping of import statements, making sure messages we use when wrapping errors match the diagnostics that we're logging. |
LGTM |
Wrap
graphdriver.{ErrNotSupported,ErrPrerequisites,ErrIncompatibleFS}
errors in contexts usinggithub.com/pkg/errors
, and dig them out for comparison usingerrors.Cause()
.