image: export `ImageListEntry` type for image list #3383

euank · 2016-11-17T01:17:01Z

This allows external consumers to just directly unmarshal json into this
exported type.

I modified the types of some of its values for consistency with other
exported types as well. This is unfortunately backwards incompatible.

s-urbaniak

I have just a small nit to refer to the struct members format, and full inside methods of printableImage instead of referencing the constants flagImageFormat, and flagFullOutput.

Thanks!

s-urbaniak · 2016-11-17T07:55:24Z

rkt/image_list.go

+)
+
+func (pi *printableImage) imageSize() string {
+	if flagImageFormat != outputFormatTabbed || flagFullOutput {


s/flagImageFormat/pi.format

s/flagFullOutput/pi.full

s-urbaniak · 2016-11-17T07:55:40Z

rkt/image_list.go

+
+func (pi *printableImage) importTime() string {
+	t := time.Unix(0, pi.ImportTime)
+	if flagImageFormat != outputFormatTabbed || flagFullOutput {


s/flagImageFormat/pi.format (as above)

s/flagFullOutput/pi.full (as above)

s-urbaniak · 2016-11-17T07:58:02Z

rkt/image_list.go

+
+func (pi *printableImage) lastUsedTime() string {
+	t := time.Unix(0, pi.LastUsedTime)
+	if flagImageFormat != outputFormatTabbed || flagFullOutput {


s/flagImageFormat/pi.format (as above)

s/flagFullOutput/pi.full (as above)

euank · 2016-11-17T08:10:42Z

Good catch, addressed.

euank · 2016-11-17T08:14:07Z

Also, for some context, this came as a result of the change in rktlet where we're programmatically consuming this and it would be nice to unmarshal into a shared type and not have to do as much string parsing.

I feel less bad about the technically backwards incompatible change because this is such a new feature that it's unlikely any other clients exist in the wild.

s-urbaniak

Agreed, the whole functionality is actually marked as experimental, and marked behind an env var.

Reading this I already assumed that this was intended for rtklet ;-)

LGTM on green

euank · 2016-11-17T08:50:13Z

image list --format=json isn't behind any env var mask afaik, and that's where the backwards incompatibility lies. Maybe worth calling out in release notes.

s-urbaniak · 2016-11-18T14:15:07Z

asking @lucab for a final pass

lucab · 2016-11-18T15:02:22Z

lib/types.go

+		ImportTime int64 `json:"import_time"`
+		// LastUsedTimeNano indicates when was last used in nanoseconds since the
+		// unix epoch
+		LastUsedTime int64 `json:"last_used_time"`


Field name and comment don't match (here and in the field above).

lucab · 2016-11-18T15:13:45Z

This LGTM except a very minor nit.

I'm not too much concerned about json-out compatibility right now, but I see the problem coming to us in the near future. list is basically just dumping out our internal DB, so fields will probably have to evolve with it. On top of that we are still defining library types, so some changes are also to be expected there. Should we perhaps start sticking a version field to the JSON we emit? Other options here?

This allows external consumers to just directly unmarshal json into this exported type. I modified the types of some of its values for consistency with other exported types as well. This is unfortunately backwards incompatible.

squeed · 2016-11-22T11:41:42Z

Indeed, we now have some form of an API and can "never" remove fields from this value. So we should probably build an integration test that ensures this fact.

euank added area/api area/usability component/stage0 labels Nov 17, 2016

s-urbaniak suggested changes Nov 17, 2016

View reviewed changes

euank force-pushed the image-output-export branch from 8ece396 to 528ecd8 Compare November 17, 2016 08:10

s-urbaniak approved these changes Nov 17, 2016

View reviewed changes

s-urbaniak added this to the v1.20.0 milestone Nov 18, 2016

lucab reviewed Nov 18, 2016

View reviewed changes

euank force-pushed the image-output-export branch 2 times, most recently from 5d737cc to a140ab2 Compare November 18, 2016 19:46

image: export ImageListEntry type for image list

9c58da8

This allows external consumers to just directly unmarshal json into this exported type. I modified the types of some of its values for consistency with other exported types as well. This is unfortunately backwards incompatible.

euank force-pushed the image-output-export branch from a140ab2 to 9c58da8 Compare November 18, 2016 21:45

lucab added reviewed/lgtm reviewed/merge-on-green labels Nov 21, 2016

lucab merged commit 6038f19 into rkt:master Nov 22, 2016

euank deleted the image-output-export branch November 22, 2016 19:02

image: export ImageListEntry type for image list #3383

image: export ImageListEntry type for image list #3383

Uh oh!

Conversation

euank commented Nov 17, 2016

Uh oh!

s-urbaniak left a comment

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

s-urbaniak Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

euank commented Nov 17, 2016

Uh oh!

euank commented Nov 17, 2016

Uh oh!

s-urbaniak left a comment

Choose a reason for hiding this comment

Uh oh!

euank commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-urbaniak commented Nov 18, 2016

Uh oh!

lucab Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

lucab commented Nov 18, 2016

Uh oh!

squeed commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

image: export `ImageListEntry` type for image list #3383

image: export `ImageListEntry` type for image list #3383

euank commented Nov 17, 2016 •

edited

Loading