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

Migrate from go yaml.v2 to yaml.v3 #4178

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Migrate from go yaml.v2 to yaml.v3 #4178

merged 1 commit into from
Jan 31, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

This PR simply updates a piece of code using the yaml version 2 instead of v3. Since it has some breaking changes, it is a separate PR.

Benefits

Avoid multiple dependency versions in our codebase.

Possible drawbacks

Some additional and unnoticed breaking changes? Let's see what the CI thinks.

Applicable issues

Additional information

This is one of the few places we are using the go yaml library instead of the k8s one. In this piece of code, we don't have any typed model to marshal from/to, so I guess that's the reason.
However, I wonder about this decision, I mean, why not rely on a PodSpec since it is a well-established k8s API (corev1)?
Happy to discuss it offline in our standup, though.

@absoludity
Copy link
Contributor

This is one of the few places we are using the go yaml library instead of the k8s one. In this piece of code, we don't have any typed model to marshal from/to, so I guess that's the reason.
However, I wonder about this decision, I mean, why not rely on a PodSpec since it is a well-established k8s API (corev1)?

I'm not 100% about "the" reason (it's been a while), but a post renderer takes a byte array of the generated manifests and returns a byte array of the mutated manifests. These are arbitrary yaml manifests, not specific pod specs or similar. But if there's a way to parse that with a typed parser and pull out just the pod specs, that'd be excellent. Perhaps a parser that just parses the standard metadata, then you can just deal with the know (typed) resources that you're interested in.

Happy to discuss it offline in our standup, though.

+1

@antgamdia
Copy link
Contributor Author

But if there's a way to parse that with a typed parser and pull out just the pod specs, that'd be excellent. Perhaps a parser that just parses the standard metadata, then you can just deal with the know (typed) resources that you're interested in.

Thanks for the explanation!

As I mentioned in #4175 (comment), I wonder if using the built-in decoder in k8s UniversalDecoder() + DecodeInto(...) [1] would be useful here, haven't tried though.

[1] https://stackoverflow.com/a/63338697

@absoludity
Copy link
Contributor

But if there's a way to parse that with a typed parser and pull out just the pod specs, that'd be excellent. Perhaps a parser that just parses the standard metadata, then you can just deal with the know (typed) resources that you're interested in.

Thanks for the explanation!

As I mentioned in #4175 (comment), I wonder if using the built-in decoder in k8s UniversalDecoder() + DecodeInto(...) [1] would be useful here, haven't tried though.

[1] https://stackoverflow.com/a/63338697

Hmm, maybe? Haven't played with it, but DecodeInto is the typed version, which assumes you know the type which the post-processor doesn't. There is also there Object.Decode, but then you're back with a non-typed object. Universal decoder might be removed, but could help - not sure. But yep, happy if there's a better way to do so with these.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia antgamdia changed the base branch from unifyImports-2 to main January 31, 2022 15:23
@antgamdia antgamdia merged commit dacc11c into main Jan 31, 2022
@antgamdia antgamdia deleted the unifyImports-3.1 branch January 31, 2022 16:36
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.

None yet

2 participants