10000 stage0/image: list images output in JSON format by lucasallan · Pull Request #3334 · rkt/rkt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

stage0/image: list images output in JSON format #3334

Merged
merged 1 commit into from
Nov 7, 2016
Merged

stage0/image: list images output in JSON format #3334

merged 1 commit into from
Nov 7, 2016

Conversation

lucasallan
Copy link
Contributor
@lucasallan lucasallan commented Oct 31, 2016 •
edited by lucab
Loading

@lucab PTAL

List images in the json format:

ubuntu@ubuntu-xenial:/rkt/build-rir$ sudo target/bin/rkt image list --format=json
[{"id":"sha512-bd4c342830c8","name":"coreos.com/rkt/stage1-coreos:1.17.0","import_time":"2 days ago","last_used":"2 days ago","size":"179MiB"},{"id":"sha512-7b1e1a77f0b6","name":"coreos.com/rkt/builder:1.1.0","import_time":"2 days ago","last_used":"2 days ago","size":"1.6GiB"}]

And json-pretty:

ubuntu@ubuntu-xenial:/rkt/build-rir$ sudo target/bin/rkt image list --format=json-pretty
[
	{
		"id": "sha512-bd4c342830c8",
		"name": "coreos.com/rkt/stage1-coreos:1.17.0",
		"import_time": "2 days ago",
		"last_used": "2 days ago",
		"size": "179MiB"
	},
	{
		"id": "sha512-7b1e1a77f0b6",
		"name": "coreos.com/rkt/builder:1.1.0",
		"import_time": "2 days ago",
		"last_used": "2 days ago",
		"size": "1.6GiB"
	}
]

Fixes #3298

@ghost
Copy link
ghost commented Oct 31, 2016

Can one of the admins verify this patch?

@s-urbaniak
Copy link
Contributor

ok to test

@lucab lucab changed the title Feature: rkt image list --format=json stage0/image: list images output in JSON format Nov 1, 2016
@lucab
Copy link
Member
lucab commented Nov 1, 2016 •
edited
Loading

@lucasallan this looks really nice, thanks a lot! Code seems fine at a quick glance, can you please just format the commit titles as shown in the doc? I also retitled this PR to follow the same style, if you are unsure about the format.

@euank @s-urbaniak can one of you please do a final pass on this and shepherd it in?

@lucab lucab added this to the v1.19.0 milestone Nov 1, 2016
s-urbaniak
s-urbaniak suggested changes Nov 1, 2016
View reviewed changes
Copy link
Contributor
@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

Just a few nits, also I suggest to squash all commits adhering to https://github.com/coreos/rkt/blob/master/CONTRIBUTING.md.

rkt/image_list.go
@@ -140,21 +157,15 @@ func init() {
cmdImageList.Flags().Var(&flagImagesSortAsc, "order", `choose the sorting order if at least one sort field is provided (--sort). Accepted values: "asc", "desc"`)
cmdImageList.Flags().BoolVar(&flagNoLegend, "no-legend", false, "suppress a legend with the list")
cmdImageList.Flags().BoolVar(&flagFullOutput, "full", false, "use long output format")
cmdImageList.Flags().StringVar(&flagImageFormat, "format", "", "choose the output format, allowed format includes 'json', 'json-pretty'. If empty, then the result is printed as key value pairs")

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary newline

rkt/image_list.go
}

imageImportTime := humanize.Time(aciInfo.ImportTime)
imageLastUsed := humanize.Time(aciInfo.LastUsed)
Copy link
Contributor

Choose a reason for hiding this comment

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

these humanized time strings will also appear in the JSON output, but JSON is meant to be machine-readable. I suggest to use rfc3339 based outputs (see https://godoc.org/time#pkg-constants) in this case.

rkt/image_list.go
imageImportTime := humanize.Time(aciInfo.ImportTime)
imageLastUsed := humanize.Time(aciInfo.LastUsed)
totalSize := aciInfo.Size + aciInfo.TreeStoreSize
imageSize := humanize.IBytes(uint64(totalSize))
Copy link
Contributor

Choose a reason for hiding this comment

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

for machine readable JSON output I'd go with plain totalSize.

rkt/image_list.go
trimLength := pos + 13
if pos > 0 && trimLength < len(imageID) {
imageID = imageID[:trimLength]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's put the whole else block in a separate private function, i.e. trimImageID

Copy link
Contributor

Choose a reason for hiding this comment

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

Also for the JSON case, I would not trim the imageID, this also retains symmetry with rkt list, which also ignores rkt list --full --format=json.

@lucasallan
Copy link
Contributor Author

Thanks for the feedback, I really appreciate it. I'll make the requested changes and get back to you guys ASAP.

@s-urbaniak
Copy link
Contributor

@lucasallan looking forward to, thanks a lot for the contribution!

@lucasallan
Copy link
Contributor Author

I've made all the requested changes and I also squashed and reworded the commits (I hope I got it right). Let me know if you need me to do any other change.

Thanks!

s-urbaniak
s-urbaniak suggested changes Nov 2, 2016
View reviewed changes
Copy link
Contributor
@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

The code LGTM, thanks!

I retriggered a build, as it seems the git checkout didn't work properly on the CI machine.

One small nit in the commit message:
Replace "For more information: github.com//issues/3298" with "Fixes #3298"

@lucasallan
Copy link
Contributor Author

Fixed the commit message, cc @s-urbaniak

euank
euank reviewed 8000 Nov 3, 2016
View reviewed changes
rkt/image_list.go
@@ -113,6 +131,7 @@ var (
flagImagesFields *rktflag.OptionList
flagImagesSortFields *rktflag.OptionList
flagImagesSortAsc ImagesSortAsc
flagImageFormat string
Copy link
Member

Choose a reason for hiding this comment

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

We could make a type OutputFormat int that implements the cobra Set method and does input validation.

It would also let all the == "" things below look prettier with a const OutputFormatTabbed type thing.

I'm okay with this cleanup as a followup, just wanted to capture the thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great, thanks!

@euank
Copy link
Member
euank commented Nov 3, 2016

LGTM, though I tossed in a suggestion for a cleanup (either now or later).

The other missing thing that can come in a followup is tests!

@s-urbaniak
Copy link
Contributor

@lucasallan Do you mind tackling @euank's comments? :-)

@lucasallan
Copy link
Contributor Author

@s-urbaniak yes, will do asap. thanks!

@lucasallan
Copy link
Contributor Author
lucasallan commented Nov 4, 2016 •
edited
Loading

I've made some changes, could you take a look please? @euank

I'm also looking into adding tests for it and for rkt list --format=json as well - but I think I'll do in a different PR. Just to let you guys know, I'm not an experienced Go developer, I have been learning it in my free time and I'm using OSS contributions to improve my knowledge so all the feedback is welcome!

euank
euank suggested changes Nov 4, 2016
View reviewed changes
Copy link
Member
@euank euank left a comment

Choose a reason for hiding this comment

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

I clarified a bit more what I was suggesting before.
Also, the test failures do look related (FAIL: TestImageSize, looks like --full used to print size-in-bytes and FAIL: TestAPIServiceListInspectImages).
I find semaphore's output the easiest to read.

The help is appreciated! Tests in a followup PR sound fine to me.

rkt/image_list.go
return strings.Join(r.attributes(), "\t")
}

type outputFormat int
Copy link
Member

Choose a reason for hiding this comment

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

I didn't express myself totally clearly on this, but I was also saying some consts could help readability.

Even though go doesn't have real enums, it's sorta idiomatic to make something like them with a type "alias" + consts (sorta like the iota docs reference). In this case, it would look something like:

const (
    outputFormatTabbed outputFormat = iota
    outputFormatJSON
    outputFormatPrettyJSON
)

The consts can then be used for assignment and comparison to make things more readable and to give a little bit of type safety.

Copy link
Contributor Author 1E80

Choose a reason for hiding this comment

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

I've made the changes. I've never used iota before - Thanks.

Copy link
Member
@euank euank Nov 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

One last nit... Typically the consts for an "enum like thing" are grouped right under the type definition (for example, ConnState in the go stdlib).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

@lucasallan lucasallan force-pushed the image-list-json branch 2 times, most recently from 12c317b to e54dc98 Compare November 4, 2016 23:20
@euank
Copy link
Member
euank commented Nov 5, 2016 •
edited
Loading

I have one last nit about where the consts are; after that LGTM on green.

Edit: Nit addressed, proper LGTM!
I'll leave it for @s-urbaniak to give a final look and merge

@lucasallan
stage0/image: list images output in JSON format
ee41e22 
These changes add the option of printing the image list in json and
json-pretty format. Now you can specify the output format when listing
images:
  rkt image list --format=json
  rkt image list --format=json-pretty

Fixes rkt#3298
s-urbaniak
s-urbaniak approved these changes Nov 7, 2016
View reviewed changes
Copy link
Contributor
@s-urbaniak s-urbaniak 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 a lot! 🎉

E9DC
@s-urbaniak s-urbaniak merged commit 4444fe1 into rkt:master Nov 7, 2016
@lucasallan lucasallan deleted the image-list-json branch November 7, 2016 17:37
@lucasallan
Copy link
Contributor Author

Thank you! I'm already working in another PR.

@s-urbaniak s-urbaniak mentioned this pull request Nov 9, 2016
Closed
lucab added a commit to lucab/rkt that referenced this pull request Nov 9, 2016
@lucab
image/list: bring back field filtering in plaintext mode
4061862 
This fixes a regression introduced in rkt#3334,
where the `--field` is not anymore working for plaintext output.
Tests around image listing are also introduced here to catch future regressions.
lucab added a commit to lucab/rkt that referenced this pull request Nov 9, 2016
@lucab
image/list: bring back field filtering in plaintext mode
e4de723 
This fixes a regression introduced in rkt#3334,
where the `--field` flag is not anymore working for plaintext output.
Tests around image listing are also introduced here to catch future regressions.
@lucab lucab mentioned this pull request Nov 9, 2016
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s-urbaniak s-urbaniak s-urbaniak approved these changes

@euank euank euank approved these changes

Assignees
No one assigned
Labels
area/usability component/stage0 kind/enhancement
Projects
None yet
Milestone
v1.19.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lucasallan @s-urbaniak @lucab @euank
0