Merge pull request #72 from thaJeztah/fix-prefix-matching

Fix prefix-matching for service ps

cli/command/service/ps.go

@@ -12,6 +12,7 @@ import (
 	"github.com/docker/cli/opts"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/client"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"golang.org/x/net/context"
@@ -52,57 +53,10 @@ func runPS(dockerCli command.Cli, options psOptions) error {
 	client := dockerCli.Client()
 	ctx := context.Background()
 
-	filter := options.filter.Value()
+	filter, notfound, err := createFilter(ctx, client, options)
-
-	serviceIDFilter := filters.NewArgs()
-	serviceNameFilter := filters.NewArgs()
-	for _, service := range options.services {
-		serviceIDFilter.Add("id", service)
-		serviceNameFilter.Add("name", service)
-	}
-	serviceByIDList, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: serviceIDFilter})
 	if err != nil {
 		return err
 	}
-	serviceByNameList, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: serviceNameFilter})
-	if err != nil {
-		return err
-	}
-
-	for _, service := range options.services {
-		serviceCount := 0
-		// Lookup by ID/Prefix
-		for _, serviceEntry := range serviceByIDList {
-			if strings.HasPrefix(serviceEntry.ID, service) {
-				filter.Add("service", serviceEntry.ID)
-				serviceCount++
-			}
-		}
-
-		// Lookup by Name/Prefix
-		for _, serviceEntry := range serviceByNameList {
-			if strings.HasPrefix(serviceEntry.Spec.Annotations.Name, service) {
-				filter.Add("service", serviceEntry.ID)
-				serviceCount++
-			}
-		}
-		// If nothing has been found, return immediately.
-		if serviceCount == 0 {
-			return errors.Errorf("no such services: %s", service)
-		}
-	}
-
-	if filter.Include("node") {
-		nodeFilters := filter.Get("node")
-		for _, nodeFilter := range nodeFilters {
-			nodeReference, err := node.Reference(ctx, client, nodeFilter)
-			if err != nil {
-				return err
-			}
-			filter.Del("node", nodeFilter)
-			filter.Add("node", nodeReference)
-		}
-	}
 
 	tasks, err := client.TaskList(ctx, types.TaskListOptions{Filters: filter})
 	if err != nil {
@@ -117,6 +71,80 @@ func runPS(dockerCli command.Cli, options psOptions) error {
 			format = formatter.TableFormatKey
 		}
 	}
-
+	if err := task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format); err != nil {
-	return task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format)
+		return err
+	}
+	if len(notfound) != 0 {
+		return errors.New(strings.Join(notfound, "\n"))
+	}
+	return nil
+}
+
+func createFilter(ctx context.Context, client client.APIClient, options psOptions) (filters.Args, []string, error) {
+	filter := options.filter.Value()
+
+	serviceIDFilter := filters.NewArgs()
+	serviceNameFilter := filters.NewArgs()
+	for _, service := range options.services {
+		serviceIDFilter.Add("id", service)
+		serviceNameFilter.Add("name", service)
+	}
+	serviceByIDList, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: serviceIDFilter})
+	if err != nil {
+		return filter, nil, err
+	}
+	serviceByNameList, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: serviceNameFilter})
+	if err != nil {
+		return filter, nil, err
+	}
+
+	var notfound []string
+	serviceCount := 0
+loop:
+	// Match services by 1. Full ID, 2. Full name, 3. ID prefix. An error is returned if the ID-prefix match is ambiguous
+	for _, service := range options.services {
+		for _, s := range serviceByIDList {
+			if s.ID == service {
+				filter.Add("service", s.ID)
+				serviceCount++
+				continue loop
+			}
+		}
+		for _, s := range serviceByNameList {
+			if s.Spec.Annotations.Name == service {
+				filter.Add("service", s.ID)
+				serviceCount++
+				continue loop
+			}
+		}
+		found := false
+		for _, s := range serviceByIDList {
+			if strings.HasPrefix(s.ID, service) {
+				if found {
+					return filter, nil, errors.New("multiple services found with provided prefix: " + service)
+				}
+				filter.Add("service", s.ID)
+				serviceCount++
+				found = true
+			}
+		}
+		if !found {
+			notfound = append(notfound, "no such service: "+service)
+		}
+	}
+	if serviceCount == 0 {
+		return filter, nil, errors.New(strings.Join(notfound, "\n"))
+	}
+	if filter.Include("node") {
+		nodeFilters := filter.Get("node")
+		for _, nodeFilter := range nodeFilters {
+			nodeReference, err := node.Reference(ctx, client, nodeFilter)
+			if err != nil {
+				return filter, nil, err
+			}
+			filter.Del("node", nodeFilter)
+			filter.Add("node", nodeReference)
+		}
+	}
+	return filter, notfound, err
 }

cli/command/service/ps_test.go

@@ -0,0 +1,118 @@
+package service
+
+import (
+	"testing"
+
+	"bytes"
+
+	"github.com/docker/cli/cli/internal/test"
+	"github.com/docker/cli/opts"
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/api/types/swarm"
+	"github.com/docker/docker/client"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/net/context"
+)
+
+type fakeClient struct {
+	client.Client
+	serviceListFunc func(context.Context, types.ServiceListOptions) ([]swarm.Service, error)
+}
+
+func (f *fakeClient) ServiceList(ctx context.Context, options types.ServiceListOptions) ([]swarm.Service, error) {
+	if f.serviceListFunc != nil {
+		return f.serviceListFunc(ctx, options)
+	}
+	return nil, nil
+}
+
+func (f *fakeClient) TaskList(ctx context.Context, options types.TaskListOptions) ([]swarm.Task, error) {
+	return nil, nil
+}
+
+func newService(id string, name string) swarm.Service {
+	return swarm.Service{
+		ID:   id,
+		Spec: swarm.ServiceSpec{Annotations: swarm.Annotations{Name: name}},
+	}
+}
+
+func TestCreateFilter(t *testing.T) {
+	client := &fakeClient{
+		serviceListFunc: func(ctx context.Context, options types.ServiceListOptions) ([]swarm.Service, error) {
+			return []swarm.Service{
+				{ID: "idmatch"},
+				{ID: "idprefixmatch"},
+				newService("cccccccc", "namematch"),
+				newService("01010101", "notfoundprefix"),
+			}, nil
+		},
+	}
+
+	filter := opts.NewFilterOpt()
+	require.NoError(t, filter.Set("node=somenode"))
+	options := psOptions{
+		services: []string{"idmatch", "idprefix", "namematch", "notfound"},
+		filter:   filter,
+	}
+
+	actual, notfound, err := createFilter(context.Background(), client, options)
+	require.NoError(t, err)
+	assert.Equal(t, notfound, []string{"no such service: notfound"})
+
+	expected := filters.NewArgs()
+	expected.Add("service", "idmatch")
+	expected.Add("service", "idprefixmatch")
+	expected.Add("service", "cccccccc")
+	expected.Add("node", "somenode")
+	assert.Equal(t, expected, actual)
+}
+
+func TestCreateFilterWithAmbiguousIDPrefixError(t *testing.T) {
+	client := &fakeClient{
+		serviceListFunc: func(ctx context.Context, options types.ServiceListOptions) ([]swarm.Service, error) {
+			return []swarm.Service{
+				{ID: "aaaone"},
+				{ID: "aaatwo"},
+			}, nil
+		},
+	}
+	options := psOptions{
+		services: []string{"aaa"},
+		filter:   opts.NewFilterOpt(),
+	}
+	_, _, err := createFilter(context.Background(), client, options)
+	assert.EqualError(t, err, "multiple services found with provided prefix: aaa")
+}
+
+func TestCreateFilterNoneFound(t *testing.T) {
+	client := &fakeClient{}
+	options := psOptions{
+		services: []string{"foo", "notfound"},
+		filter:   opts.NewFilterOpt(),
+	}
+	_, _, err := createFilter(context.Background(), client, options)
+	assert.EqualError(t, err, "no such service: foo\nno such service: notfound")
+}
+
+func TestRunPSWarnsOnNotFound(t *testing.T) {
+	client := &fakeClient{
+		serviceListFunc: func(ctx context.Context, options types.ServiceListOptions) ([]swarm.Service, error) {
+			return []swarm.Service{
+				{ID: "foo"},
+			}, nil
+		},
+	}
+
+	out := new(bytes.Buffer)
+	cli := test.NewFakeCli(client, out)
+	options := psOptions{
+		services: []string{"foo", "bar"},
+		filter:   opts.NewFilterOpt(),
+		format:   "{{.ID}}",
+	}
+	err := runPS(cli, options)
+	assert.EqualError(t, err, "no such service: bar")
+}