Carry #20574 : Add a --filter option to `docker search` #22369

Merged
merged 1 commit into from May 20, 2016

Projects

None yet
@vdemeester
Member
vdemeester commented Apr 27, 2016 edited

Carrying #20574 : Add a --filter option to docker search 🐙.

fixes #18980

This make the filtering happening daemon-side, like all other filtering (images, containers, networks, volumes).

Related PR in engine-api is docker/engine-api#214 👼

  • Update docs
  • Update api-docs
  • Update integration tests (?)
  • Update completion scripts

🐸


The following search parameters are allowed:

  • is-official
  • is-automated
  • has-stars

They deprecate

--automated
-s --stars

Signed-off-by: Fabrizio Soppelsa fsoppelsa@mirantis.com

@vdemeester vdemeester referenced this pull request in docker/engine-api Apr 27, 2016
Merged

Add filter support for ImageSearch #214

@vdemeester vdemeester added this to the 1.12.0 milestone Apr 27, 2016
@thaJeztah thaJeztah commented on an outdated diff Apr 29, 2016
api/client/search.go
cmd := Cli.Subcmd("search", []string{"TERM"}, Cli.DockerCommands["search"].Description, true)
noTrunc := cmd.Bool([]string{"-no-trunc"}, false, "Don't truncate output")
- automated := cmd.Bool([]string{"-automated"}, false, "Only show automated builds")
- stars := cmd.Uint([]string{"s", "-stars"}, 0, "Only displays with at least x stars")
+ cmd.Var(&flFilter, []string{"f", "-filter"}, "Filter output based on conditions provided")
+
+ // Deprecated since Docker 1.13 in favor of "--filter"
@thaJeztah
thaJeztah Apr 29, 2016 Member

s/1.13/1.12/

@thaJeztah thaJeztah commented on an outdated diff Apr 29, 2016
api/client/search.go
@@ -62,6 +83,7 @@ func (cli *DockerCli) CmdSearch(args ...string) error {
w := tabwriter.NewWriter(cli.out, 10, 1, 3, ' ', 0)
fmt.Fprintf(w, "NAME\tDESCRIPTION\tSTARS\tOFFICIAL\tAUTOMATED\n")
for _, res := range results {
+ // --automated and -s, --stars are deprecated since Docker 1.13
@thaJeztah
thaJeztah Apr 29, 2016 Member

s/1.13/1.12/

@thaJeztah thaJeztah commented on an outdated diff Apr 29, 2016
docs/deprecated.md
@@ -53,6 +53,15 @@ defining it at container creation (`POST /containers/create`).
The `docker ps --before` and `docker ps --since` options are deprecated.
Use `docker ps --filter=before=...` and `docker ps --filter=since=...` instead.
+### Docker search 'automated' and 'stars' options
+
+**Deprecated in Release: [v1.12.0](https://github.com/docker/docker/releases/tag/v1.12.0)**
+
+**Removed In Release: v1.12**
@thaJeztah
thaJeztah Apr 29, 2016 Member

Target for removal in release: v1.14

@vdemeester
Member

@thaJeztah Updated
ping @sdurrheimer @albers for completions (I tried my best but not sure I got it right 👼)

@sdurrheimer sdurrheimer and 1 other commented on an outdated diff May 1, 2016
contrib/completion/bash/docker
case "$prev" in
- --stars|-s)
+ --filter|-f)
+ COMPREPLY=( $( compgen -S = -W "has-starts is-automated is-official" -- "$cur" ) )
@sdurrheimer
sdurrheimer May 1, 2016 Contributor

Typo: has-stars

@vdemeester
vdemeester May 1, 2016 Member

oh lol yeah 😅

@sdurrheimer sdurrheimer and 3 others commented on an outdated diff May 1, 2016
contrib/completion/bash/docker
@@ -1890,15 +1890,29 @@ _docker_save() {
}
_docker_search() {
+ local key=$(__docker_map_key_of_current_option '--filter|-f')
+ case "$key" in
+ is-automated)
+ COMPREPLY=( $( compgen -W "true false" -- "${cur##*=}" ) )
+ return
+ ;;
+ is-official)
+ COMPREPLY=( $( compgen -W "true false" -- "${cur##*=}" ) )
+ return
+ ;;
@sdurrheimer
sdurrheimer May 1, 2016 Contributor

Isn't has-stars also a boolean here ?

@vdemeester
vdemeester May 1, 2016 Member

@sdurrheimer no. has-stars takes a number, and filters the list with image that have at least this number of stars 😉

@thaJeztah
thaJeztah May 1, 2016 Member

Should we change it to just "stars"? Thinking of it, "has-stars" sounds like it's a Boolean indeed

@vdemeester
vdemeester May 1, 2016 Member

@thaJeztah hum, good point !

@albers
albers May 1, 2016 Contributor

+1 for changing it to stars.

@sdurrheimer sdurrheimer and 2 others commented on an outdated diff May 1, 2016
contrib/completion/zsh/_docker
"($help)--no-trunc[Do not truncate output]" \
- "($help -s --stars)"{-s=,--stars=}"[Only display with at least X stars]:stars:(0 10 100 1000)" \
"($help -):term: " && ret=0
;;
@sdurrheimer
sdurrheimer May 1, 2016 Contributor

We need completion values here :)

(search)
    _arguments $(__docker_arguments) \
        $opts_help \
        "($help)*"{-f=,--filter=}"[Filter values]:filter:->filter-options" \
        "($help)--no-trunc[Do not truncate output]" \
        "($help -):term: " && ret=0

    case $state in
        (filter-options)
            __docker_complete_search_filters && ret=0
            ;;
    esac
    ;;
__docker_complete_ps_filters() {
    ...
}

__docker_complete_search_filters() {
    [[ $PREFIX = -* ]] && return 1
    integer ret=1
    declare -a boolean_opts opts

    boolean_opts=('true' 'false')
    opts=('is-automated' 'is-official' 'has-stars')

    if compset -P '*='; then
        case "${${words[-1]%=*}#*=}" in
            (is-automated|is-official)
                _describe -t boolean-filter-opts "filter options" boolean_opts && ret=0
                ;;
            *)
                _message 'value' && ret=0
                ;;
        esac
    else
        _describe -t filter-opts "filter options" opts -qS "=" && ret=0
    fi

    return ret
}

__docker_network_complete_ls_filters() {
    ...
}
@vdemeester
vdemeester May 1, 2016 Member

@sdurrheimer thank you very much on that !! 😻
I wasn't sure I should add them or not, and even more if I could write them 😛

By the way, I couldn't find the filter completion on docker images, is that normal ?

@sdurrheimer
sdurrheimer May 1, 2016 Contributor

Looks like a missing bits. I will check that in the next weeks.

@albers
albers May 1, 2016 Contributor

Bash completion LGTM (please update the sort order). Nice work, Vincent!

@albers albers commented on an outdated diff May 1, 2016
contrib/completion/bash/docker
@@ -1890,15 +1890,29 @@ _docker_save() {
}
_docker_search() {
+ local key=$(__docker_map_key_of_current_option '--filter|-f')
+ case "$key" in
+ is-automated)
+ COMPREPLY=( $( compgen -W "true false" -- "${cur##*=}" ) )
@albers
albers May 1, 2016 Contributor

please sort the options alphabetically

@thaJeztah thaJeztah changed the title from Carry #25074 : Add a --filter option to `docker search` to Carry #20574 : Add a --filter option to `docker search` May 7, 2016
@thaJeztah
Member

ping @vdemeester this needs a rebase now 😢

@vdemeester
Member

@thaJeztah updated 😉

@sdurrheimer sdurrheimer commented on an outdated diff May 14, 2016
contrib/completion/zsh/_docker
@@ -1123,10 +1147,15 @@ __docker_subcommand() {
(search)
_arguments $(__docker_arguments) \
$opts_help \
- "($help)--automated[Only show automated builds]" \
+ "($help)*"{-f=,--filter=}"[Filter values]:filter: " \
@sdurrheimer
sdurrheimer May 14, 2016 Contributor

Missing ->filter-options.

"($help)*"{-f=,--filter=}"[Filter values]:filter:->filter-options" \
@albers albers commented on an outdated diff May 14, 2016
contrib/completion/bash/docker
case "$prev" in
- --stars|-s)
+ --filter|-f)
+ COMPREPLY=( $( compgen -S = -W "stars is-automated is-official" -- "$cur" ) )
@albers
albers May 14, 2016 Contributor

Please sort alphabetically here.

@thaJeztah
Member

Thanks @vdemeester, finally got around to testing this. It works good, but some inconsistencies I noticed;

The validation for boolean flags isn't validating all values, and seems to pick the first "valid" one;

# produces an error
docker search --filter is-official=lkjlkj --filter is-official=blabla ubuntu

ERRO[1309] Handler for GET /v1.24/images/search returned error: Invalid filter 'is-official=[lkjlkj blabla]'

# shows only "official"
docker search --filter is-official=false --filter is-official=true ubuntu

# also shows only "official"
docker search --filter is-official=true --filter is-official=false ubuntu

# same (and doesn't complain)
docker search --filter is-official=true --filter is-official=zzz ubuntu

Whereas the "stars" filter validates all values (and picks the "highest" one);

docker search --filter stars=30 --filter stars=bla ubuntu
ERRO[2500] Handler for GET /v1.24/images/search returned error: Invalid filter 'stars=bla'

docker search --filter stars=bla --filter stars=30 ubuntu
ERRO[2500] Handler for GET /v1.24/images/search returned error: Invalid filter 'stars=bla'

docker search --filter stars=30 --filter stars=3 ubuntu
(works - highest number is used)

Not sure what the most logical choice is; should we accept multiple boolean filters? And which one should be picked?

@vdemeester
Member

Thanks @thaJeztah.
On the filters, it's actually done the same way it's done with the other filters (images & co) and comes from the engine-api code. Currently, the boolean one can hold more than one values and thus, depending on how the code using filters act, it will or will not validate it correctly.

It's the same with dangling=… on images command for example, as long as you have a dangling=true somewhere, it's considered valid 😅.

We might want/need to add a function to validate the exact value is this and it's the only value (I'm gonna take a look at it and probably open a PR on that in engine-api).

@vdemeester vdemeester referenced this pull request in docker/engine-api May 18, 2016
Merged

Add a new UniqueExactMatch method to filters #226

@vdemeester
Member

Rebased and updated it (on completions thanks @albers @sdurrheimer). Created docker/engine-api#226 for the filtering question.

@thaJeztah
Member

We might want/need to add a function to validate the exact value is this and it's the only value (I'm gonna take a look at it and probably open a PR on that in engine-api).

Yes, I think we need some utility methods for those. Basically it got my attention because for the "stars" you were using a loop, so I wondered what happened for other values 😄

@thaJeztah
Member

LGTM (code and docs)

improvements to filter validation/parsing will be done in a follow up PR

@runcom
Member
runcom commented May 18, 2016

LGTM

@thaJeztah
Member

ping @SvenDowideit @MHBauer could you review the docs?

@albers
Contributor
albers commented May 19, 2016

bash completion LGTM. Thanks, Vincent!

@sdurrheimer
Contributor

zsh completion LGTM 👍

@SvenDowideit
Collaborator

Docs LGTM!

@thaJeztah
Member

oh! looks like we have a genuine failure on WindowsTP5;

https://jenkins.dockerproject.org/job/Docker-PRs-WoW-TP5/1799/console

23:53:07 ----------------------------------------------------------------------
23:53:07 FAIL: docker_cli_search_test.go:46: DockerSuite.TestSearchCmdOptions
23:53:07 
23:53:07 docker_cli_search_test.go:71:
23:53:07     c.Assert(outSearchCmdOfficialSlice, checker.HasLen, 3) // 1 header, 1 line, 1 carriage return
23:53:07 ... obtained []string = []string{"NAME                                   DESCRIPTION                                     STARS     OFFICIAL   AUTOMATED", "microsoft/dotnet35:windowsservercore   .NET 3.5 Runtime installed in a Windows Se...   1         [OK]       [OK]", "microsoft/iis:windowsservercore        Internet Information Services (IIS) instal...   1         [OK]       [OK]", ""}
23:53:07 ... n int = 3
23:53:07 
23:53:07 
@vdemeester
Member

/ping @jhowardmsft 👼
There has probably been some changes on the hub when asked from a windows daemon ?

@MHBauer MHBauer and 1 other commented on an outdated diff May 19, 2016
docs/reference/api/docker_remote_api.md
@@ -116,6 +116,7 @@ This section lists each version from latest to oldest. Each listing includes a
* `GET /info` now returns `SecurityOptions` field, showing if `apparmor`, `seccomp`, or `selinux` is supported.
* `GET /networks` now supports filtering by `label` and `driver`.
* `POST /containers/create` now takes `MaximumIOps` and `MaximumIOBps` fields. Windows daemon only.
+* `GET /images/search` now takes a `filters` query parameter.
@MHBauer
MHBauer May 19, 2016 Member

maybe link to the deprecated part here as well? As filters is new, and the other stuff is on the way out.

@vdemeester
vdemeester May 20, 2016 Member

API wise it does not deprecate anything really. Before, the filtering for --stars and --automated were done client side.

@MHBauer MHBauer and 4 others commented on an outdated diff May 19, 2016
docs/reference/commandline/search.md
-This example displays images with a name containing 'busybox',
-at least 3 stars and the description isn't truncated in the output:
+ $ docker search --filter "is-automated=true stars=3" busybox
@MHBauer
MHBauer May 19, 2016 Member

Is this how it works? you can combine multiple K-V pairs in one arg? Up above it says to use multiple instances of the --filter flag.

@MHBauer
MHBauer May 19, 2016 Member

I do not see any of the tests like it. If it is true, I would like to have a test.

@SvenDowideit
SvenDowideit May 20, 2016 Collaborator

oh, good catch! i thought you needed to use multiple --filter args, but this is an interesting approach.

@thaJeztah
thaJeztah May 20, 2016 Member

good catch indeed. Even though this may work, I don't think we ever documented it like this, perhaps we should change it to multiple --filter args in the example? Or do we want to support this notation?

@albers
albers May 20, 2016 Contributor

Do other commands that have a --filter also support this?

@vdemeester
vdemeester May 20, 2016 Member

oh weird I thought I updated the syntax everywhere.. definitely we should keepuse muktiple --filter notation to be consistent. i'll update.

@vdemeester
vdemeester May 20, 2016 Member

@albers nop, no --filter flags supports that, it's a typo in the original PR 😝

@MHBauer
Member
MHBauer commented May 19, 2016

Left some comments, one needs to be addressed.

@jhowardmsft
Contributor

@vdemeester I haven't been keeping up to date on the hub stuff - @jstarks is more informed here. But AFAIK, search on Windows just returns a static list regardless of input currently.

@vdemeester
Member

@jhowardmsft thanks ! I'll disable this test for windows for now and will make a followup with unit tests and a windows one 👼

@fsoppelsa @vdemeester fsoppelsa Add a --filter option to `docker search`
The filtering is made server-side, and the following filters are
supported:

* is-official (boolean)
* is-automated (boolean)
* has-stars (integer)

Signed-off-by: Fabrizio Soppelsa <fsoppelsa@mirantis.com>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
e009ebd
@thaJeztah
Member

re-LGTM the docs are fixed now. Moving to "merge" (if it's green)

@estesp estesp merged commit 642b7b1 into docker:master May 20, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 18695 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 5552 has succeeded
Details
janky Jenkins build Docker-PRs 27519 has succeeded
Details
userns Jenkins build Docker-PRs-userns 9702 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 26063 has succeeded
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 1822 has succeeded
Details
@thaJeztah
Member

Wooooot!

@vdemeester vdemeester deleted the vdemeester:carry-pr-25074 branch May 20, 2016
@jimmyxian jimmyxian referenced this pull request in docker/swarm Jun 6, 2016
Open

Keep track of the missing API in Swarm #2333

4 of 23 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment