Skip to content

[chore]: migrate from sigs.k8s.io/yaml to goccy/go-yaml #3997

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

Merged
merged 3 commits into from
May 24, 2025

Conversation

LucaLanziani
Copy link
Contributor

Description:

Replace unmaintenance yaml library sigs.k8s.io/yaml with https://github.com/goccy/go-yaml

Link to tracking Issue(s):

Testing:

make test

@LucaLanziani LucaLanziani requested a review from a team as a code owner May 15, 2025 17:11
@LucaLanziani LucaLanziani changed the title feat: migrate from sigs.k8s.io/yaml to goccy/go-yaml [chore]: migrate from sigs.k8s.io/yaml to goccy/go-yaml May 15, 2025
@LucaLanziani
Copy link
Contributor Author

I'll check these in a moment

@LucaLanziani LucaLanziani force-pushed the issue/3988-go-yaml branch 2 times, most recently from ba7a1cb to 2e2b226 Compare May 16, 2025 18:54
@iblancasa iblancasa self-requested a review May 19, 2025 13:06
@iblancasa
Copy link
Contributor

@LucaLanziani can you take a look to the CI?

@LucaLanziani
Copy link
Contributor Author

@iblancasa sure, I'll fix the remaining test this evening, I assumed that make e2e was running all tests but apparently it doesn't.

@LucaLanziani
Copy link
Contributor Author

@iblancasa I don't know if you have seen my comment on the issue
There other instances of the package yaml.v2

I believe we should target these as part of this PR

cmd/operator-opamp-bridge/internal/config/config.go:    "gopkg.in/yaml.v2"
cmd/otel-allocator/internal/config/config.go:   "gopkg.in/yaml.v2"
cmd/otel-allocator/internal/server/server.go:   "gopkg.in/yaml.v2"
cmd/otel-allocator/internal/server/server_test.go:      "gopkg.in/yaml.v2"
cmd/otel-allocator/internal/watcher/promOperator.go:    "gopkg.in/yaml.v2"
internal/manifests/collector/adapters/config_from.go:   "gopkg.in/yaml.v2"
internal/manifests/collector/config_replace_test.go:    "gopkg.in/yaml.v2"
internal/manifests/opampbridge/configmap.go:    "gopkg.in/yaml.v2"
internal/manifests/targetallocator/configmap.go:        "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_19_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_24_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_31_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_36_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_38_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_39_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_43_0.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_57_2.go:       "gopkg.in/yaml.v2"
pkg/collector/upgrade/v0_9_0.go:        "gopkg.in/yaml.v2"

@LucaLanziani
Copy link
Contributor Author

Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

I guess we need to adapt all the e2e tests in that case 🙈

@@ -7,7 +7,7 @@ import (
"errors"
"fmt"

"gopkg.in/yaml.v3"
go_yaml "github.com/goccy/go-yaml"
Copy link
Member

Choose a reason for hiding this comment

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

(nit)

Suggested change
go_yaml "github.com/goccy/go-yaml"
yaml "github.com/goccy/go-yaml"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better for this to consistently be go_yaml everywhere. When I see yaml, I assume it's gopkg.in/yaml, so a different alias seems useful to indicate that it's not.

@LucaLanziani
Copy link
Contributor Author

LucaLanziani commented May 22, 2025

After looking at the indentation here once more https://github.com/open-telemetry/opentelemetry-operator/actions/runs/15075431358/job/42467505446?pr=3997#step:8:430

I realized that the github.com/goccy/go-yaml library is right and the format in the test here is wrong.

If we use two spaces for the following:

{"readers":[{"pull":{"exporter":{"prometheus":{"host":"0.0.0.0","port":8888}}}}]}

looks like:

readers:
  - pull:
      exporter:
        prometheus:
          host: 0.0.0.0
          port: 8888

so indenting by four can only be:

readers:
    - pull:
          exporter:
              prometheus:
                  host: 0.0.0.0
                  port: 8888

but in the test you see we have a mix of the two.

You can see the difference between the two libraries here: https://go.dev/play/p/fo6MEYZyIuy

So ideally we should fix the indentation in the tests but I believe @swiatekm you said that doing that will cause an unnecessary reconciliation for users.

@LucaLanziani
Copy link
Contributor Author

@swiatekm
Copy link
Contributor

So ideally we should fix the indentation in the tests but I believe @swiatekm you said that doing that will cause an unnecessary reconciliation for users.

If the current output is incorrect and difficult to reproduce, then we should bite the bullet and change it.

@swiatekm swiatekm enabled auto-merge (squash) May 23, 2025 12:50
@LucaLanziani
Copy link
Contributor Author

I'll create a second issue to replace all other yaml libraries.

@LucaLanziani
Copy link
Contributor Author

I might be doing something wrong but I'm not able to reproduce this error on my machine.

@swiatekm
Copy link
Contributor

I might be doing something wrong but I'm not able to reproduce this error on my machine.

It's flaky, I restarted it.

@swiatekm swiatekm requested review from swiatekm and frzifus May 24, 2025 11:45
@swiatekm swiatekm merged commit a419080 into open-telemetry:main May 24, 2025
46 of 47 checks passed
@LucaLanziani LucaLanziani deleted the issue/3988-go-yaml branch May 24, 2025 11:54
@LucaLanziani
Copy link
Contributor Author

New issue created github.com//issues/4028

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.

Replace instances of gopkg.in/yaml.v3
4 participants