-
Notifications
You must be signed in to change notification settings - Fork 4
Try docker daemon before reading remote image #18
Conversation
pkg/registry/remote.go
Outdated
@@ -58,7 +68,7 @@ func writeRemoteImage(i v1.Image, n image.Name) error { | |||
return err | |||
} | |||
|
|||
return remote.Write(ref, i, auth, http.DefaultTransport) | |||
return remote.Write(ref, i, remote.WithAuth(auth), remote.WithTransport(http.DefaultTransport)) |
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.
This change was necessitated because of bumping go-containerregistry.
pkg/registry/remote.go
Outdated
var derr error | ||
img, derr = daemon.Image(ref) | ||
if derr != nil { | ||
return nil, fmt.Errorf("reading remote image %s failed: %v; attempting to read from daemon also failed: %v", n.String(), err, derr) |
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 had to think carefully about the error message in this case. Initially, I ignored the daemon error and reported back just the repository error - the rationale being that since there is no hard dependency on the daemon, we should report errors from the daemon. However, this could mask a genuine problem with the daemon, so I decided to combine the two errors together, for example:
$ ./irel layout add layout glyn/helloworld-cnab:a028fb535114b81ae38a6711b5de896642a07dfd
2019/08/07 13:34:50 add failed: reading remote image docker.io/glyn/helloworld-cnab:a028fb535114b81ae38a6711b5de896642a07dfd failed: MANIFEST_UNKNOWN: manifest unknown; map[Tag:a028fb535114b81ae38a6711b5de896642a07dfd]; attempting to read from daemon also failed: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
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.
Is there a way to add tests for these changes?
I considered that previously, but on second thoughts, you're right and I'll add some tests. |
See cnabio/duffle#828 (comment). Bump the go-containerregistry dependency and use a corresponding version of docker. Handle an empty image name more gracefully (than panicking).
There are some paths that can't be exercised because it's impossible to construct image names which are invalid in certain ways. However, the coverage is much improved and the main logic is now tested. Thanks for suggesting this. |
Codecov Report
@@ Coverage Diff @@
## master #18 +/- ##
==========================================
+ Coverage 61.01% 68.07% +7.05%
==========================================
Files 7 7
Lines 277 285 +8
==========================================
+ Hits 169 194 +25
+ Misses 96 75 -21
- Partials 12 16 +4
Continue to review full report at Codecov.
|
Expect(err).NotTo(HaveOccurred()) | ||
mockImage.DigestReturns(h1, nil) | ||
|
||
testError = errors.New("hard cheese") |
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.
🧀
It's better to use an image in the daemon than to fail. See cnabio/duffle#828 (comment).
Bump the go-containerregistry dependency and use a corresponding version of docker.