From 62796124432c7e56e7dda226c3c53c8c2356a30c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 31 May 2017 21:43:56 +0200 Subject: [PATCH 1/3] Fix prefix-matching for service ps The docker CLI matches objects either by ID _prefix_ or a full name match, but not partial name matches. The correct order of resolution is; - Full ID match (a name should not be able to mask an ID) - Full name - ID-prefix This patch changes the way services are matched. Also change to use the first matching service, if there's a full match (by ID or Name) instead of continue looking for other possible matches. Error handling changed; - Do not error early if multiple services were requested and one or more services were not found. Print the services that were not found after printing those that _were_ found instead - Print an error if ID-prefix matching is ambiguous Signed-off-by: Sebastiaan van Stijn --- cli/command/service/ps.go | 51 +++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/cli/command/service/ps.go b/cli/command/service/ps.go index 87b29d2043..7c744478ec 100644 --- a/cli/command/service/ps.go +++ b/cli/command/service/ps.go @@ -69,29 +69,43 @@ func runPS(dockerCli command.Cli, options psOptions) error { return err } + var errs []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 } } - // If nothing has been found, return immediately. - if serviceCount == 0 { - return errors.Errorf("no such services: %s", service) + found := false + for _, s := range serviceByIDList { + if strings.HasPrefix(s.ID, service) { + if found { + return errors.New("multiple services found with provided prefix: " + service) + } + filter.Add("service", s.ID) + serviceCount++ + found = true + } + } + if !found { + errs = append(errs, "no such service: "+service) } } - + if serviceCount == 0 { + return errors.New(strings.Join(errs, "\n")) + } if filter.Include("node") { nodeFilters := filter.Get("node") for _, nodeFilter := range nodeFilters { @@ -117,6 +131,11 @@ func runPS(dockerCli command.Cli, options psOptions) error { format = formatter.TableFormatKey } } - - return task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format) + if err := task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format); err != nil { + return err + } + if len(errs) != 0 { + return errors.New(strings.Join(errs, "\n")) + } + return nil } From b5baffde44cf16bc2eb030c09da76d39abb62252 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 1 Jun 2017 11:47:43 -0400 Subject: [PATCH 2/3] Fix complexity of service/ps. Signed-off-by: Daniel Nephin --- cli/command/service/ps.go | 65 ++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/cli/command/service/ps.go b/cli/command/service/ps.go index 7c744478ec..741f6b589f 100644 --- a/cli/command/service/ps.go +++ b/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,14 +91,14 @@ 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 errs []string + 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 @@ -92,7 +121,7 @@ loop: for _, s := range serviceByIDList { if strings.HasPrefix(s.ID, service) { if found { - return errors.New("multiple services found with provided prefix: " + service) + return filter, nil, errors.New("multiple services found with provided prefix: " + service) } filter.Add("service", s.ID) serviceCount++ @@ -100,42 +129,22 @@ loop: } } if !found { - errs = append(errs, "no such service: "+service) + notfound = append(notfound, "no such service: "+service) } } if serviceCount == 0 { - return errors.New(strings.Join(errs, "\n")) + 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 - } - } - if err := task.Print(ctx, dockerCli, tasks, idresolver.New(client, options.noResolve), !options.noTrunc, options.quiet, format); err != nil { - return err - } - if len(errs) != 0 { - return errors.New(strings.Join(errs, "\n")) - } - return nil + return filter, notfound, err } From 3718833f2cecde35ae1ed007a9b2d5bf507c7c66 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 1 Jun 2017 12:59:11 -0400 Subject: [PATCH 3/3] Add unit tests for service/ps Signed-off-by: Daniel Nephin --- cli/command/service/ps_test.go | 118 +++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 cli/command/service/ps_test.go diff --git a/cli/command/service/ps_test.go b/cli/command/service/ps_test.go new file mode 100644 index 0000000000..a53748d6d3 --- /dev/null +++ b/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") +}