Add filter for `network ls` to hide predefined network #17782

WeiZhang555 · 2015-11-07T08:37:38Z

Add option --no-predefined for network ls to hide predefined network.
If will be extremely useful when user want to run "docker network rm docker network ls -q --no-predefined" to delete a bundle of networks, without this change, this deletion will always return non-zero exit code which usually indicates error happens because predefined network cant be removed.

Signed-off-by: Zhang Wei zhangwei555@huawei.com

Docs are still on working, will be posted after some time.

thaJeztah · 2015-11-07T08:41:16Z

I understand the use case, but to be in line with other "lists", I'd prefer this to be implemented as a filter, I.e. --filter something=...

haven't thought about what to call the filter :-)

icecrime · 2015-11-07T10:10:58Z

I understand the use case, but flags, flags, more flags, ... I don't think it's worth it =/ Sorry I'm leaning toward -1.

vdemeester · 2015-11-07T10:37:28Z

I do agree with @icecrime on adding another flag, -1 on that — but I understand the use case and I like the idea of the --filter flags as it's already present in other listing commands (images, ps) so it does feel more consistent (and extensible 😝).

The name of the filter could be type and values would be built-in, etc..

icecrime · 2015-11-07T11:23:27Z

@WeiZhang555 Would you be willing to update to use a --filter=... syntax? Thanks!

WeiZhang555 · 2015-11-07T17:26:55Z

Yeah, I can update it with a --filter=..., but before doing that, I want to say something more for myself.

Actually I have considered these two very different implementations, I mean one more option vs --filter, but as docker network ls gives only three fields: container id, name and driver, it shows very limited information for us to manipulate, --filter is powerful of course, but I don't think it needs so powerful an option.

What's more is that comparing to docker image, we have docker rmi 'docker images -aq', you see that it's quite short and easy to type, I prefer to have one option to let me run docker network rm 'docker network ls -u' (-u can stand for --user-defined, my original --no-predefined is too long and I'm not satisfied with it either.). The key point is that I think it's worth a short flag such as (docker network rm 'docker network ls -u') instead of a long format (docker network rm 'docker network ls --filter=built-in').

As for consistency, (docker rm 'docker ps -aq') is also a short command similar to (docker rmi 'docker images -aq')

However, I won't argue if you all think --filter is better, so do you still insist on option of --filter ? 😄
Whatever thank you for your kind suggestions! @thaJeztah @icecrime @vdemeester

WeiZhang555 · 2015-11-09T07:21:37Z

I just renamed original option --no-predefined to -u/--user-defined, so user can run docker network rm 'docker network ls -qu' to delete a bundle of networks.
This is more friendly than something like docker network rm 'docker network ls -q --filter type=custom' for lazy person like me 😛

But I'm still waiting for your opinions, and I won't mind to change is to filter format if you insist. 😄

WeiZhang555 · 2015-11-10T09:11:09Z

I just re-implemented -u option with --filter for hiding predefined network,
now user can use docker network rm 'docker network ls -f type=custom'
to delete a bundle of userdefined networks.

Currently filter support only two options:
type=custom : only show userdefined networks
type=built-in: only show built in networks

Final behavior look likes this:

$ docker network ls       (# normal network ls)
NETWORK ID          NAME                DRIVER
e94695c3f8a2        none                null
3841be13515b        host                host
e397be974220        dev                 bridge
aad8b004629c        dev1                bridge
f71a378fc0c8        bridge              bridge

$ docker network ls -f type=custom       (# only show user defined network)
NETWORK ID          NAME                DRIVER
e397be974220        dev                 bridge
aad8b004629c        dev1                bridge

$ docker network ls -f type=built-in       (# only show built-in network)
NETWORK ID          NAME                DRIVER
3841be13515b        host                host
f71a378fc0c8        bridge              bridge
e94695c3f8a2        none                null

$ docker network ls -f type=built-in -f type=custom (# compose two kinds of filter equals to normal ls)
NETWORK ID          NAME                DRIVER
f71a378fc0c8        bridge              bridge
e94695c3f8a2        none                null
3841be13515b        host                host
e397be974220        dev                 bridge
aad8b004629c        dev1                bridge

thaJeztah · 2015-11-10T09:26:53Z

Thanks @WeiZhang555! I understand that "shorter" options can be convenient, but I think we should be consistent; also --filter is more flexible for future extension (e.g. --filter driver=overlay). So thanks for implementing as a --filter 👍

design (UX) LGTM

WeiZhang555 · 2015-11-11T15:36:52Z

Docs updated.
I think It would be better to merge #17489 first before this, because in docs I said
" --filter type=custom is extremely useful when you are trying to delete a bundle of networks"
This is true only when docker network rm can remove many networks at one time, that's why #17489 must be done first. 😄

vdemeester · 2015-11-12T21:24:22Z

@WeiZhang555 The UX looks good to me but this should be implemented on the "server" side, as anything the cli does should be do-able using the API. Thus, the code in api/client/network.go should be really small (and more or less look like the filter part of api/client/ps.go) and the validation and actual filtering should be done server side.

Once this is done, I'll put this into code review 😉.

WeiZhang555 · 2015-11-13T03:01:28Z

@vdemeester OK, I'll move the filter function to server side, as soon as I come back from DockerCon in Europe. 😜

thaJeztah · 2015-11-13T08:59:41Z

Nice! In the airplane now to Barcelona, so see you there! (@vdemeester will be there as well \o/)

WeiZhang555 · 2015-11-13T09:21:13Z

@thaJeztah @vdemeester Look forward to seeing you guys 😆

calavera · 2015-11-17T09:07:29Z

api/client/network.go

+
+var supportedFilters map[string]filterHandlers
+
+func init() {


you can initialize those supported filters directly when you create the variable, two lines above. This init function is not necessary.

calavera · 2015-11-17T09:09:43Z

I might be wrong, but I feel like the filter validations should be on the server side. Otherwise, people using the API won't have them.

thaJeztah · 2015-11-17T13:07:14Z

agreed with @calavera, validations should be server side

WeiZhang555 · 2015-11-23T00:26:09Z

Hello everyone, I have reimplemented this function on server-side, I think it should work fine .
Now there are two more filters:

-f type=builtin
-f type-=custom

Hmm, there are already two existing filters though not enabled, I'm not sure who are using it. They are

-f id=XXX
-f name=XXX

I don't know what it's designed for, so I just keep it there.
Tell me what do you think of this, thank you all! 😄

calavera · 2015-11-23T18:49:04Z

api/server/router/network/network_routes.go

@@ -17,6 +17,68 @@ import (
 	"github.com/docker/libnetwork"
 )

+// supportedFilters predefined some supported filter handler function
+type filterHandler func(libnetwork.Network, []string) (bool, error)


can we put all this filtering logic in a separated file, like api/server/router/network/filters.go? just to keep them separated from the routes 😄

WeiZhang555 · 2015-12-14T05:15:18Z

Rebased on the new client library @calavera

And @thaJeztah , partial name match is implemented on https://github.com/docker/docker/pull/17782/files#diff-c5b0cdb16e32c93e45e51c5131209cbaR68, with relevant test case change on https://github.com/docker/docker/pull/17782/files#diff-401699de8561309143baebae1c3696e3R289. Also docs are updated as per your suggestions.

Also ping other guys involved, please take a look. 😄

vdemeester · 2015-12-14T08:11:08Z

Still LGTM for me 😉

thaJeztah · 2015-12-14T14:19:38Z

docs/reference/commandline/network_ls.md

+$ docker network ls --filter name=foo
+NETWORK ID          NAME                DRIVER
+95e74588f40d        foo                 bridge
+```


Now that we have partial matches, can you update this to:

#### Name The `name` filter matches on all or part of a network's name. The following filter matches all networks with a name containing the `foobar` string. ```bash $ docker network ls --filter name=foobar NETWORK ID NAME DRIVER 06e7eef0a170 foobar bridge ``` You can also filter for a substring in a name as this shows: ```bash $ docker ps --filter name=foo NETWORK ID NAME DRIVER 95e74588f40d foo bridge 06e7eef0a170 foobar bridge ```

thaJeztah · 2015-12-14T14:26:36Z

@WeiZhang555 just realized; this is now done server-side? If so, we need to update the API docs as well;

In the API changelog, add something like:

`GET /networks` now supports filtering by `name`, `id` and `type`.

The GET /networks endpoint in the API docs also need a mention of the filter. Similar to "list containers"

WeiZhang555 · 2015-12-15T03:38:01Z

@thaJeztah docs updated.

And I happen to find that all the filter descriptions are outdated, e.g.

**filters** - JSON encoded value of the filters (a `map[string][]string`) to process ...

It's not a map[string][]string any more, it's a map[string]map[string]bool for now, the change is introduced by #18266. That's why I updated all filter parts, not many changes, please take a look. 😄

thaJeztah · 2015-12-15T09:28:45Z

It's not a map[string][]string any more, it's a map[string]map[string]bool for now, the change is introduced by #18266. That's why I updated all filter parts, not many changes, please take a look. 😄

ping @calavera can you check that, is that a deliberate change, because that isn't documented (and sounds like a breaking change in the API)

WeiZhang555 · 2015-12-17T01:37:59Z

ping @calavera

calavera · 2015-12-21T17:15:17Z

sounds like a breaking change in the API

the api still supports map[string][]string. We don't support it internally anymore. I'm okay with leaving the old description if it's more clear and causes less confusion.

https://github.com/calavera/docker/blob/93d1dd8036d57f5cf1e5cbbbad875ae9a6fa6180/pkg/parsers/filters/parse.go#L84-L91

thaJeztah · 2015-12-21T17:27:14Z

I'm okay with leaving the old description if it's more clear and causes less confusion.

hm, perhaps it's better to keep the old description then. not sure we want to describe that "you can either formatA or formatB"

Would you be okay changing those descriptions back @WeiZhang555?

WeiZhang555 · 2015-12-22T02:01:30Z

@thaJeztah Filter descriptions have been changed back already, please take a look. :)

thaJeztah · 2015-12-22T08:54:44Z

Thanks @WeiZhang555!

LGTM

ping @moxiegirl for a final look 👍

moxiegirl · 2015-12-22T20:41:18Z

docs/reference/api/docker_remote_api_v1.22.md

 Query Parameters:

- **filters** - JSON encoded value of the filters (a `map[string][]string`) to process on the networks list. Available filters: `name=[network-names]` , `id=[network-ids]`
+- **filters** - JSON encoded value of the filters (a `map[string][]string`) to process on the networks list. Available filters:


@WeiZhang555 I'm not sure it is necessary to mention the format if there are only limited types of filters. Also, can a user filter on multiple names or ids or only one at a time? Looks like one at a time with multiple filters allowed. So...

- **filters** - JSON encoded network list filter. The filter value is one of: - `name=<network-name>` Matches all or part of a network name. - `id=<network-id>` Matches all or part of a network id. - `type=["custom"|"bridge"|"none"|"host"]` Filters networks by type. The `custom` keyword returns all user-defined networks.

It's consistent with the way it's documented for other filters though, e.g. For list containers. Perhaps we should rewrite them all, and provide some actual examples

WeiZhang555 · 2015-12-23T05:23:32Z

@moxiegirl docs updated.

Add filter support for `network ls` to hide predefined network, then user can use "docker network rm `docker network ls -f type=custom`" to delete a bundle of userdefined networks. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

thaJeztah · 2015-12-23T12:59:11Z

Thanks @WeiZhang555, given that those changes are what @moxiegirl suggested, I take that as an implicit "LGTM" from @moxiegirl, and I think this is ready to merge!

LGTM from me as well

thanks for your contribution, @WeiZhang555!!

Add filter for `network ls` to hide predefined network

GordonTheTurtle added the status/0-triage label Nov 7, 2015

icecrime added status/1-design-review and removed status/0-triage labels Nov 7, 2015

WeiZhang555 force-pushed the network-ls-nopre branch from 33280f1 to 7d605f5 Compare November 9, 2015 07:12

WeiZhang555 force-pushed the network-ls-nopre branch from 7d605f5 to 2cdb26f Compare November 10, 2015 09:02

WeiZhang555 force-pushed the network-ls-nopre branch from 2cdb26f to 4053699 Compare November 10, 2015 09:46

WeiZhang555 changed the title ~~Add option for network ls to hide predefined network~~ Add filter for network ls to hide predefined network Nov 10, 2015

WeiZhang555 force-pushed the network-ls-nopre branch from 4053699 to 8c51a71 Compare November 11, 2015 15:30

calavera added status/2-code-review and removed status/1-design-review labels Nov 17, 2015

calavera reviewed Nov 17, 2015
View reviewed changes

WeiZhang555 force-pushed the network-ls-nopre branch from 8c51a71 to 89f5a60 Compare November 23, 2015 00:20

calavera reviewed Nov 23, 2015
View reviewed changes

WeiZhang555 force-pushed the network-ls-nopre branch from 00bc8fa to 185046d Compare December 14, 2015 04:00

thaJeztah reviewed Dec 14, 2015
View reviewed changes

thaJeztah added the impact/api label Dec 14, 2015

WeiZhang555 force-pushed the network-ls-nopre branch from 185046d to 69f574f Compare December 15, 2015 03:26

thaJeztah added this to the 1.10 milestone Dec 17, 2015

WeiZhang555 force-pushed the network-ls-nopre branch 2 times, most recently from 1943873 to b76ef54 Compare December 22, 2015 01:56

WeiZhang555 force-pushed the network-ls-nopre branch from b76ef54 to f880711 Compare December 22, 2015 06:53

moxiegirl reviewed Dec 22, 2015
View reviewed changes

WeiZhang555 force-pushed the network-ls-nopre branch from f880711 to 849ad1b Compare December 23, 2015 05:22

Add filter for network ls to hide predefined net

26dd026

Add filter support for `network ls` to hide predefined network, then user can use "docker network rm `docker network ls -f type=custom`" to delete a bundle of userdefined networks. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>

WeiZhang555 force-pushed the network-ls-nopre branch from 849ad1b to 26dd026 Compare December 23, 2015 05:29

thaJeztah added a commit that referenced this pull request Dec 23, 2015

Merge pull request #17782 from WeiZhang555/network-ls-nopre

4432a89

Add filter for `network ls` to hide predefined network

thaJeztah merged commit 4432a89 into moby:master Dec 23, 2015

WeiZhang555 deleted the network-ls-nopre branch December 24, 2015 00:33

albers mentioned this pull request Dec 27, 2015

bash completion for docker network ls --filter #18923

Merged

dongluochen mentioned this pull request Feb 2, 2016

swarm doesn't support docker network ls type filter docker-archive/classicswarm#1738

Closed

sdurrheimer mentioned this pull request Apr 29, 2016

Add zsh completion for 'docker network ls -f {id,name,type}' #22426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filter for `network ls` to hide predefined network #17782

Add filter for `network ls` to hide predefined network #17782

WeiZhang555 commented Nov 7, 2015

thaJeztah commented Nov 7, 2015

icecrime commented Nov 7, 2015

vdemeester commented Nov 7, 2015

icecrime commented Nov 7, 2015

WeiZhang555 commented Nov 7, 2015

WeiZhang555 commented Nov 9, 2015

WeiZhang555 commented Nov 10, 2015

thaJeztah commented Nov 10, 2015

WeiZhang555 commented Nov 11, 2015

vdemeester commented Nov 12, 2015

WeiZhang555 commented Nov 13, 2015

thaJeztah commented Nov 13, 2015

WeiZhang555 commented Nov 13, 2015

calavera Nov 17, 2015

calavera commented Nov 17, 2015

thaJeztah commented Nov 17, 2015

WeiZhang555 commented Nov 23, 2015

calavera Nov 23, 2015

WeiZhang555 commented Dec 14, 2015

vdemeester commented Dec 14, 2015

thaJeztah Dec 14, 2015

thaJeztah commented Dec 14, 2015

WeiZhang555 commented Dec 15, 2015

thaJeztah commented Dec 15, 2015

WeiZhang555 commented Dec 17, 2015

calavera commented Dec 21, 2015

thaJeztah commented Dec 21, 2015

WeiZhang555 commented Dec 22, 2015

thaJeztah commented Dec 22, 2015

moxiegirl Dec 22, 2015

thaJeztah Dec 22, 2015

WeiZhang555 commented Dec 23, 2015

thaJeztah commented Dec 23, 2015

Add filter for network ls to hide predefined network #17782

Add filter for network ls to hide predefined network #17782

Conversation

WeiZhang555 commented Nov 7, 2015

thaJeztah commented Nov 7, 2015

icecrime commented Nov 7, 2015

vdemeester commented Nov 7, 2015

icecrime commented Nov 7, 2015

WeiZhang555 commented Nov 7, 2015

WeiZhang555 commented Nov 9, 2015

WeiZhang555 commented Nov 10, 2015

thaJeztah commented Nov 10, 2015

WeiZhang555 commented Nov 11, 2015

vdemeester commented Nov 12, 2015

WeiZhang555 commented Nov 13, 2015

thaJeztah commented Nov 13, 2015

WeiZhang555 commented Nov 13, 2015

calavera Nov 17, 2015

Choose a reason for hiding this comment

calavera commented Nov 17, 2015

thaJeztah commented Nov 17, 2015

WeiZhang555 commented Nov 23, 2015

calavera Nov 23, 2015

Choose a reason for hiding this comment

WeiZhang555 commented Dec 14, 2015

vdemeester commented Dec 14, 2015

thaJeztah Dec 14, 2015

Choose a reason for hiding this comment

thaJeztah commented Dec 14, 2015

WeiZhang555 commented Dec 15, 2015

thaJeztah commented Dec 15, 2015

WeiZhang555 commented Dec 17, 2015

calavera commented Dec 21, 2015

thaJeztah commented Dec 21, 2015

WeiZhang555 commented Dec 22, 2015

thaJeztah commented Dec 22, 2015

moxiegirl Dec 22, 2015

Choose a reason for hiding this comment

thaJeztah Dec 22, 2015

Choose a reason for hiding this comment

WeiZhang555 commented Dec 23, 2015

thaJeztah commented Dec 23, 2015

Add filter for `network ls` to hide predefined network #17782

Add filter for `network ls` to hide predefined network #17782