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

Add --format to docker-search #440

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Add --format to docker-search #440

merged 1 commit into from
Aug 22, 2017

Conversation

boaz0
Copy link
Contributor

@boaz0 boaz0 commented Aug 15, 2017

- What I did

This is a follow-up to Jeremy's PR moby/moby#31539.

This patch adds the format flag to the docker search command and by that completes moby/moby#30431.

Signed-off-by: Jeremy Chambers jeremy@thehipbot.com
Signed-off-by: Boaz Shuster ripcurld.github@gmail.com

- How I did it

  • Created cli/command/formatter/search.go and cli/command/formatter/search_test.go
  • Updated cli/command/registry/search.go and docs/reference/commandline/search.md

- How to verify it

Run unit tests

- Description for the changelog

Add format to docker search

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


### Format the output

The formatting option (`--format`) will pretty print search output
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty-print

The formatting option (`--format`) will pretty print search output
using a Go template.

Valid placeholders for the Go template are listed below:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/are listed below:/are:

| `.IsOfficial` | "OK" if image is official |
| `.IsAutomated` | "OK" if image build was automated |

When using the `--format` option, the `search` command will either
Copy link
Contributor

Choose a reason for hiding this comment

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

s/when using/when you use

| `.IsAutomated` | "OK" if image build was automated |

When using the `--format` option, the `search` command will either
output the data exactly as the template declares or, when using the
Copy link
Contributor

Choose a reason for hiding this comment

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

declares. If you use the `table directive, column headers are included as well.

{% endraw %}
```

To search for images in a table format youcan use:
Copy link
Contributor

Choose a reason for hiding this comment

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

This example outputs a table format:

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@codecov-io
Copy link

codecov-io commented Aug 22, 2017

Codecov Report

Merging #440 into master will decrease coverage by 0.2%.
The diff coverage is 96.36%.

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
- Coverage   47.04%   46.84%   -0.21%     
==========================================
  Files         198      198              
  Lines       16349    16339      -10     
==========================================
- Hits         7692     7654      -38     
- Misses       8262     8295      +33     
+ Partials      395      390       -5

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.

Thanks! LGTM

some minor comments, but nothing blocking merge

format := options.format
if len(format) == 0 {
format = formatter.TableFormatKey
}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: we could handle this default is formatter.NewSearchFormat()

}
}
return official
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, a function could be used to remove the duplication in IsOfficial() and IsAutomated()

func (c *searchContext) formatBool(value bool) string {
    switch {
    case value && c.json:
        return "true"
    case value:
        return "[OK]"
    case c.json:
        return "false"
    }
    return ""
}

}
}

func TestSearchContext_Description(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: usually there are no underscores in test names: TestSearchContextDescription

@boaz0
Copy link
Contributor Author

boaz0 commented Aug 22, 2017

@dnephin no problemo

@dnephin
Copy link
Contributor

dnephin commented Aug 22, 2017

@mstanleyjones docs looks good now?

Copy link
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

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

Approved with one teensy little suggestion for improvement. Thanks! 🙌


### Format the output

The formatting option (`--format`) will pretty-print search output
Copy link
Contributor

Choose a reason for hiding this comment

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

s/will pretty-print/pretty-prints

@boaz0
Copy link
Contributor Author

boaz0 commented Aug 22, 2017

@mstanleyjones done! thanks for your review 👍 🍡

@mdlinville
Copy link
Contributor

GMail renders that 🍡 as a radish!


### Format the output

The formatting option (`--format`) will pretty-prints search output
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to take out the word "will" here.

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>
Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
@boaz0
Copy link
Contributor Author

boaz0 commented Aug 22, 2017

@mstanleyjones ☕️

Copy link
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

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

🍦

@dnephin dnephin merged commit 05308fc into docker:master Aug 22, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 22, 2017
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.

7 participants