Add network label filter support #21495

Merged
merged 1 commit into from Apr 19, 2016

Projects

None yet

6 participants

@HackToday
Contributor

This patch did following:

  1. Make filter check logic same as docker ps filters

Right now docker container logic work as following:
when same filter used like below:
-f name=jack -f name=tom
it would get all containers name is jack or tom(it is or logic)

when different filter used like below:

-f name=jack -f id=7d1
it would get all containers name is jack and id contains 7d1(it is and logic)

It would make sense in many user cases, but it did lack of compliate filter cases,
like "I want to get containers name is jack or id=7d1", it could work around use
(get id=7d1 containers' name and get name=jack containers, and then construct the
final containers, they could be done in user side use shell or rest API)

  1. Fix one network filter bug which could include duplicate result
    when use -f name= -f id=, it would get duplicate results

  2. Make id filter same as container id filter, which means match any string.
    not use prefix match.

It is for consistent match logic

Closes: #21417

Signed-off-by: Kai Qiang Wu(Kennan) wkqwu@cn.ibm.com

@HackToday
Contributor

The jenknis failed because of the issue #21490

@HackToday
Contributor

ping @vdemeester @calavera and @thaJeztah

This PR not include doc change, as I found #21270 still have doc-revisit. :0

@vdemeester
Member

Although it changes the way filters worked on network before (It's now an and instead of an or), it's coherent with how filters work on other command so, design LGTM 😉

@HackToday
Contributor
@thaJeztah thaJeztah and 1 other commented on an outdated diff Mar 28, 2016
api/server/router/network/filter.go
+ if filter.Include("id") {
+ if !filter.Match("id", nw.ID()) {
+ continue
+ }
+ }
+ if filter.Include("label") {
+ if !filter.MatchKVList("label", nw.Info().Labels()) {
+ continue
+ }
+ }
+ displayNet = append(displayNet, nw)
+ }
+
+ if filter.Include("type") {
+ var typeNet []libnetwork.Network
+ errFilter := filter.WalkValues("type", func(fval string) error {
@thaJeztah
thaJeztah Mar 28, 2016 Member

Is there a reason for using two different approaches now for filtering? i.e. the loop above, and the filter.WalkValues() approach here?

@thaJeztah
thaJeztah Mar 28, 2016 Member

oh, is that to implement the "and" behavior instead of "or"?

@HackToday
HackToday Mar 29, 2016 Contributor

hi @thaJeztah The reason we use that, is

  1. filterNetworkByType is specific logic in network, general filter(engine-api/types/filters/parse.go) not include that, and we'd better not put such logic in general filters

  2. we directly use filter.WalkValues, which I can reduce duplicate filter logic, if we want to above loop ways, we need write another function, which fetch filter "type" values, and validate it and then return bool etc. I think is not good idea.

Please let me know what you think. Thanks

@HackToday
Contributor

PTAL the new change, Thanks
ping @calavera and @thaJeztah

@HackToday
Contributor

Do we have any plan for this PR ?

@thaJeztah
Member

Sorry about that, it's been busy for the release candidates and @calavera had a few days off.

ping @calavera @vdemeester PTAL

@vdemeester
Member

Design and code LGTM for me

@HackToday
Contributor

ping @calavera PTAL. Thanks

@calavera
Contributor

LGTM. This will need to be added to the documentation.

@HackToday
Contributor

Since #21270 still have doc-revisit , so not sure if it is good start from this patch, as this patch doc depend on that #21270 doc changes

@thaJeztah
Member

Docs for #21270 were added in #21522 (the "revisit" is to check if additional changes are still needed). But this feature is separate from that PR, so if you can update;

This may also need changes to the bash and zsh completion scripts, so ping @albers, @sdurrheimer

@HackToday
Contributor

Thanks @thaJeztah I misunderstood doc-revisit

Will add doc changes, do we need separate PR or still in this PR ?

@thaJeztah
Member

@HackToday please as part of this PR, and (given that the code changes are not that large), I think it can be in the same commit

@albers
Contributor
albers commented Apr 14, 2016

I'll take care of the bash completion part (if noone objects) after this is merged. Thanks for the ping, @thaJeztah

@HackToday
Contributor

ping @thaJeztah PTAL Thanks

@thaJeztah
Member

Looking good!! I just realized we should also document that the API accepts a new "filter" type for the /networks endpoint. Can you add a note under https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v124-api-changes, something like;

GET /networks now supports filtering by label.

Also, can you add the new filter to the list of accepted filters here; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.24.md#list-networks

@thaJeztah thaJeztah added this to the 1.12.0 milestone Apr 15, 2016
Kai Qiang Wu(Kennan) Add network label filter support
This patch did following:

1) Make filter check logic same as `docker ps ` filters

Right now docker container logic work as following:
when same filter used like below:
 -f name=jack -f name=tom
it would get all containers name is jack or tom(it is or logic)

when different filter used like below:

 -f name=jack -f id=7d1
it would get all containers name is jack and id contains 7d1(it is and logic)

It would make sense in many user cases, but it did lack of compliate filter cases,
like "I want to get containers name is jack or id=7d1", it could work around use
(get id=7d1 containers' name and get name=jack containers, and then construct the
final containers, they could be done in user side use shell or rest API)

2) Fix one network filter bug which could include duplicate result
when use -f name=  -f id=, it would get duplicate results

3) Make id filter same as container id filter, which means match any string.
not use prefix match.

It is for consistent match logic

Closes: #21417

Signed-off-by: Kai Qiang Wu(Kennan) <wkqwu@cn.ibm.com>
f812b55
@HackToday
Contributor

ping @thaJeztah PTAL the docs change. Thanks

@thaJeztah
Member

retest this please

@thaJeztah
Member

docs LGTM, thanks!

ping @vdemeester @MHBauer PTAL

@vdemeester
Member

LGTM 👍

@vdemeester vdemeester merged commit 75cc2c9 into docker:master Apr 19, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 17691 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 4530 has succeeded
Details
janky Jenkins build Docker-PRs 26520 has succeeded
Details
userns Jenkins build Docker-PRs-userns 8730 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 24969 has succeeded
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 732 has succeeded
Details
@HackToday HackToday deleted the HackToday:addnetworkfilter branch Apr 25, 2016
@albers
Contributor
albers commented May 13, 2016

note: bash completion support for this was added by @thaJeztah in #22319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment