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.

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

stack rm should accept multiple arguments #32110

Merged
merged 1 commit into from Apr 11, 2017

Conversation

@adshmh
Contributor

adshmh commented Mar 26, 2017

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

  1. 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

  1. 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

This comment has been minimized.

Show comment
Hide comment
@adshmh

adshmh Mar 26, 2017

Contributor

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?

Contributor

adshmh commented Mar 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 27, 2017

Member

@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 😉 ).

Member

vdemeester commented Mar 27, 2017

@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 😉 ).

func namespaceFromFilters(filters filters.Args) string {
label := filters.Get("label")[0]
return strings.TrimPrefix(label, convert.LabelNamespace+"=")

This comment has been minimized.

@dnephin

dnephin Mar 27, 2017

Member

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

@dnephin

dnephin Mar 27, 2017

Member

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

This comment has been minimized.

@adshmh

adshmh Mar 27, 2017

Contributor

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?

@adshmh

adshmh Mar 27, 2017

Contributor

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?

This comment has been minimized.

@dnephin

dnephin Mar 27, 2017

Member

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

@dnephin

dnephin Mar 27, 2017

Member

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

This comment has been minimized.

@adshmh

adshmh Mar 29, 2017

Contributor

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

@adshmh

adshmh Mar 29, 2017

Contributor

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

Show outdated Hide outdated cli/command/stack/client_test.go Outdated

@adshmh adshmh changed the title from stack rm should accept multiple labels to stack rm should accept multiple arguments Mar 27, 2017

@adshmh

This comment has been minimized.

Show comment
Hide comment
@adshmh

adshmh Mar 27, 2017

Contributor

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.

Contributor

adshmh commented Mar 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 28, 2017

Member

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

Member

thaJeztah commented Mar 28, 2017

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

@adshmh

This comment has been minimized.

Show comment
Hide comment
@adshmh

adshmh Mar 28, 2017

Contributor

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

Contributor

adshmh commented Mar 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 28, 2017

Member

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

Member

thaJeztah commented Mar 28, 2017

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Mar 28, 2017

@vdemeester @dnephin ptal 👍

@vdemeester

LGTM 🐯

@dnephin

LGTM

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 30, 2017

Member

/cc @thaJeztah for docs review 👼

Member

vdemeester commented Mar 30, 2017

/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

Show outdated Hide outdated cli/command/stack/remove.go Outdated
Show outdated Hide outdated cli/command/stack/remove.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 30, 2017

Member

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

Member

thaJeztah commented Mar 30, 2017

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

@adshmh adshmh referenced this pull request Mar 30, 2017

Closed

[tests] cli unit tests #31217

6 of 19 tasks complete
stack rm should accept multiple arguments
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
@adshmh

This comment has been minimized.

Show comment
Hide comment
@adshmh

adshmh Apr 5, 2017

Contributor

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

Contributor

adshmh commented Apr 5, 2017

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

@thaJeztah

LGTM, thanks!

@MHBauer

This comment has been minimized.

Show comment
Hide comment
@MHBauer

MHBauer Apr 10, 2017

Contributor

Wow, nice test, good docs update.

Contributor

MHBauer commented Apr 10, 2017

Wow, nice test, good docs update.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Apr 11, 2017

Collaborator

LGTM

Collaborator

vieux commented Apr 11, 2017

LGTM

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32546 has succeeded
Details
janky Jenkins build Docker-PRs 41154 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1329 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12535 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1399 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 11, 2017

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…
…t-multiple-labels

stack rm should accept multiple arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment