output some details when stack name does not exist #24051

allencloud · 2016-06-28T18:21:04Z

return err when stack name does not exist

Since there is no stack concept in the docker engine side, so check this in the client side.

As a result, output shows like this:

root@10-11-17-53:~/docker/bundles/1.12.0-dev/binary-client# ./docker stack tasks asasas
Nothing found in stack: asasas
root@10-11-17-53:~/docker/bundles/1.12.0-dev/binary-client# ./docker stack rm dsadasdsa
Nothing found in stack: dsadasdsa

This PR did:

add hint with nothing found in stack in command docker stack service xxx and docker stack remove xxx
add test for stack
fix some typos

Signed-off-by: allencloud allen.sun@daocloud.io

cpuguy83 · 2016-06-28T19:14:03Z

ping @aanand

aanand · 2016-06-28T21:52:16Z

Nice, thanks. Error message wording is a little inconsistent when a name isn't found, but the most common phrase seems to be "No such [thing]: [name]". Would you mind updating the message accordingly, so it reads "No such stack: %s"?

aanand · 2016-06-28T21:54:53Z

Hmm. Reading over the change to remove.go, I don't think it should abort at that point. If a stack's services have been manually removed but there are still networks sitting around, then it'll never get around to removing them.

Perhaps it should only error out if there are no resources of any kind found which match the stack name. @dnephin?

dnephin · 2016-06-28T22:10:43Z

I don't think either of these changes are correct. The current behaviour is correct.

Think of tasks like ps. If you run ps and it doesn't list any containers, that isn't an error, it's just an empty table.

For remove, I think we should consider the operation idempotent. So running remove again and having it remove nothing is fine. This is a bit different from other rm commands, but all other rm commands operate on a single object. In this case we're operating on a collection of objects. So it's more of a "remove things from this namespace". So if the namespace is empty, it's not really an error.

Ssaying remove object with id x, and that object not existing would be an error.

thaJeztah · 2016-06-30T08:12:14Z

moving back to design review pending the discussion

thaJeztah · 2016-07-01T18:13:44Z

ping @dnephin @aanand what's the status on this one; should we move this forward, or close?

aanand · 2016-07-01T18:33:46Z

I'm fine with neither condition being an error, but I think we can still try to help with typos, at least in the stack down case. We could print something like Nothing found for 'stackname' and exit 0, maybe?

allencloud · 2016-07-03T15:56:43Z

@dnephin
Thanks for your feedback. I agree with your namespace concept totally.
In a namespace, ps output nothing, then we show nothing. That is reasonable.
While, I think there is a little bit difference between two kinds of scenes. Correct me if I did the wrong assumption. Since docker ps by default has a namespace of locally the whole engine, we can treat it as default. In a default namespace, there may be no namespace mistake.
On the other hand, if there are several namespaces like stack on a single engine. First, we should specify one stack, then in the stack namespace, we do ps or list or something else. Maybe if a user typed some typos, and docker can do a little bit thing to remind our users on stack name.

allencloud · 2016-07-03T16:18:04Z

@aanand
If this PR makes sense, I have already avoided the network missing removal in stack if there is no such stack name.

dnephin · 2016-07-03T16:32:19Z

We could print something like "Nothing found for 'stackname'" and exit 0

This sounds good to me

allencloud · 2016-07-03T16:40:21Z

@dnephin
Thanks. I am thinking that way for a while and just do not make sure. I will update the PR in 12 hours, midnight here. :)

allencloud · 2016-07-04T04:04:24Z

@dnephin @aanand @thaJeztah
PR updated, PTAL.

dnephin · 2016-07-04T16:47:33Z

api/client/stack/tasks.go

+	if len(services) == 0 {
+		fmt.Fprintf(dockerCli.Out(), "Nothing found in stack: %s\n", namespace)
+		return nil
+	}


In this case I don't think we should add another API call.

We could add the warning if len(tasks) == 0, or we could just not warn.

allencloud · 2016-07-05T17:04:11Z

Updated.PATL @dnephin @aanand

icecrime · 2016-07-05T20:11:54Z

LGTM, thanks!

icecrime · 2016-07-05T20:12:32Z

Also bonus points if we can add a test ;-)

thaJeztah · 2016-07-05T20:52:00Z

LGTM, but agree that a test would be nice to prevent future regressions

tiborvass · 2016-07-05T20:55:26Z

api/client/stack/remove.go

@@ -63,6 +63,11 @@ func runRemove(dockerCli *client.DockerCli, opts removeOptions) error {
 		}
 	}

+	if len(services) == 0 && len(networks) == 0 {


@allencloud i might be missing something but what does this mean wrt a stack? As in, what's a stack with no services but only networks? just trying to make sense of this.

This is part of the stack rm command which also removes the networks, so if a network existed this command wouldn't be a no-op, hence no warning. I believe that's the idea.

Yeah. If a stack was brought up with both networks and services, then the services were all manually deleted, you'd expect stack rm to still remove the networks.

vdemeester · 2016-07-06T09:51:42Z

LGTM 🐯

allencloud · 2016-07-06T09:54:50Z

I agree with adding test. I will ping you guys once added. Thank all of you.

tiborvass · 2016-07-06T17:28:21Z

LGTM

thaJeztah · 2016-07-06T20:30:21Z

Looks like tests are failing:

https://jenkins.dockerproject.org/job/Docker-PRs/29613/console

8:28:42 
18:28:42 ----------------------------------------------------------------------
18:28:42 FAIL: docker_cli_stack_test.go:10: DockerSwarmSuite.TestStackRemove
18:28:42 
18:28:42 [d7838666] waiting for daemon to start
18:28:42 [d7838666] daemon started
18:28:42 docker_cli_stack_test.go:16:
18:28:42     c.Assert(err, checker.IsNil)
18:28:42 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc82030ece0), Stderr:[]uint8(nil)} ("exit status 1")
18:28:42 
18:28:42 [d7838666] exiting daemon
18:28:45 
18:28:45 ----------------------------------------------------------------------
18:28:45 FAIL: docker_cli_stack_test.go:20: DockerSwarmSuite.TestStackTasks
18:28:45 
18:28:45 [d28599362] waiting for daemon to start
18:28:45 [d28599362] daemon started
18:28:45 docker_cli_stack_test.go:26:
18:28:45     c.Assert(err, checker.IsNil)
18:28:45 ... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc8203f8800), Stderr:[]uint8(nil)} ("exit status 1")
18:28:45 
18:28:45 [d28599362] exiting daemon
18:28:55 
18:28:55 ----------------------------------------------------------------------

and on experimental:
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/20917/console

18:02:44 
18:02:44 ----------------------------------------------------------------------
18:02:44 FAIL: docker_cli_stack_test.go:10: DockerSwarmSuite.TestStackRemove
18:02:44 
18:02:44 [d31394419] waiting for daemon to start
18:02:44 [d31394419] daemon started
18:02:44 docker_cli_stack_test.go:17:
18:02:44     c.Assert(out, check.DeepEquals, "Nothing found in stack: UNKNOWN_STACK")
18:02:44 ... obtained string = "Nothing found in stack: UNKNOWN_STACK\n"
18:02:44 ... expected string = "Nothing found in stack: UNKNOWN_STACK"
18:02:44 
18:02:44 [d31394419] exiting daemon
18:02:45 
18:02:45 ----------------------------------------------------------------------
18:02:45 FAIL: docker_cli_stack_test.go:20: DockerSwarmSuite.TestStackTasks
18:02:45 
18:02:45 [d63031334] waiting for daemon to start
18:02:45 [d63031334] daemon started
18:02:45 docker_cli_stack_test.go:27:
18:02:45     c.Assert(out, check.DeepEquals, "Nothing found in stack: UNKNOWN_STACK")
18:02:45 ... obtained string = "Nothing found in stack: UNKNOWN_STACK\n"
18:02:45 ... expected string = "Nothing found in stack: UNKNOWN_STACK"
18:02:45 
18:02:45 [d63031334] exiting daemon
18:02:54

cpuguy83 · 2016-07-08T19:36:41Z

CI still failing.

Signed-off-by: allencloud <allen.sun@daocloud.io>

allencloud · 2016-07-11T18:40:11Z

PR updated and test updated. PTAL.
ping @vdemeester @tiborvass

vdemeester · 2016-07-11T18:43:07Z

Still LGTM and it's all green, merging 😉

GordonTheTurtle added the status/0-triage label Jun 28, 2016

cpuguy83 added this to the 1.12.0 milestone Jun 28, 2016

cpuguy83 added status/2-code-review and removed status/0-triage labels Jun 28, 2016

dnephin mentioned this pull request Jun 28, 2016

[1.12-rc2] docker stack tasks and docker stack rm silently ignore invalid stack names #23727

Closed

allencloud force-pushed the return-err-when-stack-name-nonexist branch from 2613e81 to 8099b55 Compare June 29, 2016 05:35

thaJeztah added status/1-design-review and removed status/2-code-review labels Jun 30, 2016

allencloud force-pushed the return-err-when-stack-name-nonexist branch 2 times, most recently from 6627058 to 4bea57c Compare July 3, 2016 16:15

allencloud force-pushed the return-err-when-stack-name-nonexist branch from 4bea57c to b78fde5 Compare July 4, 2016 02:50

dnephin reviewed Jul 4, 2016
View reviewed changes

allencloud force-pushed the return-err-when-stack-name-nonexist branch 3 times, most recently from 49c63f6 to 17534bb Compare July 5, 2016 08:54

icecrime added status/2-code-review and removed status/1-design-review labels Jul 5, 2016

tiborvass reviewed Jul 5, 2016
View reviewed changes

allencloud force-pushed the return-err-when-stack-name-nonexist branch 4 times, most recently from f7083fa to e524014 Compare July 6, 2016 17:04

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 6, 2016

allencloud changed the title ~~return err when stack name does not exist~~ [WIP] return err when stack name does not exist Jul 7, 2016

allencloud force-pushed the return-err-when-stack-name-nonexist branch 2 times, most recently from 0255390 to 667024a Compare July 8, 2016 03:30

allencloud force-pushed the return-err-when-stack-name-nonexist branch from 667024a to b4e7b06 Compare July 11, 2016 10:54

return err when stack name does not exist

416613f

Signed-off-by: allencloud <allen.sun@daocloud.io>

allencloud force-pushed the return-err-when-stack-name-nonexist branch from b4e7b06 to 416613f Compare July 11, 2016 16:59

allencloud changed the title ~~[WIP] return err when stack name does not exist~~ output some details when stack name does not exist Jul 11, 2016

vdemeester added process/cherry-pick and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Jul 11, 2016

vdemeester merged commit 4664975 into moby:master Jul 11, 2016

allencloud deleted the return-err-when-stack-name-nonexist branch July 12, 2016 00:40

tiborvass mentioned this pull request Jul 12, 2016

[WIP] Cherry picks 1.12.0 rc4 #24467

Merged

tiborvass added process/cherry-picked and removed process/cherry-pick labels Jul 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

output some details when stack name does not exist #24051

output some details when stack name does not exist #24051

allencloud commented Jun 28, 2016 •

edited

cpuguy83 commented Jun 28, 2016

aanand commented Jun 28, 2016

aanand commented Jun 28, 2016

dnephin commented Jun 28, 2016

thaJeztah commented Jun 30, 2016

thaJeztah commented Jul 1, 2016

aanand commented Jul 1, 2016

allencloud commented Jul 3, 2016 •

edited

allencloud commented Jul 3, 2016

dnephin commented Jul 3, 2016

allencloud commented Jul 3, 2016

allencloud commented Jul 4, 2016

dnephin Jul 4, 2016

allencloud commented Jul 5, 2016

icecrime commented Jul 5, 2016

icecrime commented Jul 5, 2016

thaJeztah commented Jul 5, 2016

tiborvass Jul 5, 2016

dnephin Jul 5, 2016

aanand Jul 5, 2016

vdemeester commented Jul 6, 2016

allencloud commented Jul 6, 2016

tiborvass commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

cpuguy83 commented Jul 8, 2016

allencloud commented Jul 11, 2016

vdemeester commented Jul 11, 2016

output some details when stack name does not exist #24051

output some details when stack name does not exist #24051

Conversation

allencloud commented Jun 28, 2016 • edited

cpuguy83 commented Jun 28, 2016

aanand commented Jun 28, 2016

aanand commented Jun 28, 2016

dnephin commented Jun 28, 2016

thaJeztah commented Jun 30, 2016

thaJeztah commented Jul 1, 2016

aanand commented Jul 1, 2016

allencloud commented Jul 3, 2016 • edited

allencloud commented Jul 3, 2016

dnephin commented Jul 3, 2016

allencloud commented Jul 3, 2016

allencloud commented Jul 4, 2016

dnephin Jul 4, 2016

Choose a reason for hiding this comment

allencloud commented Jul 5, 2016

icecrime commented Jul 5, 2016

icecrime commented Jul 5, 2016

thaJeztah commented Jul 5, 2016

tiborvass Jul 5, 2016

Choose a reason for hiding this comment

dnephin Jul 5, 2016

Choose a reason for hiding this comment

aanand Jul 5, 2016

Choose a reason for hiding this comment

vdemeester commented Jul 6, 2016

allencloud commented Jul 6, 2016

tiborvass commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

cpuguy83 commented Jul 8, 2016

allencloud commented Jul 11, 2016

vdemeester commented Jul 11, 2016

allencloud commented Jun 28, 2016 •

edited

allencloud commented Jul 3, 2016 •

edited