Docker compat: fix JSON key capitalization in podman inspect

[rufoa]

The JSON output of podman inspect <container> has a key with name ID, whereas
the JSON output of docker inspect <container> has a key with name Id.

An attempted fix for this discrepancy was committed in #306. This made podman inspect --format '{{.Id}}' <container> work correctly by substituting {{.Id}} -> {{.ID}} internally when present in the --format arg.

However this fix is inadequate because it does not also change the name of the key in the full JSON output (e.g. when no --format is specified). This causes problems for tooling which does not specify a --format and instead parses the full JSON output itself.

Contrived example:

$ podman inspect --format '{{.Id}}' foobar  # works fine
1baec244c4ad57ac2437981f686aef0e774420a775bd531b0737e472ce0eacee

$ podman inspect foobar | jq '.[].Id'    # fails because it's still called ID in the json
null

(The Jenkins "Docker pipeline" plugin works this way and is currently incompatible with podman for this reason.)

This PR changes the JSON tag in the appropriate struct so the correct name is used when marshaled for output.

This also makes the behaviour of podman inspect <container> consistent with podman inspect <image>, which already uses the Docker-compatible key Id.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rufoa
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: giuseppe

If they are not already assigned, you can assign the PR to them by writing /assign @giuseppe in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci-robot]

Hi @rufoa. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rh-atomic-bot]

Can one of the admins verify this patch?
I understand the following commands:

• bot, add author to whitelist
• bot, test pull request
• bot, test pull request once

[mheon]

I don't know how to feel about this one. Our main goal is Docker compatability, but on the other hand we've already shipped a 1.0 and are supposed to be providing some degree of API stability. We could add a flag to toggle between pedantic Docker compat and what we're doing now, but that seems excessive.

[rufoa]

I understand your concern. This would indeed potentially break any tooling which is designed around this idiosyncrasy in the inspect schema. And I agree that putting this change behind a flag would be excessive, as would e.g. having duplicate entries for ID and Id.

I suppose this possibility should be weighed against how much existing code consumes the output of docker inspect as json and would break if podman were dropped-in as a replacement for Docker – i.e. just how seamless you aim for the transition to be.

---

FWIW the existing substitution trick is pretty brittle, and doesn't cope with variations in whitespace like {{ .Id }} (apparently not that rare in the wild), let alone more complicated templates.

I wonder whether a simple regex substitution like \.Id(\W) -> .ID$1 would give better coverage here (assuming the current behaviour isn't also frozen since 1.0).

[mheon]

@vrothberg @baude @rhatdan Thoughts on this one?

[rhatdan]

Would putting special casing inside of --format be enough.
If user looks for matching with Id or ID, we try both cases?

[vrothberg]

Would putting special casing inside of --format be enough.
If user looks for matching with Id or ID, we try both cases?

I think that would only be a partial solution as the issue persists when post-processing the data at a later point. Before making a decision on this specific case, I think we should check if there are other fields where identifiers differ. Maybe that would influence our decision making.

[mheon]

I started doing that yesterday, and found a few places where it format was blatantly wrong (most notably mounts), but I didn't see many other cases of slightly off

…

On Wed, May 22, 2019, 03:02 Valentin Rothberg ***@***.***> wrote: Would putting special casing inside of --format be enough. If user looks for matching with Id or ID, we try both cases? I think that would only be a partial solution as the issue persists when post-processing the data at a later point. Before making a decision on this specific case, I think we should check if there are other fields where identifiers differ. Maybe that would influence our decision making. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3172>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3AOCHEKN3LXJP2O4AGXQTPWTVZHANCNFSM4HOGVX4Q> .

[rufoa]

Just ran into another mismatch (fixing this one won't break the API – I can prepare a separate PR if you want)

The ContainerInspectData struct has a member

ImageID         string                 `json:"Image"`

but there is no corresponding Image->ImageID substitution logic for the arg to podman inspect -f. So this is like the mirror image of the problem in the OP.

We just need something like

outputFormat = strings.Replace(outputFormat, "{{.Image}}", "{{.ImageID}}", -1)

or, imho more robustly,

outputFormat = regexp.MustCompile(`\.Image(\W)`).ReplaceAllString(outputFormat, ".ImageID$1")

[mheon]

@rufoa For reference, I'm beginning a major refactor of podman inspect in #3180 as I took a deeper look after this PR and started finding some thing that did not match Docker, and others that were reporting incorrectly.

At this point, I'm thinking that matching the names to the Docker JSON is the correct thing to do - we're going to need to make significant structural changes in several places anyways, so we should try and ensure compatability.

[mheon]

I'd already included the ID -> Id change, and I'll add Image -> ImageID. If you find anything else, I think we're good to include it; as long as we can get it in relatively quickly, before we cut a 1.4 release (I'd like to get all the inspect changes in at once).

[rhatdan]

@mheon Did you make the changes?

[rh-atomic-bot]

☔ The latest upstream changes (presumably #3180) made this pull request unmergeable. Please resolve the merge conflicts.

[mheon]

Yeah, these should have landed with the inspect refactor changes

…

On Sat, Jun 8, 2019, 08:46 Atomic Bot ***@***.***> wrote: ☔️ The latest upstream changes (presumably #3180 <#3180>) made this pull request unmergeable. Please resolve the merge conflicts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3172>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3AOCEARM3WRWM6EBV2FF3PZOS25ANCNFSM4HOGVX4Q> .

[rhatdan]

Thanks @rufoa, I believe we now have your changes in master branch.

[rufoa]

Thanks @mheon - the ID/Id fix works 👍

However, the ImageID problem still seems to be present in master - please see my proposed fix for cmd/podman/inspect.go here #3172 (comment)

[rufoa]

i.e. this is what I was thinking 39f5ea4...rufoa:rufoa-patch-1