Skip to content

cli: fix images filter when use multi reference filter#38171

Merged
thaJeztah merged 1 commit intomoby:masterfrom
ZYecho:fix-multi-images-filter
Nov 23, 2018
Merged

cli: fix images filter when use multi reference filter#38171
thaJeztah merged 1 commit intomoby:masterfrom
ZYecho:fix-multi-images-filter

Conversation

@ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Nov 10, 2018

Signed-off-by: zhangyue [email protected]

- What I did
fix #38164 , to refuse docker images -f undefined behavior when use multi reference filter.

- How I did it
just add a judge statement when filter image ref.

- How to verify it
run docker images

REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
hello-world         latest              4ab4c602aa5e        2 months ago        1.84kB
img2                test1               4ab4c602aa5e        2 months ago        1.84kB
img                 test1               4ab4c602aa5e        2 months ago        1.84kB
img                 test2               4ab4c602aa5e        2 months ago        1.84kB

run docker images -f reference=img:* -f reference=hello-*

REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
hello-world         latest              4ab4c602aa5e        2 months ago        1.84kB
img                 test1               4ab4c602aa5e        2 months ago        1.84kB
img                 test2               4ab4c602aa5e        2 months ago        1.84kB

run docker images -f reference=img:* -f reference=hello-* -f reference=img2:*

REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
hello-world         latest              4ab4c602aa5e        2 months ago        1.84kB
img2                test1               4ab4c602aa5e        2 months ago        1.84kB
img                 test1               4ab4c602aa5e        2 months ago        1.84kB
img                 test2               4ab4c602aa5e        2 months ago        1.84kB

- Description for the changelog

fix #38164 , to refuse docker images -f undefined behavior when use multi reference filter.

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

@ZYecho
Copy link
Contributor Author

ZYecho commented Nov 20, 2018

@yongtang @thaJeztah PTAL, thanks.

@thaJeztah
Copy link
Member

thaJeztah commented Nov 20, 2018

Relates to #28477, which implemented this filter

This looks to be consistent with docker ps / docker container ls and docker service ls, which treat multiple --filter name=... as an or (not an and);

Containers

docker run -dit --name foobar busybox
docker run -dit --name something busybox

docker container ls --filter name=foobar
CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS              PORTS               NAMES
81ada6c88e9f        busybox             "sh"                     26 seconds ago      Up 25 seconds                           foobar

docker container ls --filter name=foobar --filter name=something 
CONTAINER ID        IMAGE               COMMAND                  CREATED              STATUS              PORTS               NAMES
d05e62da1ba4        busybox             "sh"                     51 seconds ago       Up 50 seconds                           something
81ada6c88e9f        busybox             "sh"                     52 seconds ago       Up 51 seconds                           foobar

Services

docker service create --name foobar nginx:alpine
docker service create --name something nginx:alpine

docker service ls --filter name=foobar
ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
so8i1x1sjdl1        foobar              replicated          1/1                 nginx:alpine        

docker service ls --filter name=foobar --filter name=something
ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
so8i1x1sjdl1        foobar              replicated          1/1                 nginx:alpine        
gcaogsysqvjh        something           replicated          1/1                 nginx:alpine

Some things to change;

I'd like some more eyes on this change though, because it's a change in behavior, so there's a risk that this breaks people 😓

@thaJeztah
Copy link
Member

thaJeztah commented Nov 20, 2018

ping @vdemeester PTAL

@ZYecho ZYecho force-pushed the fix-multi-images-filter branch from b853106 to 75acddc Compare November 21, 2018 02:22
@ZYecho ZYecho requested a review from vdemeester as a code owner November 21, 2018 02:22
@codecov
Copy link

codecov bot commented Nov 21, 2018 •
edited
Loading

Codecov Report

Merging #38171 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #38171      +/-   ##
==========================================
+ Coverage    36.1%   36.11%   +<.01%     
==========================================
  Files         610      610              
  Lines       45234    45236       +2     
==========================================
+ Hits        16333    16337       +4     
  Misses      26661    26661              
+ Partials     2240     2238       -2

@ZYecho ZYecho force-pushed the fix-multi-images-filter branch 4 times, most recently from 7c88ce1 to d454d5d Compare November 21, 2018 02:55
vdemeester
vdemeester suggested changes Nov 21, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like some more eyes on this change though, because it's a change in behavior, so there's a risk that this breaks people 😓

That's my concern too, but I think it should be ok — having multiple --filter=reference=* makes it pretty useless to have multiple reference filter, so I don't think too many people uses that.

@ZYecho ZYecho force-pushed the fix-multi-images-filter branch from d454d5d to 908acc8 Compare November 22, 2018 01:57
@ZYecho ZYecho force-pushed the fix-multi-images-filter branch from 908acc8 to 5007c36 Compare November 22, 2018 02:34
vdemeester
vdemeester approved these changes Nov 22, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐯

@ZYecho
Copy link
Contributor Author

ZYecho commented Nov 23, 2018

@thaJeztah Hi, what's the next step before merge this pr?

thaJeztah
thaJeztah approved these changes Nov 23, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no other steps needed, other than a second maintainer reviewing the last code changes (the updated tests) 😄

And, those look good to me (thanks!)

LGTM

@thaJeztah
Copy link
Member

thaJeztah commented Nov 23, 2018

Experimental failure is a known flaky test (see #32673);

03:34:29 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection

Ignoring that failiure

@thaJeztah thaJeztah merged commit 618741b into moby:master Nov 23, 2018
@thaJeztah
Copy link
Member

thaJeztah commented Nov 23, 2018

Thanks! And congratulations with your first contribution merged 🎉

@ZYecho ZYecho deleted the fix-multi-images-filter branch November 23, 2018 10:38
@ZYecho ZYecho mentioned this pull request Dec 5, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request Apr 17, 2019
Merged
@ZYecho ZYecho mentioned this pull request May 5, 2019
Open
@corhere corhere mentioned this pull request Aug 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

impact/changelog status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: docker images filter undefined behavior when use multi reference filter

4 participants

@ZYecho @thaJeztah @vdemeester @GordonTheTurtle