Skip to content

tests: add imagetools tests for copying manifests and indexes - #2013

Merged
jedevc merged 1 commit into
docker:masterfrom
jedevc:tests-add-imagetools-create
Aug 23, 2023
Merged

jedevc merged 1 commit into
docker:masterfrom
jedevc:tests-add-imagetools-create

Conversation

@jedevc

@jedevc jedevc commented Aug 22, 2023

Copy link
Copy Markdown
Collaborator

While in the process of investigating, #1660, realized we didn't have any tests for it.

I've added some tests to ensure that manifests and indexes are copied with their digests preserved intact.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
jedevc requested a review from crazy-max August 22, 2023 16:39
Comment thread tests/imagetools.go
Comment on lines +61 to +65
var idx2 ocispecs.Index
err = json.Unmarshal(dt, &idx2)
require.NoError(t, err)
require.Equal(t, images.MediaTypeDockerSchema2ManifestList, idx2.MediaType)
require.Equal(t, 1, len(idx2.Manifests))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Out of curiosity: is this case actually correct? If the imagetools create input is only a single manifest, do we expect to create an index containing just it? Or would we expect to just copy it unchanged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hum I would say we just copy it unchanged imo. It looks linked to #1797 (comment) where we default to OCI now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I don't think this is oci/docker media type confusion, it seems like this was always present before that PR?

I think maybe we want the just copy behaviour agreed, I think it's worth merging this PR to track the current behaviour, then fixing that as a followup, if we agree that's worth doing?

@crazy-max crazy-max Aug 23, 2023 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I don't think this is oci/docker media type confusion, it seems like this was always present before that PR?

Ah right indeed this is for manifest list / index case.

I think maybe we want the just copy behaviour agreed,

I took a further look and we already return the original manifest for a single source:

// on single source, return original bytes
if len(srcs) == 1 && len(ann) == 0 {
if mt := srcs[0].Desc.MediaType; mt == images.MediaTypeDockerSchema2ManifestList || mt == ocispec.MediaTypeImageIndex {
return dts[0], srcs[0].Desc, nil
}
}

But seems we hit this case instead:

var mfst ocispec.Index
if err := json.Unmarshal(dts[i], &mfst); err != nil {
return nil, ocispec.Descriptor{}, errors.WithStack(err)
}
for _, d := range mfst.Manifests {
addDesc(d)
}

I think it's worth merging this PR to track the current behaviour, then fixing that as a followup, if we agree that's worth doing?

Yes sounds good for a follow-up 👍

@jedevc
jedevc merged commit 3bd4bca into docker:master Aug 23, 2023
@jedevc
jedevc deleted the tests-add-imagetools-create branch August 23, 2023 12:31
@crazy-max crazy-max mentioned this pull request Apr 5, 2024
19 of 35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants