Skip to content

Move containerd media type warnings for attestations#3865

Merged
jedevc merged 2 commits intomoby:masterfrom
jedevc:attestation-media-types
May 15, 2023
Merged

Move containerd media type warnings for attestations#3865
jedevc merged 2 commits intomoby:masterfrom
jedevc:attestation-media-types

Conversation

@jedevc
Copy link
Copy Markdown
Member

@jedevc jedevc commented May 12, 2023

⬆️ Follow-up to #3063

This moves the containerd suppressions for media type warnings from the
exporter package into the push and copy packages which allow them to be
used by all lower-level buildkit utilities, e.g. tests:

if err := contentutil.CopyChain(context.TODO(), ingester, provider, desc); err != nil {
return err
}
.

Previously, this would cause containerd media type warnings to be printed in test logs if an image with attestations was copied into the local image cache.

While working on this, I also noticed that we can remove our custom definition of the application/vnd.in-toto+json media type and replace it with intoto.PayloadType (which we were already using in some places, but inconsistently).

Comment thread util/contentutil/copy.go Outdated
}

if err := images.Dispatch(ctx, images.Handlers(handlers...), nil, desc); err != nil {
ctx2 := remotes.WithMediaTypeKeyPrefix(ctx, intoto.PayloadType, "intoto")
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.

Although you want to be precise is here and only change the context where it is needed it is better if this is always the first line of the function because the call is not specific to "intoto" at all. Also add a comment where this is used that this registers types that are not defined by default. Otherwise this reads like push.Push() or contentutil.Copy always register current payloadtype as "intoto".

Maybe this is better in a util pkg function, eg. RegisterContentPayloadTypes()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Although you want to be precise is here and only change the context where it is needed it is better if this is always the first line of the function because the call is not specific to "intoto" at all

Makes sense, it's a lot cleaner all at the top 😄

Maybe this is better in a util pkg function, eg. RegisterContentPayloadTypes()

Agreed, have moved this to contentutil.RegisterContentPayloadTypes().

We were using a mixture of own custom const and the vendored const. For
consistency, we should use only the vendored const everywhere.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the attestation-media-types branch from bd3b585 to 74b1d9f Compare May 15, 2023 09:23
This moves the containerd suppressions for media type warnings from the
exporter package into the push and copy packages which allow them to be
used by all lower-level buildkit utilities, e.g. tests.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the attestation-media-types branch from 74b1d9f to 86d89ac Compare May 15, 2023 09:25
@jedevc jedevc requested a review from tonistiigi May 15, 2023 09:25
@jedevc jedevc merged commit 31c870e into moby:master May 15, 2023
@jedevc jedevc deleted the attestation-media-types branch May 15, 2023 16:26
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.

3 participants