driver/docker: pull image with digest #4298
Conversation
GH #4290 Add digest support to the docker driver image config. This commit factors out some common code to print the repo:tag (dockerImageRef) for events/logs as well as parsing the image to retreive the repo,tag (parseDockerImage) so that the results are consistent/sane for both repo:tag and repo@sha256:... references. When pulling an image with a digest, the tag is blank and the repo contains the digest. See: https://github.com/fsouza/go-dockerclient/blob/master/image_test.go#L471
client/driver/docker.go
Outdated
|
|
||
| func parseDockerImage(image string) (repo, tag string) { | ||
| repo, tag = docker.ParseRepositoryTag(image) | ||
| if tag == "" { |
There was a problem hiding this comment.
if tag != "" { return repo, tag }
reduce indentation and make it explicit whats returned :)
| if tag != "" { | ||
| return repo, tag | ||
| } | ||
| if i := strings.IndexRune(image, '@'); i > -1 { // Has digest (@sha256:...) |
There was a problem hiding this comment.
is there a risk of someone using @ in their repo name/url/tag? wonder if its more safe to actually sniff for @sha256: in its entirely - not aware of the docker repo spec if its invalid or not
There was a problem hiding this comment.
I'm pretty sure that '@' is not a valid repo name. The official spec is codified here:
https://github.com/docker/distribution/blob/master/reference/regexp.go
client/driver/docker_test.go
Outdated
| t.Run(test.Image, func(t *testing.T) { | ||
| repo, tag := parseDockerImage(test.Image) | ||
| if repo != test.Repo { | ||
| t.Errorf("expected repo '%s' but got '%s'", test.Repo, repo) |
There was a problem hiding this comment.
Could you change these to use the require package: https://godoc.org/github.com/stretchr/testify/require#Equal
client/driver/docker_test.go
Outdated
|
|
||
| waitForExist(t, client, handle) | ||
|
|
||
| _, err := client.InspectContainer(handle.ContainerID()) |
There was a problem hiding this comment.
Check that the inspected container's image is whats expected.
There was a problem hiding this comment.
I can do this, but the remote digest won't match the local digest due to "reasons" so I'll have to just hard-code in another id for the local digest to match against. Not ideal
client/driver/docker_test.go
Outdated
| waitForExist(t, client, handle) | ||
|
|
||
| _, err := client.InspectContainer(handle.ContainerID()) | ||
| if err != nil { |
| return repo, tag | ||
| } | ||
|
|
||
| func dockerImageRef(repo string, tag string) string { |
There was a problem hiding this comment.
Would you mind adding a quick test for this. Could even just be a line in TestParseDockerImage
There was a problem hiding this comment.
TestDockerImageRef added
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #4290
Fixes #4211
Add digest support to the docker driver image config. This commit
factors out some common code to print the repo:tag (dockerImageRef) for
events/logs as well as parsing the image to retreive the repo,tag
(parseDockerImage) so that the results are consistent/sane for both
repo:tag and repo@sha256:... references.
When pulling an image with a digest, the tag is blank and the repo
contains the digest. See:
https://github.com/fsouza/go-dockerclient/blob/master/image_test.go#L471