-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Metadata descriptors #5454
base: master
Are you sure you want to change the base?
Metadata descriptors #5454
Conversation
ef36bb8
to
87e2f67
Compare
87e2f67
to
24317a6
Compare
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.
Check the CI errors
e1a6070
to
9825a27
Compare
10da43c
to
89e41a8
Compare
89e41a8
to
566828f
Compare
cmd/buildctl/build_test.go
Outdated
@@ -160,6 +160,13 @@ func testBuildMetadataFile(t *testing.T, sb integration.Sandbox) { | |||
require.NotEmpty(t, desc.MediaType) | |||
require.NotEmpty(t, desc.Digest.String()) | |||
|
|||
require.Contains(t, metadata, exptypes.ExporterImageDescriptorsKey) | |||
var descList []*ocispecs.Descriptor |
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.
Don't quite understand what is happening here. Why are we marshalling just to unmarshal the same value again.
After parsing, check that these are real descriptors, with real digest/size/mediatype and that the length matches.
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.
TBH, I just followed the previous block:
https://github.com/moby/buildkit/blob/master/cmd/buildctl/build_test.go#L156-L158
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.
Ok, this looks to be (convoluted) way to convert interface{}
backed by map to a typed struct. Usually, a better approach would be to parse into map[string]json.RawMessage
.
If you add a comment for this, then you can leave the conversion as is but do add the checks to make sure that the descriptors are real and contain correct values. Ideally, I think this should also be checked for both single-arch and multi-arch results as looks that the expected output is different.
idx.Manifests = append(idx.Manifests, *desc) | ||
mfstDesc.Platform = &dp | ||
idx.Manifests = append(idx.Manifests, *mfstDesc) | ||
descriptors = append(descriptors, mfstDesc, configDesc) |
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.
Shouldn't this be just mfstDesc
. Mixing manifest and config descriptors together does not make a predictable result. And how would one even connect the ones that are for the same image.
Testing For single arch build I get:
So there isn't much difference between old field and new one. Except for the config descriptor. Multi-arch:
This one has root manifest (duplicate), single-arch manifests, configs for single-arch, attestations manifest, (but no configs for attestations). I guess my initial thought would have been that the old field contains the descriptor of the root, and new field contains the descriptors immediately under the root (without the root itself and without the configs). I don't know if that helps with the use-case you are trying to use it though, but atm. there doesn't seem to be a very clear definition of which descriptors are appended to the result and which are skipped. And as mentioned in the previous comment, it isn't very clear how these additional config descriptors should be used as it cannot be determined what manifest or platform they belong to. |
Its usefulness is more obvious when using attestation manifest and tags. Here is the output I get for a single arch
|
25ffd37
to
ba57483
Compare
Signed-off-by: Laurent Goderre <laurent.goderre@docker.com>
ba57483
to
d2d8e47
Compare
This PR allows outputting all the descriptors in the metadata file. This is how it would look: