-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Cleanup content-type protobuf constants #76824
Cleanup content-type protobuf constants #76824
Conversation
@wojtek-t: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
f08c035
to
a9eb903
Compare
a9eb903
to
8c63fc9
Compare
Can you add a release note about the breaking change in the go signature? |
@@ -71,20 +71,18 @@ func IsNotMarshalable(err error) bool { | |||
// as-is (any type info passed with the object will be used). | |||
// | |||
// This encoding scheme is experimental, and is subject to change at any time. | |||
func NewSerializer(creater runtime.ObjectCreater, typer runtime.ObjectTyper, defaultContentType string) *Serializer { |
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.
@smarterclayton is there a reason for this to vary (e.g. streaming), or is this safe to hard code?
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.
This isn't vary in the current codebase (we have also RawSerializer for protobuf for streaming purpose) - I don't see any reason for having that.
But I'm happy to hear @smarterclayton opinion too.
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.
The content type used in Unknown is always content protobuf. So this should be safe if it's going into Unknown only.
// CodecFactory on initialization. | ||
contentTypeProtobuf = "application/vnd.kubernetes.protobuf" | ||
) | ||
|
||
func protobufSerializer(scheme *runtime.Scheme) (serializerType, bool) { |
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.
Wait, I removed this file. Oh wait, it hasn't merged yet. Please cherry-pick my change in #76054 that removes this file.
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.
Done
The separation is now unneeded.
8c63fc9
to
d9ea3f1
Compare
d9ea3f1
to
b48cc43
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.