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

Updated media type to a format compatible with OCI registries #279

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

kommendorkapten
Copy link
Member

Note that i did not opt for bumping the version in the media type, only changing the encoding. I would be very interesting to hearing the pros/cons. As there are no semantical changes, I kept the semantic versioning. But I'm struggling to come to a resolution if that was a good or bad move. If we bump the semantic versions, some parts of the code will be more complicated client side. I'm easy to convince the semantic version should be bumped if there are more concrete use-cases where it would be simpler 😅

Summary

Closes #165

Release Note

Added a new format for the media type that is compatible with OCI registries

Documentation

N/A

@woodruffw
Copy link
Member

LGTM overall!

I'm also divided on bumping the version here:

  • On one hand, not bumping here will result in bundles/trust roots that clients will reject if (like sigstore-python) they perform strict matching on the media type.
  • On the other, this isn't really a breaking change in the underlying formats themselves, and bumping the version contributes to version fatigue here.

@kommendorkapten
Copy link
Member Author

On the other, this isn't really a breaking change in the underlying formats themselves, and bumping the version contributes to version fatigue here.

This is exactly the one I tried to avoid :)

But yes, agree with all you said @woodruffw

Copy link
Collaborator

@bdehamer bdehamer left a comment

Choose a reason for hiding this comment

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

I agree. Leaving the version at 0.3 feels like the right thing to do here.

When we publish the new protobuf packages, I assume we'll treat this as a patch update to the v0.3.x family, yeah?

@haydentherapper
Copy link
Collaborator

Are there any clients generating v0.3 bundles yet?

@bdehamer
Copy link
Collaborator

In sigstore-js, we can verify a v0.3 bundle, but don't yet generate them.

Copy link
Collaborator

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

LGTM. Only question is if we have any v3 bundles floating about. If so, then this LGTM. If not, then we could change the v3 string without having to support the previous version.

@haydentherapper
Copy link
Collaborator

sigstore-go isn't generating v3 bundles.

@kommendorkapten
Copy link
Member Author

What about java and python, do you produce v0.3 bundles yet? @woodruffw @loosebazooka

@loosebazooka
Copy link
Member

I think it's okay to keep it at v3 for java. It's committed in code, but unused.

@woodruffw
Copy link
Member

I think it's okay to keep it at v3 for java. It's committed in code, but unused.

Ah yeah, I forgot that nobody has really rolled out v3 bundle support yet. Given that, I think we're safe to not bump the version here!

@haydentherapper
Copy link
Collaborator

Do we need to support application/vnd.dev.sigstore.bundle+json;version=0.3, or could we remove that string so we don't have two ways of representing v3 bundles?

@kommendorkapten
Copy link
Member Author

Do we need to support application/vnd.dev.sigstore.bundle+json;version=0.3, or could we remove that string so we don't have two ways of representing v3 bundles?

That is a very tempting idea!

We do have a release/tag v0.3.0 out which specifies the version=0.3. So if we remove the old version 0.3, should we remove that release? And is it used? If we remove it It would be a breaking change? So I'm leaning towards keeping it.

The language we have now is:

        // MUST be application/vnd.dev.sigstore.bundle.v0.3+json when
        // when encoded as JSON.
        // Clients must to be able to accept media type using the previously
        // defined formats:
        // * application/vnd.dev.sigstore.bundle+json;version=0.1
        // * application/vnd.dev.sigstore.bundle+json;version=0.2
        // * application/vnd.dev.sigstore.bundle+json;version=0.3

To minimize the risk of breaking anything I think we should keep as is? It's unlikely, but someone can be using the v3 version in some private scenario and I would hate to make their lives harder.

@woodruffw
Copy link
Member

Removing version=0.3 is temping, but I don't think we should muddle with the version history 😅

IMO, we can do the following

  1. Tweak the language to say "v3 bundles MUST use the new version format, but clients MAY allow the old format for compatibility"
  2. Release 0.3.1 with the changes

In practice, this should have no compatibility concerns, since no clients are generating v3 bundles at the moment.

@kommendorkapten
Copy link
Member Author

I think we are more or less saying the same @woodruffw, but I think the language we have is enough? But yes, v0.3.1 tag is what I would propose too (but didn't write that) 😄

@woodruffw
Copy link
Member

I think we are more or less saying the same @woodruffw, but I think the language we have is enough? But yes, v0.3.1 tag is what I would propose too (but didn't write that) 😄

Yes, I think so!

@haydentherapper
Copy link
Collaborator

SGTM, agreed we shouldn't yank a release.

@haydentherapper
Copy link
Collaborator

Merging since there's 3 LGTMs

@haydentherapper haydentherapper merged commit 0d09353 into sigstore:main Apr 1, 2024
24 checks passed
@kommendorkapten kommendorkapten deleted the new-media-type branch April 2, 2024 05:47
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.

Bundle Media Type
5 participants