Skip to content
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

Refactor trust view command into a --pretty flag on trust inspect #934

Merged
merged 2 commits into from
Mar 9, 2018

Conversation

n4ss
Copy link
Contributor

@n4ss n4ss commented Mar 9, 2018

Signed-off-by: Nassim 'Nass' Eddequiouaq eddequiouaq.nassim@gmail.com

- What I did

  • Remove docker trust view which contains the same information as docker trust inspect but in a pretty-print format.
  • Moved the pretty-print format feature to a --pretty flag on docker trust inspect (same as docker service inspect [--pretty]).
  • Cleaned-up the pretty-print ouput format.
  • Adapted the tests of docker trust view to test docker trust inspect --pretty instead with the new output format (some duplicates tests are now present because the two original commands had some similar tests)
  • docker trust inspect --pretty (old docker trust view) now accepts several images as parameters to inspect (used to be limited to 1, but docker trust inspect accepts several images so we had to keep consistency)

- How I did it
For now (I'll refactor that later), docker trust inspect executes the old codepath if no --pretty flag is provided on the cli, otherwise executes docker trust view's codepath for each image arguments.

- How to verify it
$ make -f docker.Makefile test

and

$ docker-dev trust inspect [--pretty] nginx:latest redis:latest docker:dind

- Description for the changelog

  • Moved the pretty-print format feature of docker trust view to a --pretty flag on docker trust inspect. The docker trust view has been removed.

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
@n4ss
Copy link
Contributor Author

n4ss commented Mar 9, 2018

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

couple small nits

@@ -11,24 +12,56 @@ import (
"github.com/theupdateframework/notary/tuf/data"
)

type inspectOptions struct {
remotes []string
/* FIXME(n4ss): this is consistent with `docker service inspect` but we should provide
Copy link
Contributor

Choose a reason for hiding this comment

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

go comments are generally done with // even if they are multi-line. /* */ is only used for package comments.

@@ -17,24 +17,21 @@ import (
"github.com/theupdateframework/notary/tuf/data"
)

/* TODO(n4ss): remove common tests with the regular inspect command */
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit about comment

@dnephin
Copy link
Contributor

dnephin commented Mar 9, 2018

some build failures as well

@n4ss n4ss force-pushed the refactor-trust-inspect branch 2 times, most recently from 8f98b14 to 6cfc125 Compare March 9, 2018 19:42
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
@codecov-io
Copy link

codecov-io commented Mar 9, 2018

Codecov Report

Merging #934 into master will increase coverage by <.01%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
+ Coverage   53.91%   53.92%   +<.01%     
==========================================
  Files         262      262              
  Lines       16601    16605       +4     
==========================================
+ Hits         8951     8954       +3     
  Misses       7050     7050              
- Partials      600      601       +1

Copy link
Contributor

@vieux vieux left a comment

Choose a reason for hiding this comment

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

LGTM thanks @n4ss!

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

Successfully merging this pull request may close these issues.

6 participants