Fix prefix-matching for service ps #72

thaJeztah · 2017-05-11T13:12:01Z

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

This PR carries the CLI changes of moby/moby#32800

thaJeztah · 2017-05-11T13:12:23Z

ping @aaronlehmann @vdemeester PTAL

dnephin · 2017-05-11T15:28:13Z

I have a fix for the complexity failure in #61. If you're interested in applying those changes first (just the service/ps.go diff), then these changes on top, I think it should fix the build.

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 <github@gone.nl>

thaJeztah · 2017-05-31T19:47:16Z

Rebased, not sure if I should pick the changes from #61, due to the changes still being discussed there

dnephin · 2017-06-01T03:50:51Z

This doesn't need to be blocked on #61 . I'll prepare a commit with just the relevant fix for this failure and push it to this branch tomorrow.

codecov-io · 2017-06-01T15:47:47Z

Codecov Report

Merging #72 into master will increase coverage by 0.25%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##           master     #72      +/-   ##
=========================================
+ Coverage   44.95%   45.2%   +0.25%     
=========================================
  Files         169     169              
  Lines       11381   11399      +18     
=========================================
+ Hits         5116    5153      +37     
+ Misses       5972    5947      -25     
- Partials      293     299       +6

Signed-off-by: Daniel Nephin <dnephin@docker.com>

thaJeztah · 2017-06-01T15:56:03Z

Thanks @dnephin I was quite busy, and didn't find a moment into applying just that commit ❤️

dnephin · 2017-06-01T15:57:55Z

No problem!

I'm working on a unit test for this, since there aren't any yet.

dnephin · 2017-06-01T17:11:58Z

Added another commit with unit tests. Build is green

vdemeester

LGTM 👼

aaronlehmann · 2017-06-02T08:30:55Z

cli/command/service/ps_test.go

+	expected.Add("service", "abababab")
+	expected.Add("service", "cccccccc")
+	expected.Add("node", "somenode")
+	assert.Equal(t, expected, actual)


Can we also test prefix matches? Specifically:

An unambiguous ID prefix should match

An ambiguous ID prefix should lead to an error or warning

A name prefix should never match.

I've added the additional test cases

aaronlehmann · 2017-06-02T08:31:53Z

cli/command/service/ps.go

+		for _, s := range serviceByIDList {
+			if strings.HasPrefix(s.ID, service) {
+				if found {
+					return filter, nil, errors.New("multiple services found with provided prefix: " + service)


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?

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.

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?

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.

aaronlehmann · 2017-06-02T08:56:10Z

cli/command/service/ps.go

 		}
 	}
-
+	if serviceCount == 0 {
+		return filter, nil, errors.New(strings.Join(notfound, "\n"))


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.

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.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

aaronlehmann · 2017-06-02T15:38:34Z

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.

Why can't we do the work for the other values provided on the command line? Sorry if I seem difficult; I'm just trying to understand the distinction.

dnephin · 2017-06-07T16:58:13Z

Technically I think we could. I think this behaviour is designed to match what we do for other commands.

GordonTheTurtle added the status/0-triage label May 11, 2017

vdemeester added status/2-code-review and removed status/0-triage labels May 11, 2017

thaJeztah mentioned this pull request May 11, 2017

Fix prefix-matching for service ps moby/moby#32800

Closed

thaJeztah force-pushed the fix-prefix-matching branch from d564621 to 6279612 Compare May 31, 2017 19:45

thaJeztah mentioned this pull request May 31, 2017

17.06.0 RC2 tracker docker/for-linux#2

Closed

23 tasks

Fix complexity of service/ps.

b5baffd

Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the fix-prefix-matching branch from 0284d81 to b5baffd Compare June 1, 2017 15:49

vdemeester requested review from vdemeester, dnephin and aaronlehmann and removed request for dnephin June 1, 2017 17:13

vdemeester approved these changes Jun 1, 2017

View reviewed changes

aaronlehmann reviewed Jun 2, 2017

View reviewed changes

dnephin force-pushed the fix-prefix-matching branch from 13d1121 to 3718833 Compare June 2, 2017 15:29

Add unit tests for service/ps

3718833

Signed-off-by: Daniel Nephin <dnephin@docker.com>

thaJeztah mentioned this pull request Jun 7, 2017

17.06.0 RC3 tracker docker/for-linux#8

Closed

40 tasks

thaJeztah mentioned this pull request Jun 7, 2017

docker service ps matches prefix moby/moby#30417

Closed

andrewhsu mentioned this pull request Jun 13, 2017

17.06.0 RC4 tracker docker/for-linux#14

Closed

2 tasks

aaronlehmann merged commit 275c488 into docker:master Jun 13, 2017

GordonTheTurtle added this to the 17.07.0 milestone Jun 13, 2017

thaJeztah deleted the fix-prefix-matching branch June 13, 2017 22:04

andrewhsu mentioned this pull request Jun 14, 2017

[17.06] cherry-pick Fix prefix-matching for service ps docker-archive/docker-ce#77

Closed

dnephin mentioned this pull request Jun 15, 2017

docker servcie ps <service_name> lists all instances of services with names starting with service_name moby/moby#32556

Closed

andrewhsu mentioned this pull request Jun 19, 2017

17.06.0 RC5 tracker docker/for-linux#37

Closed

6 tasks

andrewhsu mentioned this pull request Jul 26, 2017

WIP [17.07] fix tests docker-archive/docker-ce#140

Closed

This was referenced Jul 27, 2017

docker ps <service> lists tasks for all services starting with service name <service> moby/moby#34265

Closed

docker ps <service> lists tasks for all services starting with service name <service> docker-archive/for-aws#77

Open

thaJeztah mentioned this pull request Dec 13, 2017

docker service ps gives all the services starting with the same name moby/moby#35766

Closed

thaJeztah mentioned this pull request Apr 30, 2018

[Kubernetes] stack services filters #1023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prefix-matching for service ps #72

Fix prefix-matching for service ps #72

thaJeztah commented May 11, 2017

thaJeztah commented May 11, 2017

dnephin commented May 11, 2017 •

edited

Loading

thaJeztah commented May 31, 2017

dnephin commented Jun 1, 2017

codecov-io commented Jun 1, 2017 •

edited

Loading

thaJeztah commented Jun 1, 2017

dnephin commented Jun 1, 2017

dnephin commented Jun 1, 2017

vdemeester left a comment

aaronlehmann Jun 2, 2017

dnephin Jun 2, 2017

aaronlehmann Jun 2, 2017

thaJeztah Jun 2, 2017

aaronlehmann Jun 2, 2017

dnephin Jun 2, 2017

aaronlehmann Jun 2, 2017

dnephin Jun 2, 2017

aaronlehmann commented Jun 2, 2017 via email

dnephin commented Jun 7, 2017

Fix prefix-matching for service ps #72

Fix prefix-matching for service ps #72

Conversation

thaJeztah commented May 11, 2017

thaJeztah commented May 11, 2017

dnephin commented May 11, 2017 • edited Loading

thaJeztah commented May 31, 2017

dnephin commented Jun 1, 2017

codecov-io commented Jun 1, 2017 • edited Loading

Codecov Report

thaJeztah commented Jun 1, 2017

dnephin commented Jun 1, 2017

dnephin commented Jun 1, 2017

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Jun 2, 2017 via email

dnephin commented Jun 7, 2017

dnephin commented May 11, 2017 •

edited

Loading

codecov-io commented Jun 1, 2017 •

edited

Loading