Skip to content
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

sparse: do not modify media type when unspecified #233

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

jabrown85
Copy link
Contributor

The media type should only be modified when the user specifies it. In my case I noticed the digest given by the sparse.Image was not the same as the image it was dervied from. This was because the logic to set the media type defaulted to overriding to OCI if the media type was not specified. This commit changes the logic to only override the media type if the user specifies it.

The media type should only be modified when the user specifies it. In my case I noticed the digest given by the sparse.Image was not the same as the image it was dervied from. This was because the logic to set the media type defaulted to overriding to OCI if the media type was not specified. This commit changes the logic to only override the media type if the user specifies it.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
@jabrown85 jabrown85 self-assigned this Nov 2, 2023
@jabrown85 jabrown85 requested a review from a team as a code owner November 2, 2023 22:52
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
layout/new.go Outdated
@@ -58,7 +59,7 @@ func NewImage(path string, ops ...ImageOption) (*Image, error) {
ri.createdAt = imageOpts.createdAt
}

if imageOpts.mediaTypes == imgutil.MissingTypes {
if imageOpts.mediaTypes == imgutil.MissingTypes && !hasBaseImage {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. The idea is that if layout.WithMediaTypes is omitted, the imageOpts.mediaTypes will be imgutil.MissingTypes. In the case where there was a base image (always true in sparse, not always true in layout) - we should default to matching the base image.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Copy link
Member

@jjbustamante jjbustamante left a comment

Choose a reason for hiding this comment

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

Great job @jabrown85! This looks good to me!

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Are we going to need to change the lifecycle at all? Or should that be covered by "has base image"?

@jabrown85
Copy link
Contributor Author

I don't think lifecycle is aware of sparse vs layout at all today and this should change nothing there.

@jjbustamante
Copy link
Member

Are we going to need to change the lifecycle at all? Or should that be covered by "has base image"?

As I mentioned on Slack:

  • Originally I was always overriding the media types to use OCI because tooling like umoci, which I used to verify the image was working, didn't recognize docker media types.
  • I would like to add a flag similar to skopeo copy to allow users to override the media type if they want to:
-f, --format string                         MANIFEST TYPE (oci, v2s1, or v2s2) to use in the destination (default is manifest type of source, with fallbacks)
  • In this case, I think we will need to make some changes in lifecycle and pack to pass through the media-type option, but I think this can just be an improvement.

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