Skip to content

Commit 617022a

Browse files
committed
errors: remove usage of errors.Cause
This removes usages of `errors.Cause` in favor of using `errors.As` which is the more standard way of handling error causes. `Cause` was not used frequently and `errors.As` is part of the standard library and is supported by `github.com/pkg/errors`. For one usage of `errors.Cause`, the underlying implementation itself has a reliance on the internal implementation of the error being returned and this method isn't guaranteed to work. Since we are relying on an implementation detail and not an API, I've added a test to ensure the behavior continues working as designed. This also removes a check for an error being returned from containerd that has since been fixed so the code is dead. Signed-off-by: Jonathan A. Sternberg <[email protected]>
1 parent 8f4aa21 commit 617022a

File tree

4 files changed

+50
-16
lines changed

4 files changed

+50
-16
lines changed

cache/util/fsutil.go

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -131,16 +131,24 @@ func StatFile(ctx context.Context, mount snapshot.Mountable, path string) (*fsty
131131
// The filename here is internal to the mount, so we can restore
132132
// the request base path for error reporting.
133133
// See os.DirFS.Open for details.
134-
err1 := err
135-
if err := errors.Cause(err); err != nil {
136-
err1 = err
137-
}
138-
if pe, ok := err1.(*os.PathError); ok {
139-
pe.Path = path
140-
}
134+
replaceErrorPath(err, path)
141135
return errors.WithStack(err)
142136
}
143137
return nil
144138
})
145139
return st, err
146140
}
141+
142+
// replaceErrorPath will override the path in an os.PathError in the error chain.
143+
// This works with the fsutil library, but it isn't necessarily the correct
144+
// way to do this because the error message of wrapped errors doesn't necessarily
145+
// update or change when a wrapped error is changed.
146+
//
147+
// Still, this method of updating the path works with the way this specific
148+
// library returns errors.
149+
func replaceErrorPath(err error, path string) {
150+
var pe *os.PathError
151+
if errors.As(err, &pe) {
152+
pe.Path = path
153+
}
154+
}

cache/util/fsutil_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package util
2+
3+
import (
4+
"path"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
"github.com/tonistiigi/fsutil"
9+
)
10+
11+
// TestSetErrorPath ensures that modifying the os.PathError from fsutil.Stat
12+
// modifies the error message appropriately.
13+
//
14+
// This method of modifying the path error is an implementation
15+
// detail of how the error is returned. It isn't necessarily the
16+
// case that this will always work if the underlying library changes.
17+
func TestSetErrorPath(t *testing.T) {
18+
// Temporary directory to reduce the chance of using stat on a real file.
19+
dir := t.TempDir()
20+
21+
// Random path that shouldn't exist.
22+
fpath := path.Join(dir, "a/b/c")
23+
_, err := fsutil.Stat(fpath)
24+
require.Error(t, err)
25+
26+
require.ErrorContains(t, err, "a/b/c")
27+
// Set the path in the error to a new path.
28+
replaceErrorPath(err, "/my/new/path")
29+
require.NotContains(t, err.Error(), "a/b/c")
30+
require.Contains(t, err.Error(), "/my/new/path")
31+
}

util/resolver/retryhandler/retry.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,7 @@ func retryError(err error) bool {
6464
return true
6565
}
6666
// catches TLS timeout or other network-related temporary errors
67-
if ne, ok := errors.Cause(err).(net.Error); ok && ne.Temporary() { //nolint:staticcheck // ignoring "SA1019: Temporary is deprecated", continue to propagate net.Error through the "temporary" status
68-
return true
69-
}
70-
// https://github.com/containerd/containerd/pull/4724
71-
if errors.Cause(err).Error() == "no response" {
67+
if ne := net.Error(nil); errors.As(err, &ne) && ne.Temporary() { //nolint:staticcheck // ignoring "SA1019: Temporary is deprecated", continue to propagate net.Error through the "temporary" status
7268
return true
7369
}
7470

util/system/path_test.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package system
33
import (
44
"testing"
55

6-
"github.com/pkg/errors"
76
"github.com/stretchr/testify/require"
87
)
98

@@ -77,7 +76,7 @@ func TestNormalizeWorkdir(t *testing.T) {
7776
t.Run(tc.name, func(t *testing.T) {
7877
result, err := NormalizeWorkdir(tc.currentWorkdir, tc.newWorkDir, "linux")
7978
if tc.err != "" {
80-
require.EqualError(t, errors.Cause(err), tc.err)
79+
require.ErrorContains(t, err, tc.err)
8180
} else {
8281
require.NoError(t, err)
8382
}
@@ -300,7 +299,7 @@ func TestNormalizeWorkdirWindows(t *testing.T) {
300299
t.Run(tc.name, func(t *testing.T) {
301300
result, err := NormalizeWorkdir(tc.currentWorkdir, tc.newWorkDir, "windows")
302301
if tc.err != "" {
303-
require.EqualError(t, errors.Cause(err), tc.err)
302+
require.ErrorContains(t, err, tc.err)
304303
} else {
305304
require.NoError(t, err)
306305
}
@@ -365,7 +364,7 @@ func TestNormalizeWorkdirUnix(t *testing.T) {
365364
t.Run(tc.name, func(t *testing.T) {
366365
result, err := NormalizeWorkdir(tc.currentWorkdir, tc.newWorkDir, "linux")
367366
if tc.err != "" {
368-
require.EqualError(t, errors.Cause(err), tc.err)
367+
require.ErrorContains(t, err, tc.err)
369368
} else {
370369
require.NoError(t, err)
371370
}

0 commit comments

Comments
 (0)