-
Notifications
You must be signed in to change notification settings - Fork 379
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
manifest/docker_schema2.go
Outdated
@@ -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, |
There was a problem hiding this comment.
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:
- Always return the config-level comment field, or
""
if it does not exist - Somehow choose between the config-level comment, and the last layer-history comment. Perhaps to be compatible with Docker.
- 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.
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a4b4c81
to
3ac2a0e
Compare
3ac2a0e
to
ee343c1
Compare
c92b433
to
e17d446
Compare
Signed-off-by: danishprakash <[email protected]>
e17d446
to
0408e3c
Compare
@mtrmac Ready to go in? |
Briefly checking…
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). |
Also the discussion in that review comment seemed to steer towards not reporting Docker-specific fields in the OCI handler… |
Adds the
comment
attribute for all image formats.Partially fixes containers/skopeo#2098