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 format to secret ls #31552

Merged
merged 1 commit into from Mar 20, 2017

Conversation

@boaz1337
Contributor

boaz1337 commented Mar 5, 2017

- What I did
Add the format option to the docker-secret-ls command
related to #30431

- How I did it

  • Add the format option to the cli/command/secret/ls.go
  • Create cli/command/formatter/secret.go
  • Add unit tests

- How to verify it
TESTDIRS='cli/command/formatter/' TESTFLAGS='-test.run ^TestSecret*' hack/make.sh test-unit

- Description for the changelog
Add format to secret ls

@boaz1337 boaz1337 referenced this pull request Mar 5, 2017

Closed

[UX] implementation status of the `--format` #30431

23 of 24 tasks complete
return sCtx
}
type secretContext struct {

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Mar 6, 2017

Member

MarshalJSON is needed (please refer to similar code in the package)

@AkihiroSuda

AkihiroSuda Mar 6, 2017

Member

MarshalJSON is needed (please refer to similar code in the package)

This comment has been minimized.

@boaz1337

boaz1337 Mar 6, 2017

Contributor

👍

@boaz1337

boaz1337 Mar 6, 2017

Contributor

👍

@AkihiroSuda

Also, please add docs?

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 6, 2017

Contributor

@AkihiroSuda thanks a lot. I will fix that 😺

Contributor

boaz1337 commented Mar 6, 2017

@AkihiroSuda thanks a lot. I will fix that 😺

@vdemeester

design LGTM for me 👼

Show outdated Hide outdated cli/command/secret/ls.go Outdated
@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 6, 2017

Contributor

Added documentation + fixed comments

Contributor

boaz1337 commented Mar 6, 2017

Added documentation + fixed comments

@thaJeztah

Thanks @ripcurld0, overall looking good, but I think some things are missing (see my comments inline) ☺️

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
Show outdated Hide outdated cli/command/secret/ls.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 8, 2017

Member

I think we should also have .Labels and .Label (similar to the option on docker ps), given that secrets can have labels, and a --filter option is added in #30810

Member

thaJeztah commented Mar 8, 2017

I think we should also have .Labels and .Label (similar to the option on docker ps), given that secrets can have labels, and a --filter option is added in #30810

@thaJeztah

Thanks! I think we're almost there; code LGTM, left some comments for the docs

property is not set, the client falls back to the default table
format. For a list of supported formatting directives, see
[**Formatting** section in the `docker secret ls` documentation](secret_ls.md)

This comment has been minimized.

@thaJeztah
@thaJeztah

thaJeztah Mar 13, 2017

Member

Can you add an example as well in the sample JSON below? (https://github.com/docker/docker/pull/31552/files#diff-578706828b816c40dd41b66896ab92c5R214)

This comment has been minimized.

@boaz1337

boaz1337 Mar 13, 2017

Contributor

What JSON? 🐈

@boaz1337

boaz1337 Mar 13, 2017

Contributor

What JSON? 🐈

This comment has been minimized.

@thaJeztah

thaJeztah Mar 13, 2017

Member

Oh there's a sample config.json configuration file; would be good to have an example secretFormat in there;

screen shot 2017-03-13 at 18 07 45

@thaJeztah

thaJeztah Mar 13, 2017

Member

Oh there's a sample config.json configuration file; would be good to have an example secretFormat in there;

screen shot 2017-03-13 at 18 07 45

This comment has been minimized.

@boaz1337

boaz1337 Mar 13, 2017

Contributor

Got it! 👍

@boaz1337

boaz1337 Mar 13, 2017

Contributor

Got it! 👍

Show outdated Hide outdated docs/reference/commandline/secret_ls.md Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Mar 13, 2017

ping @AkihiroSuda ptal

Add format to secret ls
Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
@MHBauer

This comment has been minimized.

Show comment
Hide comment
@MHBauer

MHBauer Mar 20, 2017

Contributor

Concept, code, docs looks good to me. @AkihiroSuda, @thaJeztah anything else?

Contributor

MHBauer commented Mar 20, 2017

Concept, code, docs looks good to me. @AkihiroSuda, @thaJeztah anything else?

@thaJeztah

oh, I missed it was updated 😃

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 20, 2017

Member

I think @AkihiroSuda's comments are addressed, so let me go ahead 👍

Member

thaJeztah commented Mar 20, 2017

I think @AkihiroSuda's comments are addressed, so let me go ahead 👍

@thaJeztah thaJeztah merged commit 1ec77ba into moby:master Mar 20, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31850 has succeeded
Details
janky Jenkins build Docker-PRs 40470 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 565 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11550 has succeeded
Details
z Jenkins build Docker-PRs-s390x 454 has succeeded
Details

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 20, 2017

Member

/cc @albers @sdurrheimer for completion scripts ❤️

Member

thaJeztah commented Mar 20, 2017

/cc @albers @sdurrheimer for completion scripts ❤️

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 20, 2017

Contributor

🙌 🕺

Contributor

boaz1337 commented Mar 20, 2017

🙌 🕺

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

@boaz1337 boaz1337 deleted the boaz1337:add_format_secretls branch Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment