-
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
Rename RawJSON to Raw in runtime.RawExtension and add ContentType & C… #23112
Merged
k8s-github-robot
merged 1 commit into
kubernetes:master
from
wojtek-t:rename_raw_json_in_raw_extension
Mar 18, 2016
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So adding ContentEncoding / Type is interesting - when this object is included in an external type for JSON, it's assumed to be JSON (and there is no way to set encoding or content type). For Protobuf, it will be slightly different (the nested object would be a runtime.Unknown, but ContentEncoding and ContentType would be inside that object, not this one). For serializing nested objects (like List) we've said that that has to be something that serializer knows how to do, so the serializer is responsible for transforming Object -> Raw and vice versa.
Do we need to add them here? Maybe I didn't catch why they were required.
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.
So maybe I don't fully understand it.
My rationale for having it is that it seemed to be that in some cases there wasn't any way to infer that information if we don't have them in RawExtension - as examples see above:
But maybe I'm missing how RawExtensions generally works...
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.
We're trying to remove the conversion logic in pkg/runtime/embedded.go (conversions doesn't know enough about the context that a serialization is happening in to properly convert arbitrary objects to bytes). In OpenShift we have more cases where we use embedded RawExtensions, so we hit the use cases more. @deads2k and I ended up having an extension to the serializer that gave objects the chance to encode their nested objects with a copy of the serializer, prior to actually writing out the actual object. In that case, the encoding and type would be handled by the serializer code.
Example - we have a
List
object that has runtime.Objects. Those objects don't have to be in the same API group, or be targeted to the same API version. When we read the List, we don't decode them immediately (we just move them to unknown). The serializer that reads those knows what content type is the default - conversion does not. Likewise, when we write those to protobuf, the protobuf serializer knows that the destination content type is protobuf, not conversion. So the hooks in the serializer have to go and give each object a chance to serialize using protobuf.If we get rid of internal versions, we'd need a way to pass around objects that have been decoded (that's what Object is for). I'm not sure that we'd need content encoding or type.
I think line 102 of embedded up above is technically wrong (where we convert unknown to raw extension), because we don't want conversion doing encoding. The problem is we need to formalize how we do encoding of nested objects, probably as part of this chain of work. Since Unknown is a wrapper, rather than an object on its own, we should probably do nothing.
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.
Thanks for detailed explanation - that helps a lot!
So I guess, we should remove ContentEncoding and ContentType from here, right?
Will do that tomorrow.
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.
Yeah, let's remove and add a TODO: here and a follow up issue to determine
what the solution is. I'll open a PR on the "let callers encode themselves
from serializers" side.
On Thu, Mar 17, 2016 at 4:43 PM, Wojciech Tyczynski <
notifications@github.com> wrote: