enhance skopeo inspect #1608

ningmingxiao · 2022-07-12T14:25:57Z

Signed-off-by: ningmingxiao ning.mingxiao@zte.com.cn

ningmingxiao · 2022-07-12T14:28:11Z

skopeo inspect can't see whether an image's MediaType,I want to use MediaType to check whether it is enctryed.

mtrmac

Thanks for the PR.

c/image is following the Go semantic versioning rules, so we must not incompatibly change the externally-visible API like this (and we have no appetite for increasing the major version number either).

This needs to be done by adding new fields, without modifying any of the existing ones.

manifest/common.go

types/types.go

manifest/oci.go

mtrmac

This PR keeps growing…

While the additions are certainly valuable, perhaps it might be better to get one feature done, and then add others similarly, in one or more smaller PRs.

mtrmac · 2022-07-19T12:25:17Z

types/types.go

+	History       []v1.History
+	Author        string
+	Size          int64
+	Config        v1.ImageConfig


Adding a full config here is redundant with Image.OCIConfig, and probably unwanted for the Inspect caller, who is calling Inspect exactly not to deal with the details of the config format.

I use podman or docker inspect an image, they will show config, so I think we should stay the same with docker or podman

But then are other callers like skopeo inspect, or automation that just wants a single element (like looking up a single label) that don’t need both at the same time. Paying the cost of the full conversion and JSON formatting is not all that great for them.

As a maintainer, I’d also much prefer not to have to maintain logic to convert to OCI format in two separate implementations for each format.

mtrmac · 2022-07-19T12:25:54Z

types/types.go

+	Config        v1.ImageConfig
+}
+
+type LayerDetail struct {


ImageInspectLayer?

I think ImageInspectLayer and Layers are same meaning, at ImageInspectInfo struct

The types subpackage contains quite a few types … and it’s useful to make it clear that the purpose of this struct is only as an inspect output.

We already have types.BlobInfo (and manifest.LayerInfo); being clear about the purposes of each (especially when there is no written documentation) is helpful to users of the API.

(Especially when may users are just going to use Inspect().LayersDetail… without having to name the type, we need the type name more to be unambiguous than we need it to be short.)

mtrmac · 2022-07-19T12:27:19Z

types/types.go

@@ -466,6 +466,18 @@ type ImageInspectInfo struct {
 	Os            string
 	Layers        []string
 	Env           []string
+	LayersDetail  []LayerDetail
+	History       []v1.History


As this very PR shows, it’s valuable to be able to add extra fields — so a separate ImageInspectHistory where we can add fields to would be better than forever tying this to OCIv1, and potentially having to add a HistoryDetail.

you means I should use another func ImageInspectHistory to show history? When I use podman inspect an image, I can see history

I mean that the type of the field should be History []ImageInpectHistory.

mtrmac · 2022-07-19T12:30:03Z

manifest/oci.go

 		Env:           v1.Config.Env,
+		History:       v1.History,
+		Author:        v1.Author,
+		Size:          d1.Size,


Size is not defined in OCI — so the ImageInspectInfo documentation for the field must document how “unknown” is represented, and this code needs to set that value.

(0 is not a good “unknown” value for a size field, in principle… We typically use -1.)

Still outstanding: The -1 value needs documenting.

manifest/docker_schema2.go

internal/image/docker_schema2.go

manifest/docker_schema1.go

manifest/oci.go

mtrmac · 2022-07-19T12:41:27Z

types/types.go

@@ -466,6 +466,18 @@ type ImageInspectInfo struct {
 	Os            string
 	Layers        []string
 	Env           []string
+	LayersDetail  []LayerDetail
+	History       []v1.History


For History, we need to worry about promised semantics, if any; e.g. is the number of entries supposed to match the number of layers? (That needs to be very careful about the EmptyLayer bit in OCI — so the number of non-empty layers?) Is “unknown” a possible value?

Outstanding.

mtrmac · 2022-07-19T12:42:09Z

types/types.go

+	LayersDetail  []LayerDetail
+	History       []v1.History
+	Author        string
+	Size          int64


Size of what? The compressed representation? Uncompressed representation? Either, depending on circumstances?

In OCI this can apparently be unknown.

Still outstanding; the added comment doesn’t answer this question.

mtrmac · 2022-07-19T12:48:04Z

Unit test coverage, to make sure this does not accidentally break, would also be nice. (And, looking at how the existing tests fail, it’s in a sense unavoidable at least to that little extent.)

Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>

mtrmac · 2022-07-20T16:40:47Z

internal/image/docker_schema1_test.go

+		assert.Equal(t, Env, ii.Env)
+		assert.NotEqual(t, nil, ii.Config)
+		assert.Equal(t, int64(0), ii.Size)
+		assert.Equal(t, "", ii.Author)


Please maintain the original whole-structure comparison.

A single Equal over the whole structure is more useful because it ensures that nothing was left out in the tests. Doing this item by item requires much more manual inspection and maintenance.

(In all three formats.)

mtrmac · 2022-07-20T16:41:17Z

internal/image/docker_schema1_test.go

+		assert.Equal(t, int64(0), ii.Size)
+		assert.Equal(t, "", ii.Author)
+		for _, layer := range ii.LayersDetail {
+			assert.NotEqual(t, nil, layer)


AFAICS that’s always true (members of the slice are not pointers at all), and does not test the created contents at all. That’s not useful.

Same for the other loop.

mtrmac · 2022-07-20T16:47:08Z

types/types.go

+	History       []v1.History
+	Author        string
+	Size          int64
+	Config        v1.ImageConfig


But then are other callers like skopeo inspect, or automation that just wants a single element (like looking up a single label) that don’t need both at the same time. Paying the cost of the full conversion and JSON formatting is not all that great for them.

As a maintainer, I’d also much prefer not to have to maintain logic to convert to OCI format in two separate implementations for each format.

mtrmac · 2022-07-20T16:56:24Z

manifest/common.go

+func schema2HistoryToV1History(history []Schema2History) []v1.History {
+	v1History := make([]v1.History, len(history))
+	for index, value := range history {
+		v1History[index].Created = &value.Created


created := value.Created v1History[index].Created = &created

would be more obviously safe, because the pointer points at an individually-allocated value that will never be modified by whatever owns the input history value.

mtrmac · 2022-07-20T16:58:25Z

manifest/oci.go

 	i := &types.ImageInspectInfo{
 		Tag:           "",
 		Created:       v1.Created,
 		DockerVersion: d1.DockerVersion,
 		Labels:        v1.Config.Labels,
 		Architecture:  v1.Architecture,
 		Os:            v1.OS,
-		Layers:        layerInfosToStrings(m.LayerInfos()),
+		Layers:        layerInfosToStrings(layerInfos),


Please keep the order of fields in the initializer the same as in the type definition, to make it easier to tell which fields were / were not set. (Everywhere.)

mtrmac · 2022-07-20T17:03:23Z

manifest/docker_schema1.go

 	}
+
+	diffIDs := []digest.Digest{digest.FromString(s1.ID)}


s1.ID is not a diffID, and the single-value array would frequently not even have the right number of elements.

Passing an empty value would at least be consistent with the existing OCIConfig call.

(This is another reason not to include all of OCIv1 config in the inspect output: the value is not fully valid, and the OCIConfig API is focused enough on this single thing that it can document that at least a bit more effectively.)

Actually the best thing to do is not to involve the schema2 JSON format at all: refactor the convertedHistory computation out from ToSchema2Config into a separate function, and call it here. Then

We don’t need to invent fake DiffID values

ToSchema2Config avoids a whole lot of JSON manipulation

This code can just consume convenient Go values instead of having to unmarshal again.

mtrmac · 2022-07-20T17:07:05Z

manifest/docker_schema1.go

+	if err != nil {
+		return nil, err
+	}
+	imageConfig := &schema2Image{}


(Why is this using a custom type and not the existing schema2 manifest.Schema2Image? But see elsewhere, we shouldn’t need to involve JSON for this.)

mtrmac · 2022-07-20T17:10:03Z

types/types.go

@@ -466,6 +466,19 @@ type ImageInspectInfo struct {
 	Os            string
 	Layers        []string
 	Env           []string
+	LayersDetail  []LayerDetail


I think we can keep LayersDetail right next to Layers.

TomSweeneyRedHat · 2022-07-23T20:54:09Z

@ningmingxiao needs a rebase

mtrmac · 2022-08-08T13:55:57Z

#1626 follows up on this PR (with a subset of the functionality).

mtrmac requested changes Jul 12, 2022

View reviewed changes

manifest/common.go Outdated Show resolved Hide resolved

types/types.go Outdated Show resolved Hide resolved

types/types.go Outdated Show resolved Hide resolved

types/types.go Outdated Show resolved Hide resolved

types/types.go Outdated Show resolved Hide resolved

ningmingxiao force-pushed the dev2 branch 12 times, most recently from 7bf77ee to 831eccc Compare July 19, 2022 10:45

mtrmac reviewed Jul 19, 2022

View reviewed changes

manifest/oci.go Outdated Show resolved Hide resolved

ningmingxiao force-pushed the dev2 branch 2 times, most recently from d913113 to 2defada Compare July 19, 2022 12:02

mtrmac reviewed Jul 19, 2022

View reviewed changes

ningmingxiao force-pushed the dev2 branch 2 times, most recently from dbf036e to 7b09f2f Compare July 20, 2022 02:11

enhance skopeo inspect

53278c4

Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>

ningmingxiao force-pushed the dev2 branch from 7b09f2f to 53278c4 Compare July 20, 2022 02:34

mtrmac reviewed Jul 20, 2022

View reviewed changes

TomSweeneyRedHat added the invalid label Jul 23, 2022

This was referenced Jul 28, 2022

RFE: Make 'skopeo inspect' provide the size of the OCI image containers/skopeo#641

Closed

equivalent of docker history containers/skopeo#1309

Open

enhance inspect #1626

Merged

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance skopeo inspect #1608

enhance skopeo inspect #1608

ningmingxiao commented Jul 12, 2022

ningmingxiao commented Jul 12, 2022

mtrmac left a comment

mtrmac left a comment

mtrmac Jul 19, 2022

ningmingxiao Jul 20, 2022 •

edited

mtrmac Jul 20, 2022

mtrmac Jul 19, 2022

ningmingxiao Jul 20, 2022 •

edited

mtrmac Jul 20, 2022

mtrmac Jul 19, 2022

ningmingxiao Jul 20, 2022

mtrmac Jul 20, 2022

mtrmac Jul 19, 2022

mtrmac Jul 20, 2022

mtrmac Jul 19, 2022

mtrmac Jul 20, 2022

mtrmac Jul 19, 2022

mtrmac Jul 20, 2022

mtrmac commented Jul 19, 2022

mtrmac Jul 20, 2022 •

edited

mtrmac Jul 20, 2022 •

edited

mtrmac Jul 20, 2022

mtrmac Jul 20, 2022

mtrmac Jul 20, 2022

mtrmac Jul 20, 2022

mtrmac Jul 20, 2022

mtrmac Jul 20, 2022

TomSweeneyRedHat commented Jul 23, 2022

mtrmac commented Aug 8, 2022

enhance skopeo inspect #1608

Are you sure you want to change the base?

enhance skopeo inspect #1608

Conversation

ningmingxiao commented Jul 12, 2022

ningmingxiao commented Jul 12, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningmingxiao Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningmingxiao Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Jul 19, 2022

mtrmac Jul 20, 2022 • edited

Choose a reason for hiding this comment

mtrmac Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jul 23, 2022

mtrmac commented Aug 8, 2022

ningmingxiao Jul 20, 2022 •

edited

ningmingxiao Jul 20, 2022 •

edited

mtrmac Jul 20, 2022 •

edited

mtrmac Jul 20, 2022 •

edited