[17.06] cherry-pick Fix prefix-matching for service ps

components/cli/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,6 +53,34 @@ func runPS(dockerCli command.Cli, options psOptions) error {
 	client := dockerCli.Client()
 	ctx := context.Background()
 
+	filter, notfound, err := createFilter(ctx, client, options)
+	if err != nil {
+		return err
+	}
+
+	tasks, err := client.TaskList(ctx, types.TaskListOptions{Filters: filter})
+	if err != nil {
+		return err
+	}
+
+	format := options.format
+	if len(format) == 0 {
+		if len(dockerCli.ConfigFile().TasksFormat) > 0 && !options.quiet {
+			format = dockerCli.ConfigFile().TasksFormat
+		} else {
+			format = formatter.TableFormatKey
+		}
+	}
+	if err := task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format); err != nil {
+		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()
@@ -62,61 +91,60 @@ func runPS(dockerCli command.Cli, options psOptions) error {
 	}
 	serviceByIDList, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: serviceIDFilter})
 	if err != nil {
-		return err
+		return filter, nil, err
 	}
 	serviceByNameList, err := client.ServiceList(ctx, types.ServiceListOptions{Filters: serviceNameFilter})
 	if err != nil {
-		return err
+		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 {
-		serviceCount := 0
-		// Lookup by ID/Prefix
-		for _, serviceEntry := range serviceByIDList {
-			if strings.HasPrefix(serviceEntry.ID, service) {
-				filter.Add("service", serviceEntry.ID)
+		for _, s := range serviceByIDList {
+			if s.ID == service {
+				filter.Add("service", s.ID)
 				serviceCount++
+				continue loop
 			}
 		}
-
-		// Lookup by Name/Prefix
-		for _, serviceEntry := range serviceByNameList {
-			if strings.HasPrefix(serviceEntry.Spec.Annotations.Name, service) {
-				filter.Add("service", serviceEntry.ID)
+		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 nothing has been found, return immediately.
-		if serviceCount == 0 {
-			return errors.Errorf("no such services: %s", service)
+		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 err
+				return filter, nil, err
 			}
 			filter.Del("node", nodeFilter)
 			filter.Add("node", nodeReference)
 		}
 	}
-
-	tasks, err := client.TaskList(ctx, types.TaskListOptions{Filters: filter})
-	if err != nil {
-		return err
-	}
-
-	format := options.format
-	if len(format) == 0 {
-		if len(dockerCli.ConfigFile().TasksFormat) > 0 && !options.quiet {
-			format = dockerCli.ConfigFile().TasksFormat
-		} else {
-			format = formatter.TableFormatKey
-		}
-	}
-
-	return task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format)
+	return filter, notfound, err
 }

components/cli/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")
+}

components/engine/integration-cli/docker_cli_swarm_test.go

@@ -1691,41 +1691,33 @@ func (s *DockerSwarmSuite) TestSwarmServicePsMultipleServiceIDs(c *check.C) {
 	d := s.AddDaemon(c, true, true)
 
 	name1 := "top1"
-	out, err := d.Cmd("service", "create", "--no-resolve-image", "--detach=true", "--name", name1, "--replicas=3", "busybox", "top")
-	c.Assert(err, checker.IsNil)
-	c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
-	id1 := strings.TrimSpace(out)
+	result := icmd.RunCmd(d.Command("service", "create", "--no-resolve-image", "--detach=true", "--name", name1, "--replicas=3", "busybox", "top"))
+	result.Assert(c, icmd.Success)
+	id1 := strings.TrimSpace(result.Stdout())
+	c.Assert(id1, checker.Not(checker.Equals), "")
 
 	name2 := "top2"
-	out, err = d.Cmd("service", "create", "--no-resolve-image", "--detach=true", "--name", name2, "--replicas=3", "busybox", "top")
-	c.Assert(err, checker.IsNil)
-	c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
-	id2 := strings.TrimSpace(out)
+	result = icmd.RunCmd(d.Command("service", "create", "--no-resolve-image", "--detach=true", "--name", name2, "--replicas=3", "busybox", "top"))
+	result.Assert(c, icmd.Success)
+	id2 := strings.TrimSpace(result.Stdout())
+	c.Assert(id2, checker.Not(checker.Equals), "")
 
 	// make sure task has been deployed.
 	waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 6)
 
-	out, err = d.Cmd("service", "ps", name1)
-	c.Assert(err, checker.IsNil)
+	result = icmd.RunCmd(d.Command("service", "ps", name1))
+	result.Assert(c, icmd.Success)
+	out := result.Stdout()
 	c.Assert(out, checker.Contains, name1+".1")
 	c.Assert(out, checker.Contains, name1+".2")
 	c.Assert(out, checker.Contains, name1+".3")
 	c.Assert(out, checker.Not(checker.Contains), name2+".1")
 	c.Assert(out, checker.Not(checker.Contains), name2+".2")
 	c.Assert(out, checker.Not(checker.Contains), name2+".3")
 
-	out, err = d.Cmd("service", "ps", name1, name2)
-	c.Assert(err, checker.IsNil)
-	c.Assert(out, checker.Contains, name1+".1")
-	c.Assert(out, checker.Contains, name1+".2")
-	c.Assert(out, checker.Contains, name1+".3")
-	c.Assert(out, checker.Contains, name2+".1")
-	c.Assert(out, checker.Contains, name2+".2")
-	c.Assert(out, checker.Contains, name2+".3")
-
-	// Name Prefix
-	out, err = d.Cmd("service", "ps", "to")
-	c.Assert(err, checker.IsNil)
+	result = icmd.RunCmd(d.Command("service", "ps", name1, name2))
+	result.Assert(c, icmd.Success)
+	out = result.Stdout()
 	c.Assert(out, checker.Contains, name1+".1")
 	c.Assert(out, checker.Contains, name1+".2")
 	c.Assert(out, checker.Contains, name1+".3")
@@ -1734,21 +1726,25 @@ func (s *DockerSwarmSuite) TestSwarmServicePsMultipleServiceIDs(c *check.C) {
 	c.Assert(out, checker.Contains, name2+".3")
 
 	// Name Prefix (no hit)
-	out, err = d.Cmd("service", "ps", "noname")
-	c.Assert(err, checker.NotNil)
-	c.Assert(out, checker.Contains, "no such services: noname")
+	result = icmd.RunCmd(d.Command("service", "ps", "to"))
+	result.Assert(c, icmd.Expected{
+		ExitCode: 1,
+		Err: "no such services: to",
+	})
 
-	out, err = d.Cmd("service", "ps", id1)
-	c.Assert(err, checker.IsNil)
+	result = icmd.RunCmd(d.Command("service", "ps", id1))
+	result.Assert(c, icmd.Success)
+	out = result.Stdout()
 	c.Assert(out, checker.Contains, name1+".1")
 	c.Assert(out, checker.Contains, name1+".2")
 	c.Assert(out, checker.Contains, name1+".3")
 	c.Assert(out, checker.Not(checker.Contains), name2+".1")
 	c.Assert(out, checker.Not(checker.Contains), name2+".2")
 	c.Assert(out, checker.Not(checker.Contains), name2+".3")
 
-	out, err = d.Cmd("service", "ps", id1, id2)
-	c.Assert(err, checker.IsNil)
+	result = icmd.RunCmd(d.Command("service", "ps", id1, id2))
+	result.Assert(c, icmd.Success)
+	out = result.Stdout()
 	c.Assert(out, checker.Contains, name1+".1")
 	c.Assert(out, checker.Contains, name1+".2")
 	c.Assert(out, checker.Contains, name1+".3")