🚀 Advanced: make secret ls support filters in CLI by allencloud · Pull Request #30810 · moby/moby · GitHub

allencloud · 2017-02-08T04:03:50Z

Signed-off-by: allencloud allen.sun@daocloud.io

This pr makes command docker secret ls support filter.

I found that in daemon API, we have already supported secret list filter query. While I found that in docker CLI, there is no support of this filter yet.

- What I did

rename ./secret/ls.go to ./secret/list.go;
support filter in secret ls command;
add a test case for docker secret ls --filter;
add more filter options in swagger.yml;
update command line reference of docker secret ls.

- How I did it

- How to verify it

- Description for the changelog

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

allencloud · 2017-02-08T05:49:03Z

While here I still have some confusion that the names in secret filter:https://github.com/docker/docker/blame/master/daemon/cluster/filters.go#L102
Could you give me some help, since I guess I have some issues here in dealing names filter in this PR. @ehazlett

vdemeester

/cc @thaJeztah @ehazlett @diogomonica

ehazlett · 2017-02-08T14:21:49Z

Good catch. Looks like it should be names and name_prefixes (https://github.com/docker/swarmkit/blob/4cf6f729d7387f3d6e0a5eb3664969ebf08b66fd/api/control.proto#L352). @diogomonica could tell more about how it actually filters in swarm.

diogomonica · 2017-02-08T19:22:56Z

LGTM

diogomonica · 2017-02-08T19:23:03Z

ping @aaronlehmann

aaronlehmann · 2017-02-08T19:25:00Z

Good catch. Looks like it should be names and name_prefixes

Filters for other types of objects don't seem to support name_prefixes, so maybe just name is sufficient.

allencloud · 2017-02-09T01:13:02Z

Thanks a lot for all your feedback.
Now I remove the names filter part from swagger, filter validation, and test cases.
PTAL

aaronlehmann · 2017-02-09T01:21:21Z

LGTM

cpuguy83

LGTM

AkihiroSuda · 2017-02-10T04:42:13Z

Since it was names in 1.13, I think we need to update docs to clarify that.
https://github.com/docker/docker/blob/v1.13.1/api/swagger.yaml#L7646

allencloud · 2017-02-10T06:36:45Z

I added * GET /secretsnow does not support filternames. in version-history.md. PTAL @AkihiroSuda

vdemeester · 2017-02-10T17:17:47Z

As it is supported in 1.13 release (the names filter), we can't remove it just like that. We need to do what our deprecation policy requires us to do (i.e. mark it as deprecated for next release, and will be remove in 3 more releases)
/cc @thaJeztah

:( figured this was added after 1.13

Yeah API 1.25 and above support this 😓

vdemeester · 2017-02-10T17:19:04Z

This should be a unit test I think 👼

vdemeester · 2017-02-10T17:20:02Z

This test could be merged with the other one (that would let use create and start one daemon, create once the secret and then list them with or without filters).

removed already 😄 same as the below one.

vdemeester

Thanks for the unit tests 👼 I feel some cases are redundant (for each, think of what your are testing and anything that you test multiple time can reduced to once) but that's a nit. The integration test could be a little bit more readable (by extracting some common stuff, like the creation of secrets, the common part of each filter command), but that's a nit too.

But we need to version this change and make sure filtering using names work on API 1.25 and 1.26 🙏

vdemeester · 2017-02-13T10:08:29Z

We have to accept this still (for few version and for API versions 1.25 and 1.26). We need to "version" the accepted list of filter.

yeah, we have to say to removal seems not so reasonable, added it back already.

vdemeester · 2017-02-13T10:11:45Z

This test is testing the same as above right ? (an invalid value in the middle of valid ones) — if yes, could be remove 👼

vdemeester · 2017-02-13T10:13:02Z

Same for this one right ?

vdemeester · 2017-03-16T13:44:14Z

going to docs-review 👼 /cc @thaJeztah

thaJeztah

Thanks @allencloud, I left some comments

thaJeztah · 2017-03-20T14:46:39Z

Can you add a blank line before this? I think the Jekyll markdown parser doesn't like it if there's isn't a blank line 😓

thaJeztah · 2017-03-20T14:57:53Z

can you change the first sentence to;

The filtering flag (`-f` or `--filter`) format is a `key=value` pair. .....

thaJeztah · 2017-03-20T15:04:11Z

I think in other locations we add a short description here, something like;

* [id](secret_ls.md#id) (secret's ID) * [label](secret_ls.md#label) (`label=<key>` or `label=<key>=<value>`) * [name](secret_ls.md#name) (secret's name)

Do we need names (plural) here? I know we have "names" for docker ps because of historical reasons (linked containers), but I don't think it's needed for secrets?

thaJeztah · 2017-03-20T15:05:38Z

For a follow up discussion; I'm not very fond of just id here as heading; I think having filter by id or filter secrets by id would be more descriptive, and possibly better for discoverability (searching on Google or the docs search)

@mstanleyjones WDYT?

thaJeztah · 2017-03-20T15:06:36Z

Do we have names (plural) as filter currently? Or only name ?

Sorry for my delay. I am afraid that currently we support names. And this is something we hope to deprecate to use name instead. Actually it is indeed supported in docker/master.

@allencloud but docker 17.03 does not have docker secret ls --filter. In that case we can remove support for "names", because it has not yet been in a release

Or is it used somewhere else already?

Ah, I see, it was already used behind the scenes https://github.com/docker/docker/pull/30810/files#diff-74117112d8788c60f76ddd5fdb6e62b4R30

thaJeztah · 2017-03-20T15:08:10Z

Can you move the tests for other objects to a separate PR?

thaJeztah · 2017-03-20T15:09:18Z

I'd opt for removing names (plural). The examples in this section are also incorrect, because they use name (singular)

Any input for names in secret filtering? @ehazlett @vdemeester @aaronlehmann

Let's remove it from the doc.

Thanks @aaronlehmann, I removed names in the command docker secret ls doc, while remained that in swagger.yml. Is that OK? PTAL

thaJeztah · 2017-03-20T15:10:29Z

Hm, I think we do this in other places as well, but wondering; do we actually match a part or only a prefix (which is, erm, a part, but only if the ID starts with the given value)

I think id filtering is only supported by whole id, or prefix of ID. like:

root@ip-172-31-23-42:~/gocode/src/github.com/allencloud/docker/bundles/17.05.0-dev/binary-client# ./docker secret ls ID NAME CREATED UPDATED 6697bflskwj1998km1gnnjr38 q5s5570vtvnimefos1fyeo2u2 7 weeks ago 7 weeks ago 9u9hk4br2ej0wgngkga6rp4hq my_secret 6 weeks ago 6 weeks ago lofl440t1pmh7vz3qt81v704g allen 6 days ago 6 days ago mem02h8n73mybpgqjf0kfi1n0 test_secret 6 days ago 6 days ago sj04gk80yqk2xtdrs64nsyoln test 6 weeks ago 6 weeks ago ueo2vbjbl8kk97mrmlq509e7r my_secsret 5 weeks ago 5 weeks ago root@ip-172-31-23-42:~/gocode/src/github.com/allencloud/docker/bundles/17.05.0-dev/binary-client# ./docker secret ls --filter id=66 ID NAME CREATED UPDATED 6697bflskwj1998km1gnnjr38 q5s5570vtvnimefos1fyeo2u2 7 weeks ago 7 weeks ago

cpuguy83 · 2017-03-23T02:21:06Z

Any updates?

Signed-off-by: allencloud <allen.sun@daocloud.io>

allencloud · 2017-03-27T03:09:51Z

Updated. PTAL @thaJeztah

thaJeztah

LGTM

thaJeztah · 2017-03-28T11:41:30Z

/cc @albers @sdurrheimer for completion scripts 👍

thaJeztah · 2017-03-28T11:43:16Z

failures are not related

albers · 2017-03-29T16:38:53Z

@thaJeztah bash completion is blocked by pending #32013.

…filter make secret ls support filters in CLI

GordonTheTurtle added the status/0-triage label Feb 8, 2017

allencloud force-pushed the make-secret-ls-support-filter branch 2 times, most recently from bec81ff to 604dcca Compare February 8, 2017 05:38

allencloud force-pushed the make-secret-ls-support-filter branch from 604dcca to 848e977 Compare February 8, 2017 05:52

vdemeester added status/1-design-review and removed status/0-triage labels Feb 8, 2017

vdemeester reviewed Feb 8, 2017

View reviewed changes

allencloud force-pushed the make-secret-ls-support-filter branch 2 times, most recently from e281e7b to ce4d948 Compare February 9, 2017 01:11

aaronlehmann added status/2-code-review and removed status/1-design-review labels Feb 9, 2017

allencloud force-pushed the make-secret-ls-support-filter branch from ce4d948 to f2a6c7a Compare February 9, 2017 05:30

cpuguy83 approved these changes Feb 9, 2017

View reviewed changes

allencloud force-pushed the make-secret-ls-support-filter branch from f2a6c7a to d398b26 Compare February 10, 2017 06:35

allencloud force-pushed the make-secret-ls-support-filter branch from d398b26 to 51aed85 Compare February 10, 2017 16:18

vdemeester suggested changes Feb 10, 2017

View reviewed changes

allencloud force-pushed the make-secret-ls-support-filter branch 4 times, most recently from 6d96855 to 9c23fd3 Compare February 13, 2017 03:50

vdemeester suggested changes Feb 13, 2017

View reviewed changes

vdemeester added status/3-docs-review and removed status/2-code-review labels Mar 16, 2017

allencloud force-pushed the make-secret-ls-support-filter branch from 9a1fe51 to 6cf6994 Compare March 17, 2017 05:25

thaJeztah requested changes Mar 20, 2017

View reviewed changes

allencloud force-pushed the make-secret-ls-support-filter branch 9 times, most recently from f5f2dfa to 9bc7bd9 Compare March 26, 2017 02:30

make secret ls support filters in CLI

3935074

Signed-off-by: allencloud <allen.sun@daocloud.io>

allencloud force-pushed the make-secret-ls-support-filter branch from 9bc7bd9 to 3935074 Compare March 27, 2017 02:16

thaJeztah added impact/api impact/changelog impact/cli labels Mar 28, 2017

thaJeztah approved these changes Mar 28, 2017

View reviewed changes

thaJeztah merged commit 4df350b into moby:master Mar 28, 2017

GordonTheTurtle added this to the 17.05.0 milestone Mar 28, 2017

allencloud deleted the make-secret-ls-support-filter branch March 31, 2017 02:41

albers mentioned this pull request Apr 2, 2017

Add bash completion for secret ls --format #32305

Merged

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30810 from allencloud/make-secret-ls-support-…

d14a4d4

…filter make secret ls support filters in CLI

Conversation

allencloud commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allencloud commented Feb 8, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

ehazlett commented Feb 8, 2017

Uh oh!

diogomonica commented Feb 8, 2017

Uh oh!

diogomonica commented Feb 8, 2017

Uh oh!

aaronlehmann commented Feb 8, 2017

Uh oh!

allencloud commented Feb 9, 2017

Uh oh!

aaronlehmann commented Feb 9, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Feb 10, 2017

Uh oh!

allencloud commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Mar 16, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allencloud commented Feb 8, 2017 •

edited

Loading

thaJeztah Mar 28, 2017 •

edited

Loading