Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid importing multiple YAML libraries #2597

Open
AkihiroSuda opened this issue Sep 8, 2024 · 4 comments
Open

Avoid importing multiple YAML libraries #2597

AkihiroSuda opened this issue Sep 8, 2024 · 4 comments
Labels
help wanted Extra attention is needed kind/refactoring Refactoring

Comments

@AkihiroSuda
Copy link
Member

Currently, Lima imports 4 libraries of YAML 😓

lima/go.mod

Line 24 in 52f5ad3

github.com/goccy/go-yaml v1.12.0

lima/go.mod

Line 48 in 52f5ad3

gopkg.in/yaml.v3 v3.0.1

lima/go.mod

Line 123 in 52f5ad3

gopkg.in/yaml.v2 v2.4.0 // indirect

lima/go.mod

Line 130 in 52f5ad3

sigs.k8s.io/yaml v1.4.0 // indirect

Wish Go had provided stdlib for YAML...

@AkihiroSuda AkihiroSuda added help wanted Extra attention is needed kind/refactoring Refactoring labels Sep 8, 2024
@afbjorklund
Copy link
Member

afbjorklund commented Sep 9, 2024

go mod why can be helpful for this.

Some of these dependencies are indirect:

# github.com/goccy/go-yaml
github.com/lima-vm/lima/pkg/instance
github.com/goccy/go-yaml
# gopkg.in/yaml.v3
github.com/lima-vm/lima/pkg/limayaml
gopkg.in/yaml.v3
# gopkg.in/yaml.v2
github.com/lima-vm/lima/pkg/guestagent/kubernetesservice
k8s.io/api/core/v1
k8s.io/apimachinery/pkg/runtime
sigs.k8s.io/structured-merge-diff/v4/value
gopkg.in/yaml.v2
# sigs.k8s.io/yaml
github.com/lima-vm/lima/pkg/guestagent/kubernetesservice
k8s.io/client-go/tools/clientcmd
k8s.io/client-go/tools/clientcmd.test
sigs.k8s.io/yaml

Where yaml.v3 was inherited from yq.

	github.com/goccy/go-yaml v1.12.0
	gopkg.in/yaml.v3 v3.0.1

But I guess you are not keen on a 5th?

@afbjorklund
Copy link
Member

The "goccy" version of yqlib has this note:

NOTE this is still a WIP - not yet ready.

So I don't think it is ready to switch yet?

Wish Go had provided stdlib for YAML...

The informal standard is/was go-yaml (v3)

But it has the same lack of progress as json.

So that's the problem of it being "built-in"

@afbjorklund
Copy link
Member

afbjorklund commented Sep 9, 2024

We could use yqlib for verification, instead of calling gopkg.in/yaml.v3 directly...

This way, if/when yqlib changes their yaml library it would also be changed in lima?

It would not make a huge difference, but...

@@ -45,7 +45,6 @@ require (
        google.golang.org/grpc v1.66.0
        google.golang.org/protobuf v1.34.2
        gopkg.in/op/go-logging.v1 v1.0.0-20160211212156-b2cb9fa56473
-       gopkg.in/yaml.v3 v3.0.1
        gotest.tools/v3 v3.5.1
        inet.af/tcpproxy v0.0.0-20221017015627-91f861402626
        k8s.io/api v0.31.0
@@ -121,6 +120,7 @@ require (
        gopkg.in/inf.v0 v0.9.1 // indirect
        gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
        gopkg.in/yaml.v2 v2.4.0 // indirect
+       gopkg.in/yaml.v3 v3.0.1 // indirect
        gvisor.dev/gvisor v0.0.0-20231023213702-2691a8f9b1cf // indirect
        k8s.io/klog/v2 v2.130.1 // indirect
        k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect

@afbjorklund
Copy link
Member

afbjorklund commented Sep 9, 2024

The library https://github.com/kubernetes-sigs/yaml is a wrapper over yaml.v2 and yaml.v3

But it has forked those libraries, so doesn't depend on the original go modules any more:

kubernetes-sigs/yaml@b96582b

That makes it a bit harder to see the patches, but it was related to compact sequences?


EDIT: Actually, after checking with go mod vendor there doesn't seem to be any code differences...

It uses gopkg.in/yaml.v2@v2.4.0 without any modifcations - except to the README.md and OWNERS

@@ -1,3 +1,13 @@
+# go-yaml fork
+
+This package is a fork of the go-yaml library and is intended solely for consumption
+by kubernetes projects. In this fork, we plan to support only critical changes required for
+kubernetes, such as small bug fixes and regressions. Larger, general-purpose feature requests
+should be made in the upstream go-yaml library, and we will reject such changes in this fork
+unless we are pulling them from upstream.
+
+This fork is based on v2.4.0: https://github.com/go-yaml/yaml/releases/tag/v2.4.0
+
 # YAML support for the Go language
 
 Introduction

And neither of the Kubernetes (SIG) libraries have migrated to use gopkg.in/yaml.v3 yet.

It even has some opinionated views that one should use the JSON libraries to do YAML!

In short, this library first converts YAML to JSON using go-yaml and then uses json.Marshal and json.Unmarshal to convert to or from the struct. This means that it effectively reuses the JSON struct tags as well as the custom JSON methods MarshalJSON and UnmarshalJSON unlike go-yaml

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
help wanted Extra attention is needed kind/refactoring Refactoring
Projects
None yet
Development

No branches or pull requests

2 participants