Created timesptamp returned by imagelist should be in unix format

[rhatdan]

In the API, we are currently returning the image time of creation
as a string, in time.Time format. The API is for a 64 bit integer
representing Unix time.

Signed-off-by: Daniel J Walsh [email protected]

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

Fixes #6796

[rhatdan]

@skorhone @baude @jwhonce @mheon @giuseppe @vrothberg @QiWang19 @ashley-cui PTAL

[mheon]

@baude @jwhonce Was it the case that this should only be true for certain API versions?

[QiWang19]

LGTM

[TomSweeneyRedHat]

Tests are looking a bit red in the face @rhatdan

[skorhone]

Versions 1.24, 1.34 and v1.40 of documentation all have it defined as an integer (unix time).

LGTM

[mheon]

LGTM

[QiWang19]

LGTM

[giuseppe]

/lgtm

[vrothberg]

/lgtm

[HarryMichal]

I'm a bit late to the party but this again breaks Toolbox as it relies on the Created entry of podman images --format json to hold a string. I believe #6796 reports against the remote API of Podman but this PR also affects the CLI client.

If possible, could the changes in cmd/podman/images/list.go in the writeJSON function be reverted? And if not, could the string be moved to the CreatedAt entry (basically copying #6731 but for podman images --format json).

[vrothberg]

@rhatdan @baude PTAL

[rhatdan]

We could change the command line to translate this into a string. But it does feel wrong to translate a json file into human output.

[HarryMichal]

Similar conversation happened in #6594 already. My argument for keeping the string is mainly the fact that it was there for a long time and that it's a relatively common string in the context of Podman/Docker.

Also, I think it is not ideal to have two entries (Created and CreatedAt) holding the same information in a different format.

PS: Release notes of v2.0.2 mention this change (Fixed a bug where the timestamp format for Libpod image list endpoint was incorrect - the format has been switched to Unix time.) to be included but in fact, it is not. Maybe I'm wrong.

[rhatdan]

So if it is a string of Unix Time, then you would happy or do you want it in Human format, IE "Two hours ago"

[HarryMichal]

So if it is a string of Unix Time, then you would happy or do you want it in Human format, IE "Two hours ago"

If possible, then the Human format.

[rhatdan]

I really do not want to do human format, since it is almost impossible to translate back. Sticking it in UNIXTIME as a string, seems a good compromise. Will that break toolbox?

[HarryMichal]

The translation issue could be solved if the package (github.com/docker/go-units) that's used for generating those strings (at least I saw it in several places; I don't know if it's used everywhere) supported i18n. But that is not related to this issue, so can't be even proposed as a solution for this particular situation.

Purely technically speaking, if the Created field was a string than Toolbox will work alright. Aesthetically & practically speaking the value will be less useful because it's used in a command (toolbox list - listing of toolbox containers/images) that is meant to be used by humans. We'll have to add some handling for this to generate the string back. And if we were to do that then what was the point of #6731...

If the human format is not re-added then there will, again, be an inconsistency in the JSON format (podman ps --format json having the entry and podman images --format json not).

[skorhone]

If suggested change of Created to string also changes behavior of api v2, applications that rely on having timestamp defined as an integer will not function correctly. Not all parsers will accept integer formatted as a string when schema datatype is integer.

I must admit that it's odd that docker doesn't handle timestamps consistently in apis. While it looks like a brain fart, I think it's better to stick with the format that docker uses and use docker's way of formatting for cli's json output and podman's extended api (libpod). This will help developers to transition tooling from docker to podman with minimum effort. (I know it's just a single field and I know that breaking API sucks)

Rant: To be honest, someone should file an issue to json format. Not having timestamp format defined in json specification has generated more trouble over the years than ... well to be politically correct, let's just say that they should have defined it :-)

[mheon]

We can't break the API, so we'll have to either add a new field to it which includes human-readable time, or have a separate version of the API struct which we use exclusively for CLI output when --format=json is requested that uses the alternative field type.

[HarryMichal]

As I stated in my initial comment in this issue, I'm wondering if the human-readable string could be moved to the CreatedAt entry. That entry currently holds the same information as Created but in a different format. I don't know how the other APIs look so maybe that is not possible.

I understand that the APIs should be consistent and I'm all for it but this particular change came after V2 was released. If there was a movement to unify APIs then it should have been done on all fronts when V2 was released... (I apologize if this paragraph sounds mean.)

Anyway, it seems to me (as the devs stated several times already) that overall all APIs (not just the JSON one) should not contain the human-readable string. I'm more-or-less fine with both options.

[HarryMichal]

We can't break the API, so we'll have to either add a new field to it which includes human-readable time, or have a separate version of the API struct which we use exclusively for CLI output when --format=json is requested that uses the alternative field type.

Strictly speaking, you already broke the API by merging this PR (maybe I got the terminology wrong).

Why there has to be added a new field when there are already two fields with literally the same value but in different format (I realize this argument also goes against my request).

[debarshiray]

The problem is that the JSON output keeps changing.

Podman not having a human-readable string isn't the main problem. Toolbox can always convert a machine-readable value into it's human-readable equivalent. We discussed this in #6594 and the corresponding PR for that issue introduces an inconsistency between ps and images.

The timestamps aren't just the only fields that keep changing. See git grep "podman.CheckVersion" in toolbox.git to get a full list.

We can keep adding version checks for Podman to Toolbox, but after a certain point it becomes a game of cat and mouse.

The JSON output by its very nature is meant to be parsed by other programs. So randomly changing it is not helpful.

[rhatdan]

@debarshiray So what do you want us to do now, Change CreatedAt to use the Human format?

[owtaylor]

This seems to be an attempt to bring the HTTP API in line with the moby/docker engine HTTP API? I don't know how different compatibility should be weighed, but in the podman context, this change seems.

• To break the podman http REST api (old versions of pkg/bindings won't interoperate with newer podman)
• To break the pkg/bindings API
• To break the 'podman images --format=json' API

[ perhaps I'm missing something and not all of these will break... ]

If you just put in a backwards compat code to keep --format=json output the same, that would make things better for the current Toolbox. But what's the overall policy?

If this change is left as is, I'll point out that docs/source/markdown/podman-images.1.md has an example --format=json output that needs to be updated.

[mheon]

Every time this happens, we need to bump the API version, and introduce code in the server to properly handle the old API version. We probably need to talk about this at scrum tomorrow.

[baude]

not in this case.

[mheon]

Why? We made a breaking change to the API after it went v1.0.0 - I fail to see how this is not a version bump?

[rhatdan]

We need to revert it back to a string.

[mheon]

Best approach might be to rename it and have Created exist as a string as it did before. Confusing, but avoid breakage with the v1.0 API.

[debarshiray]

@debarshiray So what do you want us to do now, Change CreatedAt to use the Human format?

Sorry, for the delayed response. I was on vacation for the past week.

This change itself, and the other JSON changes in general, are not a huge problem for Toolbox. We can always check the Podman version at run-time and parse the JSON accordingly.

The thing is that these changes are a continuous source of minor annoyance. It would have been nicer if all these changes were evaluated and batched into the Podman 2.0.0 release, instead of doing them one by one in successive releases.

[psakar]

This fix is in the master branch but is missing in the v2.0 branch. Hence the compatibility with docker is broken at least for testcontainers, tested with podman 2.0.3

Caused by: java.lang.RuntimeException:
org.testcontainers.shaded.com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type java.lang.Long from String "2019-07-03T10:18:11.204826619Z": not a valid Long value
at [Source: (org.testcontainers.shaded.okio.RealBufferedSource$1); line: 1, column: 139] (through reference chain: java.util.ArrayList[0]->com.github.dockerjava.api.model.Image["Created"])
Caused by: org.testcontainers.shaded.com.fasterxml.jackson.databind.exc.InvalidFormatException:
Cannot deserialize value of type java.lang.Long from String "2019-07-03T10:18:11.204826619Z": not a valid Long value
at [Source: (org.testcontainers.shaded.okio.RealBufferedSource$1); line: 1, column: 139] (through reference chain: java.util.ArrayList[0]->com.github.dockerjava.api.model.Image["Created"])