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

make secret ls support filters in CLI #30810

Merged
merged 1 commit into from Mar 28, 2017

Conversation

@allencloud
Contributor

allencloud commented Feb 8, 2017

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

  1. rename ./secret/ls.go to ./secret/list.go;
  2. support filter in secret ls command;
  3. add a test case for docker secret ls --filter;
  4. add more filter options in swagger.yml;
  5. 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

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Feb 8, 2017

Contributor

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

Contributor

allencloud commented Feb 8, 2017

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

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Feb 8, 2017

Contributor

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.

Contributor

ehazlett commented Feb 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Feb 8, 2017

Contributor

LGTM

Contributor

diogomonica commented Feb 8, 2017

LGTM

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica
Contributor

diogomonica commented Feb 8, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 8, 2017

Contributor

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.

Contributor

aaronlehmann commented Feb 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Feb 9, 2017

Contributor

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

Contributor

allencloud commented Feb 9, 2017

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

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 9, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Feb 9, 2017

LGTM

@cpuguy83

LGTM

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Feb 10, 2017

Member

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

Member

AkihiroSuda commented Feb 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Feb 10, 2017

Contributor

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

Contributor

allencloud commented Feb 10, 2017

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

Show outdated Hide outdated docs/api/version-history.md Outdated
Show outdated Hide outdated integration-cli/docker_cli_secret_ls_test.go Outdated
"github.com/go-check/check"
)
func (s *DockerSwarmSuite) TestSecretList(c *check.C) {

This comment has been minimized.

@vdemeester

vdemeester Feb 10, 2017

Member

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).

@vdemeester

vdemeester Feb 10, 2017

Member

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).

This comment has been minimized.

@allencloud

allencloud Feb 15, 2017

Contributor

removed already 😄 same as the below one.

@allencloud

allencloud Feb 15, 2017

Contributor

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 🙏

Show outdated Hide outdated daemon/cluster/filters.go Outdated
Show outdated Hide outdated daemon/cluster/filters_test.go Outdated
Show outdated Hide outdated daemon/cluster/filters_test.go Outdated
@allencloud

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Feb 15, 2017

Contributor

Actually, currently I added filter names back. API 1.25 and 1.26 have already supported names filter.

If we plan to remove this, it must be from 1.27, while I think the deprecated policy is also something we need to think about. I wish in the future we could remove this if we think it is redundant.

While is the current solution OK? or more input to make more discussion. @vdemeester

Contributor

allencloud commented Feb 15, 2017

Actually, currently I added filter names back. API 1.25 and 1.26 have already supported names filter.

If we plan to remove this, it must be from 1.27, while I think the deprecated policy is also something we need to think about. I wish in the future we could remove this if we think it is redundant.

While is the current solution OK? or more input to make more discussion. @vdemeester

@cpuguy83

LGTM

ping @vdemeester

Show outdated Hide outdated daemon/cluster/filters.go Outdated

@thaJeztah thaJeztah referenced this pull request Mar 8, 2017

Merged

Add format to secret ls #31552

@allencloud

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Mar 14, 2017

Contributor

Could you help to review this one again? @vdemeester 😸

Contributor

allencloud commented Mar 14, 2017

Could you help to review this one again? @vdemeester 😸

@vdemeester

LGTM 🐸

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 16, 2017

Member

going to docs-review 👼 /cc @thaJeztah

Member

vdemeester commented Mar 16, 2017

going to docs-review 👼 /cc @thaJeztah

@thaJeztah

Thanks @allencloud, I left some comments

Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
@@ -7958,6 +7958,9 @@ paths:
description: |
A JSON encoded value of the filters (a `map[string][]string`) to process on the secrets list. Available filters:
- `id=<secret id>`
- `label=<key> or label=<key>=value`
- `name=<secret name>`
- `names=<secret name>`

This comment has been minimized.

@thaJeztah

thaJeztah Mar 20, 2017

Member

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

@thaJeztah

thaJeztah Mar 20, 2017

Member

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

This comment has been minimized.

@allencloud

allencloud Mar 23, 2017

Contributor

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

allencloud Mar 23, 2017

Contributor

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.

This comment has been minimized.

@thaJeztah

thaJeztah Mar 28, 2017

Member

@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

@thaJeztah

thaJeztah Mar 28, 2017

Member

@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

This comment has been minimized.

@thaJeztah

thaJeztah Mar 28, 2017

Member

Or is it used somewhere else already?

@thaJeztah

thaJeztah Mar 28, 2017

Member

Or is it used somewhere else already?

This comment has been minimized.

@thaJeztah
@thaJeztah

thaJeztah Mar 28, 2017

Member
Show outdated Hide outdated daemon/cluster/filters_test.go Outdated
Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 23, 2017

Contributor

Any updates?

Contributor

cpuguy83 commented Mar 23, 2017

Any updates?

make secret ls support filters in CLI
Signed-off-by: allencloud <allen.sun@daocloud.io>
@allencloud

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Mar 27, 2017

Contributor

Updated. PTAL @thaJeztah

Contributor

allencloud commented Mar 27, 2017

Updated. PTAL @thaJeztah

@thaJeztah

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 28, 2017

Member

/cc @albers @sdurrheimer for completion scripts 👍

Member

thaJeztah commented Mar 28, 2017

/cc @albers @sdurrheimer for completion scripts 👍

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 28, 2017

Member

failures are not related

Member

thaJeztah commented Mar 28, 2017

failures are not related

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

4 of 6 checks passed

windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11807 has failed
Details
z Jenkins build Docker-PRs-s390x 718 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32116 has succeeded
Details
janky Jenkins build Docker-PRs 40731 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 857 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 28, 2017

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Mar 29, 2017

Member

@thaJeztah bash completion is blocked by pending #32013.

Member

albers commented Mar 29, 2017

@thaJeztah bash completion is blocked by pending #32013.

@allencloud allencloud deleted the allencloud:make-secret-ls-support-filter branch Mar 31, 2017

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-…
…filter

make secret ls support filters in CLI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment