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

Add network label filter support #21495

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
6 participants
@HackToday
Contributor

HackToday commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 25, 2016

Contributor

The jenknis failed because of the issue #21490

Contributor

HackToday commented Mar 25, 2016

The jenknis failed because of the issue #21490

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 25, 2016

Contributor

ping @vdemeester @calavera and @thaJeztah

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

Contributor

HackToday commented Mar 25, 2016

ping @vdemeester @calavera and @thaJeztah

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

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 25, 2016

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 😉

Member

vdemeester commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@HackToday
Contributor

HackToday commented Mar 28, 2016

Show outdated Hide outdated api/server/router/network/filter.go
if filter.Include("type") {
var typeNet []libnetwork.Network
errFilter := filter.WalkValues("type", func(fval string) error {

This comment has been minimized.

@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

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

This comment has been minimized.

@thaJeztah

thaJeztah Mar 28, 2016

Member

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

@thaJeztah

thaJeztah Mar 28, 2016

Member

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

This comment has been minimized.

@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

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

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 30, 2016

Contributor

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

Contributor

HackToday commented Mar 30, 2016

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

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Apr 9, 2016

Contributor

Do we have any plan for this PR ?

Contributor

HackToday commented Apr 9, 2016

Do we have any plan for this PR ?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 9, 2016

Member

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

ping @calavera @vdemeester PTAL

Member

thaJeztah commented Apr 9, 2016

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

ping @calavera @vdemeester PTAL

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 11, 2016

Member

Design and code LGTM for me

Member

vdemeester commented Apr 11, 2016

Design and code LGTM for me

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Apr 13, 2016

Contributor

ping @calavera PTAL. Thanks

Contributor

HackToday commented Apr 13, 2016

ping @calavera PTAL. Thanks

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Apr 13, 2016

Contributor

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

Contributor

calavera commented Apr 13, 2016

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

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Apr 14, 2016

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

Contributor

HackToday commented Apr 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 14, 2016

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

Member

thaJeztah commented Apr 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Apr 14, 2016

Contributor

Thanks @thaJeztah I misunderstood doc-revisit

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

Contributor

HackToday commented Apr 14, 2016

Thanks @thaJeztah I misunderstood doc-revisit

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 14, 2016

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

Member

thaJeztah commented Apr 14, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@albers

albers Apr 14, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Apr 15, 2016

Contributor

ping @thaJeztah PTAL Thanks

Contributor

HackToday commented Apr 15, 2016

ping @thaJeztah PTAL Thanks

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 15, 2016

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

Member

thaJeztah commented Apr 15, 2016

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

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>
@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Apr 18, 2016

Contributor

ping @thaJeztah PTAL the docs change. Thanks

Contributor

HackToday commented Apr 18, 2016

ping @thaJeztah PTAL the docs change. Thanks

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 18, 2016

Member

retest this please

Member

thaJeztah commented Apr 18, 2016

retest this please

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 18, 2016

Member

docs LGTM, thanks!

ping @vdemeester @MHBauer PTAL

Member

thaJeztah commented Apr 18, 2016

docs LGTM, thanks!

ping @vdemeester @MHBauer PTAL

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 18, 2016

Member

LGTM 👍

Member

vdemeester commented Apr 18, 2016

LGTM 👍

@vdemeester vdemeester merged commit 75cc2c9 into moby: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

This comment has been minimized.

Show comment
Hide comment
@albers

albers May 13, 2016

Member

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

Member

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