Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add comment attribute to inspect #2127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danishprakash
Copy link
Contributor

@danishprakash danishprakash commented Sep 21, 2023

Adds the comment attribute for all image formats.

Partially fixes containers/skopeo#2098

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

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

@@ -281,6 +281,7 @@ func (m *Schema2) Inspect(configGetter func(types.BlobInfo) ([]byte, error)) (*t
Layers: layerInfosToStrings(layerInfos),
LayersData: imgInspectLayersFromLayerInfos(layerInfos),
Author: s2.Author,
Comment: s2.Comment,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@danishprakash danishprakash force-pushed the inspect-comment branch 3 times, most recently from a4b4c81 to 3ac2a0e Compare September 27, 2023 13:22
@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Oct 10, 2023
@danishprakash danishprakash force-pushed the inspect-comment branch 2 times, most recently from c92b433 to e17d446 Compare January 3, 2024 10:39
@rhatdan
Copy link
Member

rhatdan commented Feb 22, 2024

@mtrmac Ready to go in?

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 22, 2024

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
Copy link
Collaborator

mtrmac commented Feb 22, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment section from inspect
3 participants