Skip to content

Conversation

@mfojtik
Copy link
Collaborator

@mfojtik mfojtik commented Oct 30, 2017

  • First patch revert new k8s JSON encoder to standard Go library encoder until we figure out why it fails to decode "\"" in DockerImage
  • Second patch wraps the legacy and grouped scheme in order for both to have conversion functions

@mfojtik
Copy link
Collaborator Author

mfojtik commented Oct 30, 2017

@deads2k the second patch requires sanity check

@deads2k
Copy link

deads2k commented Oct 30, 2017

removing jsoniter is fine. I'd like you remove all traces in a single commit and we'll try to figure out the problem later.

@liggitt as predicted...

@liggitt
Copy link

liggitt commented Oct 30, 2017

@mfojtik don't forget openshift#16065

@liggitt
Copy link

liggitt commented Oct 30, 2017

I'd prefer commenting out all references to jsoniter in the file so the import is dropped... too easy for new references to creep in without conflicts on a rebase

@mfojtik mfojtik force-pushed the rebase_1.8.1-mfojtik-serialization branch from c0b8417 to fd0ab94 Compare October 30, 2017 15:56
func init() {
// TODO: Remove this once the legacy API will be deprecated. We need this now
// in order to make the serialization test pass.
LegacySchemeBuilder.Register(RegisterConversions)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@deads2k is this acceptable? drop in/out and the serialization test is passing with this.

@mfojtik
Copy link
Collaborator Author

mfojtik commented Oct 30, 2017

@liggitt do you want to drop it entirely from k8s? remove all references, get rid of vendor/ ?

@mfojtik
Copy link
Collaborator Author

mfojtik commented Oct 30, 2017

@liggitt i think the only place where it is used right know that matters is the json serializer. There are some occurrences in _tests but nothing that we run I assume.

@mfojtik mfojtik force-pushed the rebase_1.8.1-mfojtik-serialization branch from fd0ab94 to a3cf4fe Compare October 30, 2017 16:03
@mfojtik
Copy link
Collaborator Author

mfojtik commented Oct 30, 2017

updated... I comment it out instead of removing and commented the initialisation so we don't import it anymore.

@soltysh soltysh merged commit 2705eed into soltysh:rebase_1.8.1 Oct 31, 2017
@mfojtik mfojtik deleted the rebase_1.8.1-mfojtik-serialization branch September 5, 2018 21:23
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.

4 participants