stack rm should accept multiple arguments by adshmh · Pull Request #32110 · moby/moby

adshmh · 2017-03-26T06:57:11Z

stack rm should accept multiple arguments (addresses item 2 from #30977).

Signed-off-by: Arash Deshmeh adeshmeh@ca.ibm.com

- What I did
Added the ability to supply multiple stack labels to the 'docker stack rm' command.

- How I did it
A loop is added to the stack rm code to go through the list of stacks specified as input. Unit tests were also added to cover the stack rm code.

- How to verify it

Deploy multiple stacks, e.g.:

$ docker stack deploy -c ./stack.yml stack1
Creating network stack1_frontend
Creating network stack1_default
Creating service stack1_redis
Creating service stack1_lb
Creating service stack1_web

$ docker stack deploy -c ./stack.yml stack2
Creating network stack2_default
Creating network stack2_frontend
Creating service stack2_redis
Creating service stack2_lb
Creating service stack2_web

Remove the stacks with a single command:

$ docker stack remove stack1 stack2
Removing service stack1_redis
Removing service stack1_web
Removing service stack1_lb
Removing network stack1_default
Removing network stack1_frontend
Removing service stack2_web
Removing service stack2_redis
Removing service stack2_lb
Removing network stack2_frontend
Removing network stack2_default

- Description for the changelog
'docker stack rm' command now accepts multiple stack labels as input.

- A picture of a cute animal (not mandatory but encouraged)

vdemeester

Design LGTM, just wonder if we should do the same as container rm or not ? (i.e. print errors at the end, but try to delete all namespace — if the first one fail, we keep the error, and return it at the end, but we still try to remove the 2nd one).

stack rm should accept multiple labels

stack rm should accept multiple arguments or stack rm should accept multiple namespaces. Label is not the right term here I feel.

/cc @thaJeztah @dnephin

adshmh · 2017-03-26T23:24:52Z

If the design is acceptable, would it be a good idea if I submit separate PRs to add unit tests that cover more of the stack package?

vdemeester · 2017-03-27T07:37:27Z

@adshmh ❤️ for more unit tests. I don't think there is too much need to create a separate PR, could just be in another commit, but it's not a strong opinion from my part (so feel free to do what you feel makes more sence 😉 ).

dnephin · 2017-03-27T18:26:13Z

cli/command/stack/client_test.go

This should probably use convert.Namespace().Descope() in cli/compose/convert/compose.go

Thank you for the review. Since getServices() adds convert.LabelNamespace to the filters, I need to trim it and then use Namespace().Descope() in belongToNamespace(). Would this be acceptable?

Oh, you're right. Descope doesn't do what you want. It's good as-is.

Thanks. I left it as-is in the PR update.

dnephin · 2017-03-27T18:28:44Z

cli/command/stack/client_test.go

The ID string is not going to be same as the name. To avoid tests that pass incorrectly maybe we should append pr prepend some other string (ID-) so that if we differentiate between the ID and the name.

adshmh · 2017-03-27T22:06:31Z

Design LGTM, just wonder if we should do the same as container rm or not ? (i.e. print errors at the end, but try to delete all namespace — if the first one fail, we keep the error, and return it at the end, but we still try to remove the 2nd one).

Thanks, I will change the PR so it attempts to remove all specified namespaces.

thaJeztah · 2017-03-28T10:56:31Z

Oh, looks like this needs a rebase now @adshmh 😢

adshmh · 2017-03-28T20:18:05Z

The Windows build error does not seem to be due to the changes made by the PR:

16:08:37 re-exec error: exit status 1: output: time="2017-03-28T16:08:30Z" level=error msg="hcsshim::ImportLayer failed in Win32: There is not enough space on the disk. (0x70) layerId=\\?\D:\control\windowsfilter\93ab77eb1901ff76798674e483a2eef0930a5c5c1f0f1ecfc8e36a15ea5a38a8 flavour=1 folder=d:\temp\hcs746907439"
16:08:37 hcsshim::ImportLayer failed in Win32: There is not enough space on the disk. (0x70) layerId=\?\D:\control\windowsfilter\93ab77eb1901ff76798674e483a2eef0930a5c5c1f0f1ecfc8e36a15ea5a38a8 flavour=1 folder=d:\temp\hcs746907439
16:08:37
16:08:37
16:08:37 ERROR: Failed 'ERROR: Failed to build image from Dockerfile.windows' at 03/28/2017 16:08:37

thaJeztah · 2017-03-28T20:19:59Z

Yup, that's not related, looks like a node was out of disk space. I restarted the build 👍

thaJeztah · 2017-03-28T20:20:47Z

@vdemeester @dnephin ptal 👍

vdemeester

LGTM 🐯

dnephin

LGTM

vdemeester · 2017-03-30T14:05:15Z

/cc @thaJeztah for docs review 👼

thaJeztah

Thanks! I left some comments inline; it would also be good to have one or two examples added to the CLI docs; https://github.com/docker/docker/blob/master/docs/reference/commandline/stack_rm.md

The examples section should have its own header, you can use the rm.md page for inspiration 😄 https://github.com/docker/docker/blob/master/docs/reference/commandline/rm.md#examples

thaJeztah · 2017-03-30T14:21:44Z

cli/command/stack/remove.go

can you update the usage description to;

Use: "rm STACK [STACK...]",

Thank you for the review. I will update the PR.

thaJeztah · 2017-03-30T14:23:00Z

cli/command/stack/remove.go

Can you change this to;

Short: "Remove one or more stacks",

to be consistent with other commands that accept multiple options

Thank you. I will change that line.
Perhaps I need to create a PR on docker/docker.github.io as well, after the changes to the CLI docs are approved?

the CLI reference documentation on docs.docker.com is generated from this output, in combination with the markdown in this repo (#32110 (review)), so perhaps no changes are needed, but you could check if there's other markdown files (here, and in the docs repo) that include the USAGE output

thaJeztah · 2017-03-30T14:28:52Z

Also ping @albers @sdurrheimer (this may need an update to the completion scripts)

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

adshmh · 2017-04-05T14:26:26Z

I have updated the PR with changes to the stack rm command's usage message and the CLI docs.

thaJeztah

LGTM, thanks!

MHBauer · 2017-04-10T22:40:02Z

Wow, nice test, good docs update.

vieux · 2017-04-11T01:19:57Z

LGTM

…t-multiple-labels stack rm should accept multiple arguments

GordonTheTurtle added the status/0-triage label Mar 26, 2017

vdemeester added rebuild/powerpc status/1-design-review and removed status/0-triage labels Mar 26, 2017

GordonTheTurtle removed the rebuild/powerpc label Mar 26, 2017

vdemeester reviewed Mar 26, 2017

View reviewed changes

dnephin reviewed Mar 27, 2017

View reviewed changes

dnephin added status/2-code-review and removed status/1-design-review labels Mar 27, 2017

adshmh changed the title ~~stack rm should accept multiple labels~~ stack rm should accept multiple arguments Mar 27, 2017

adshmh force-pushed the 30977-stack-rm-should-accept-multiple-labels branch 2 times, most recently from fdceedd to 9ba614f Compare March 28, 2017 06:49

adshmh force-pushed the 30977-stack-rm-should-accept-multiple-labels branch from ad2cd5e to 8bf1f4e Compare March 28, 2017 16:07

thaJeztah added the rebuild/windowsRS1 label Mar 28, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Mar 28, 2017

vdemeester approved these changes Mar 29, 2017

View reviewed changes

dnephin added the rebuild/windowsRS1 label Mar 29, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Mar 29, 2017

dnephin approved these changes Mar 29, 2017

View reviewed changes

vdemeester added status/3-docs-review and removed status/2-code-review labels Mar 30, 2017

thaJeztah requested changes Mar 30, 2017

View reviewed changes

thaJeztah added impact/changelog impact/cli impact/documentation labels Mar 30, 2017

adshmh mentioned this pull request Mar 30, 2017

[tests] cli unit tests #31217

Closed

19 tasks

stack rm should accept multiple arguments

ff0899a

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

adshmh force-pushed the 30977-stack-rm-should-accept-multiple-labels branch from 8bf1f4e to ff0899a Compare April 5, 2017 05:14

GordonTheTurtle assigned MHBauer Apr 10, 2017

thaJeztah approved these changes Apr 10, 2017

View reviewed changes

thaJeztah added the rebuild/windowsRS1 label Apr 10, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Apr 10, 2017

thaJeztah added status/4-merge and removed status/3-docs-review labels Apr 10, 2017

vieux merged commit 3a9572c into moby:master Apr 11, 2017

GordonTheTurtle added this to the 17.05.0 milestone Apr 11, 2017

albers mentioned this pull request Apr 11, 2017

Bash completion supports multiple stacks in stack rm #32514

Merged

sylvainmouquet mentioned this pull request Apr 13, 2017

Docker stack improvements #30977

Closed

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#32110 from adshmh/30977-stack-rm-should-accep…

988d885

…t-multiple-labels stack rm should accept multiple arguments

Conversation

adshmh commented Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

adshmh commented Mar 26, 2017

Uh oh!

vdemeester commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adshmh commented Mar 27, 2017

Uh oh!

thaJeztah commented Mar 28, 2017

Uh oh!

adshmh commented Mar 28, 2017

Uh oh!

thaJeztah commented Mar 28, 2017

Uh oh!

thaJeztah commented Mar 28, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Mar 30, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 30, 2017

Uh oh!

adshmh commented Apr 5, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

MHBauer commented Apr 10, 2017

Uh oh!

vieux commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adshmh commented Mar 26, 2017 •

edited

Loading