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 to go-yaml fork #5419

Closed
natasha41575 opened this issue Oct 25, 2023 · 7 comments · Fixed by #5421
Closed

Migrate to go-yaml fork #5419

natasha41575 opened this issue Oct 25, 2023 · 7 comments · Fixed by #5421
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@natasha41575
Copy link
Contributor

natasha41575 commented Oct 25, 2023

kubernetes-sigs/yaml#76 is merged now, so we can use the upstream yaml fork now.

#5412 gets rid of our internal one and uses the new fork of go-yaml.v3.

However, we are still using go-yaml.v2 in some places (see the dependency here); we should migrate to using the upstream fork instead.

@natasha41575 natasha41575 added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Oct 25, 2023
@prashantrewar
Copy link
Contributor

replacing gopkg.in/yaml.v2 v2.4.0 with gopkg.in/yaml.v3 v3.0.1 is this right?

@natasha41575
Copy link
Contributor Author

natasha41575 commented Oct 25, 2023

@prashantrewar No, we want to remove our dependency on gopkg.in/yaml libraries completely. Replace gopkg.in/yaml.v2 v2.4.0 with the new fork that kubernetes-sigs/yaml#76 introduced in sigs.k8s.yaml/goyaml.v2 1.4.0

@prashantrewar
Copy link
Contributor

/assign

@shapirus
Copy link

I suggest benchmarking kustomize before and after the change to make sure that the new fork doesn't destroy performance :)

I can help with this once the change is made or when it is available in a branch.

@natasha41575
Copy link
Contributor Author

@shapirus Please feel free to help with benchmarking (always happy to have performance tests). That said, I would be very surprised if this change impacted kustomize performance as the upstream fork's code is identical to what we are using in kustomize today, it just lives in a different place now.

@natasha41575
Copy link
Contributor Author

Adding the PR here for easy tracking: #5421

@shapirus
Copy link

Meanwhile, I can confirm (using my tests described in #5422) that PR #5421 does not introduce any performance degradation compared to release v5.2.1.

p.s. it would actually be nice to have performance regression testing as part of CI. My tests can be used almost as is for that, except that they can't into fetching specific PRs. This however can easily be added with github cli which has a specific command to "checkout a PR". If someone decides to add such a test step in the CI workflow, feel free to use and modify my scripts in any way you need for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Development

Successfully merging a pull request may close this issue.

3 participants