add comment attribute to inspect

[danishprakash]

Adds the comment attribute for all image formats.

Partially fixes containers/skopeo#2098

[mtrmac]

Thanks! I’m afraid the tests seem to need further updates.

[mtrmac]

Comparing this with the OCI implementation, and what Docker does… there are multiple ways this could be implemented.

Most importantly, whatever we decide to implement, the field in types.ImageInspectInfo needs to document the semantics.

There are basically three options:

1. Always return the config-level comment field, or "" if it does not exist
2. Somehow choose between the config-level comment, and the last layer-history comment. Perhaps to be compatible with Docker.
3. Always return the last layer-history comment. I would argue this one does not make sense — if we want to look at layer-history comments, we should provide all of them in individual ImageInspectLayer fields.

This seems to implement 2, but AFAICS it does not quite follow what Docker does for schema2 images. docker import, AFAICS, sets both the config-level comment and the comment for the last history layer to the same string, but other ways to create images, like docker commit and docker build, only update the layer-history comments, and don’t set the config-level one at all. (For a random example, see skopeo --override-os linux --override-arch amd64 inspect --config --raw docker://ghcr.io/mkst/compilers/nds_arm9/mwcc_30_114:latest.) So, for approach 2. to get the same output as docker inspect, this should fall back to the layer-history comment if the config-level one is empty.

[danishprakash]

Thanks for clearing that up. I realized this wasn't right while looking into the tests.

	comment := s2.Comment
	if len(comment) == 0 && len(s2.History) > 0 {
		comment = s2.History[len(s2.History)-1].Comment
	}

[danishprakash]

On a side note, the reason we're populating the OCI inspect info using both OCI Image type and Docker's Schema2V1 is for backward compatibility? Would this implementation eventually be relying entirely on image-spec if and when those attributes are added to image-spec?

[mtrmac]

The OCI1.Inspect code only currently uses the Docker type to set DockerVersion; I think it’s quite unlikely that this field will be added to OCI in the future.

I default to thinking that’s a historical anomaly, not an example to follow for new features. Pragmatically, it depends on whether we see “OCI” images with a non-OCI “comment” field in the wild — but unless we do see them in the wild, we should start by just implement the spec and not introducing unnecessary divergence.

Do you know of any OCI images with a Docker-like comment field?

Most importantly, whatever we decide to implement, the field in types.ImageInspectInfo needs to document the semantics.

… is still a design aspect I wonder about.

[danishprakash]

Sorry for the multi month delay

---

we should start by just implement the spec and not introducing unnecessary divergence.

Fair enough, although for Inspect, it shouldn't be a concern as such since we're reading the manifest and the Inspect structure is specific to skopeo or consumers of c/image afaict.

Do you know of any OCI images with a Docker-like comment field?

If you're asking If I came across any in the wild, then I'd say I never did or I never paid attention to any If I did unfortunately.

[rhatdan]

@mtrmac Ready to go in?

[mtrmac]

Briefly checking…

Most importantly, whatever we decide to implement, the field in types.ImageInspectInfo needs to document the semantics.

With that missing, I don’t think I have read what the test fixture changes actually do. I’d eventually want to check that (but I would be very surprised if something in there ended up being a blocker).

[mtrmac]

Also the discussion in that review comment seemed to steer towards not reporting Docker-specific fields in the OCI handler…