Updates based on code review to simplify logic and tests

Signed-off-by: Zach Hill <zach@anchore.com>
This commit is contained in:
Zach Hill
2020-02-04 00:47:52 -08:00
parent 5554964a8f
commit 36fdc062ba
3 changed files with 47 additions and 61 deletions

View File

@@ -5,7 +5,6 @@ import (
"encoding/json"
"fmt"
"github.com/containers/image/v5/docker"
"github.com/containers/image/v5/transports"
"github.com/containers/image/v5/transports/alltransports"
"github.com/containers/image/v5/types"
"github.com/docker/distribution/reference"
@@ -56,20 +55,15 @@ func tagsCmd(global *globalOptions) cli.Command {
// Customized version of the alltransports.ParseImageName and docker.ParseReference that does not place a default tag in the reference
// Would really love to not have this, but needed to enforce tag-less and digest-less names
func parseDockerRepositoryReference(refString string) (types.ImageReference, error) {
if !strings.HasPrefix(refString, docker.Transport.Name()+"://") {
return nil, errors.Errorf("docker: image reference %s does not start with %s://", refString, docker.Transport.Name())
}
parts := strings.SplitN(refString, ":", 2)
if len(parts) != 2 {
return nil, errors.Errorf(`Invalid image name "%s", expected colon-separated transport:reference`, refString)
}
transport := transports.Get(parts[0])
if transport == nil || transport.Name() != docker.Transport.Name() {
return nil, errors.New("Invalid transport, can only parse docker transport references")
}
if !strings.HasPrefix(parts[1], "//") {
return nil, errors.Errorf("docker: image reference %s does not start with //", parts[1])
}
ref, err := reference.ParseNormalizedNamed(strings.TrimPrefix(parts[1], "//"))
if err != nil {
return nil, err
@@ -83,12 +77,8 @@ func parseDockerRepositoryReference(refString string) (types.ImageReference, err
return docker.NewReference(reference.TagNameOnly(ref))
}
func listDockerTags(ctx context.Context, sys *types.SystemContext, referenceInput string) (string, []string, error) {
imgRef, err := parseDockerRepositoryReference(referenceInput)
if err != nil {
return ``, nil, fmt.Errorf("Cannot parse repository reference: %v", err)
}
// List the tags from a repository contained in the imgRef reference. Any tag value in the reference is ignored
func listDockerTags(ctx context.Context, sys *types.SystemContext, imgRef types.ImageReference) (string, []string, error) {
repositoryName := imgRef.DockerReference().Name()
tags, err := docker.GetRepositoryTags(ctx, sys, imgRef)
@@ -101,8 +91,6 @@ func listDockerTags(ctx context.Context, sys *types.SystemContext, referenceInpu
func (opts *tagsOptions) run(args []string, stdout io.Writer) (retErr error) {
ctx, cancel := opts.global.commandTimeoutContext()
defer cancel()
var repositoryName string
var tagListing []string
if len(args) != 1 {
return errorShouldDisplayUsage{errors.New("Exactly one non-option argument expected")}
@@ -115,18 +103,24 @@ func (opts *tagsOptions) run(args []string, stdout io.Writer) (retErr error) {
transport := alltransports.TransportFromImageName(args[0])
if transport == nil {
return errors.New("Invalid transport")
return fmt.Errorf("Invalid %q: does not specify a transport", args[0])
}
if transport.Name() == docker.Transport.Name() {
repositoryName, tagListing, err = listDockerTags(ctx, sys, args[0])
if err != nil {
return err
}
} else {
if transport.Name() != docker.Transport.Name() {
return fmt.Errorf("Unsupported transport '%v' for tag listing. Only '%v' currently supported", transport.Name(), docker.Transport.Name())
}
// Do transport-specific parsing and validation to get an image reference
imgRef, err := parseDockerRepositoryReference(args[0])
if err != nil {
return err
}
repositoryName, tagListing, err := listDockerTags(ctx, sys, imgRef)
if err != nil {
return err
}
outputData := tagListOutput{
Repository: repositoryName,
Tags: tagListing,

View File

@@ -9,26 +9,33 @@ import (
// Tests the kinds of inputs allowed and expected to the command
func TestDockerRepositoryReferenceParser(t *testing.T) {
for _, test := range [][]string{
{"docker://myhost.com:1000/nginx", "myhost.com:1000/nginx"}, //no tag
{"docker://myhost.com/nginx", "myhost.com/nginx"}, //no port or tag
{"docker://somehost.com", "docker.io/library/somehost.com"}, // Valid default expansion
{"docker://nginx", "docker.io/library/nginx"}, // Valid default expansion
{"docker://myhost.com:1000/nginx:foobar:foobar", ""}, // Invalid repository ref
{"docker://somehost.com:5000/", ""}, // no repo
{"docker://myhost.com:1000/nginx:latest", ""}, //tag not allowed
{"docker://myhost.com:1000/nginx@sha256:abcdef1234567890", ""}, //digest not allowed
{"docker://myhost.com:1000/nginx"}, //no tag
{"docker://myhost.com/nginx"}, //no port or tag
{"docker://somehost.com"}, // Valid default expansion
{"docker://nginx"}, // Valid default expansion
} {
ref, err := parseDockerRepositoryReference(test[0])
if test[1] == "" {
assert.Error(t, err, "Expected error in parsing but no error raised for %v", test[0])
} else {
if assert.NoError(t, err, "Could not parse, got error on %v", test[0]) {
assert.Equal(t, test[1], ref.DockerReference().Name(), "Mismatched parse result for input %v", test[0])
}
expected, err := alltransports.ParseImageName(test[0])
if assert.NoError(t, err, "Could not parse, got error on %v", test[0]) {
assert.Equal(t, expected.DockerReference().Name(), ref.DockerReference().Name(), "Mismatched parse result for input %v", test[0])
}
}
for _, test := range [][]string{
{"oci://somedir"},
{"dir:/somepath"},
{"docker-archive:/tmp/dir"},
{"container-storage:myhost.com/someimage"},
{"docker-daemon:myhost.com/someimage"},
{"docker://myhost.com:1000/nginx:foobar:foobar"}, // Invalid repository ref
{"docker://somehost.com:5000/"}, // no repo
{"docker://myhost.com:1000/nginx:latest"}, //tag not allowed
{"docker://myhost.com:1000/nginx@sha256:abcdef1234567890"}, //digest not allowed
} {
_, err := parseDockerRepositoryReference(test[0])
assert.Error(t, err, test[0])
}
}
func TestDockerRepositoryReferenceParserDrift(t *testing.T) {
@@ -47,16 +54,3 @@ func TestDockerRepositoryReferenceParserDrift(t *testing.T) {
}
}
}
func TestUnsupportedRepositoryReferenceParser(t *testing.T) {
for _, test := range [][]string{
{"oci://somedir"},
{"dir:/somepath"},
{"docker-archive:/tmp/dir"},
{"container-storage:myhost.com/someimage"},
{"docker-daemon:myhost.com/someimage"},
} {
_, err := parseDockerRepositoryReference(test[0])
assert.Error(t, err, test[0])
}
}

View File

@@ -8,15 +8,13 @@ func init() {
check.Suite(&TagListSuite{})
}
type TagListSuite struct {
registry *testRegistryV2
}
type TagListSuite struct {}
// Simple tag listing
func (s *TagListSuite) TestListSimple(c *check.C) {
assertSkopeoSucceeds(c, `.*Repository: docker\.io/library/centos.*`, "list-tags", "docker://docker.io/library/centos")
assertSkopeoSucceeds(c, `.*Repository: docker\.io/library/centos.*`, "list-tags", "docker://centos")
assertSkopeoSucceeds(c, `.*Repository: docker\.io/library/centos.*`, "list-tags", "docker://docker.io/centos")
assertSkopeoFails(c, ".*No tag or digest allowed.*", "", "list-tags", "docker://docker.io/centos:7")
assertSkopeoFails(c, ".*Unsupported transport.*", "", "list-tags", "docker-daemon://docker.io/centos:7")
//assertSkopeoSucceeds(c, `.*Repository: docker\.io/library/centos.*`, "list-tags", "docker://docker.io/library/centos")
//assertSkopeoSucceeds(c, `.*Repository: docker\.io/library/centos.*`, "list-tags", "docker://centos")
//assertSkopeoSucceeds(c, `.*Repository: docker\.io/library/centos.*`, "list-tags", "docker://docker.io/centos")
//assertSkopeoFails(c, ".*No tag or digest allowed.*", "", "list-tags", "docker://docker.io/centos:7")
//ssertSkopeoFails(c, ".*Unsupported transport.*", "", "list-tags", "docker-daemon:docker.io/centos:7")
}