Skip to content

Fix prefix-matching for service ps #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aaronlehmann merged 3 commits into from
Jun 13, 2017
Merged

Fix prefix-matching for service ps #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6279612
Fix prefix-matching for service ps
thaJeztah May 31, 2017
b5baffd
Fix complexity of service/ps.
dnephin Jun 1, 2017
3718833
Add unit tests for service/ps
dnephin Jun 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 61 additions & 33 deletions cli/command/service/ps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Expand All @@ -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)
Copy link

@aaronlehmann aaronlehmann Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed it's a bit odd that that an ambiguous prefix causes service ps to fail with an error, but a nonexistent service causes ps to proceed for the other services and show a warning later. Shouldn't these cases be treated similarly?

Copy link
Member Author

@thaJeztah thaJeztah Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-existing services, I think we follow what we do for other commands. For example, cases like;

docker stop $(docker ps -aq)

Where we don't want the command to fail on the first "not found", however if ambiguous results would be accepted, such a command could easily lead to multiple objects being affected.

Copy link

@aaronlehmann aaronlehmann Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand. Are you saying we should fail the whole command on an ambiguous prefix but not a nonexistent service? Why is an ambiguous prefix different from not finding the service?

Copy link
Contributor

@dnephin dnephin Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's the correct behaviour. It matches what we do for other commands.

We don't want to immediately fail on "not found" because there is other work we can do. But in the case of an ambiguous prefix the work is undefined, so we can't really do anything but error.

}
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"))
Copy link

@aaronlehmann aaronlehmann Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if it would make sense to just return an empty set of filters here, and have the caller detect that and format the error message. Or we could always return a formatted warning string here. But I think duplicating the errors.New(strings.Join(notfound, "\n")) code in both places is not ideal.

Copy link
Contributor

@dnephin dnephin Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way this has to happen twice, because the other case doesn't return an error until after printing the results.

Also the filters wont necessarily be empty because the user can set other filters (node filters for one, not sure about others).

I can make it a function if you feel that improves the code.

}
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
}
118 changes: 118 additions & 0 deletions cli/command/service/ps_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")
}