-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 gopkg.in/yaml.v3
#37553
base: main
Are you sure you want to change the base?
Migrate to gopkg.in/yaml.v3
#37553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/13016551141/job/36309765881?pr=37553 seems you need to fix some indentations in tests
…tor-contrib into yaml.v3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand the implications of this change, given that moving from yaml.v2 to yaml.v3 does have some breaking changes https://ubuntu.com/blog/api-v3-of-the-yaml-package-for-go-is-available. Examples: not allowing integers for durations, enforcing unique keys. They are probably not common, but still may cause errors for configurations that previously did not error out.
If I understand correctly, this PR introduces potentially functional changes to the following components:
- S3 provider - affects how the provider unmarshals configurations from YAML files in S3 https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.118.0/confmap/provider/s3provider/provider.go#L108.
- ECS observer - affects how YAML is marshaled to
result_file
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.118.0/extension/observer/ecsobserver/target.go#L179. - Prometheus receiver and Target allocator - marshaling and unmarshaling of YAML
- https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.118.0/receiver/prometheusreceiver/config.go#L123
- https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.118.0/receiver/prometheusreceiver/targetallocator/config.go#L112
- https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.118.0/receiver/prometheusreceiver/targetallocator/manager.go#L208
- https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.118.0/receiver/prometheusreceiver/targetallocator/manager.go#L246
Ideally this should be carefully considered by the code owners
- S3 provider @Aneurysm9
- ECS observer @dmitryax
- Prometheus receiver @Aneurysm9 @dashpole
Please correct me if I'm missing something and the impact of this change is not as I'm seeing it.
Description
Migrate from
gopkg.in/yaml.v2
togopkg.in/yaml.v3
.Some modules were already pulling the two versions and moving to
v3
helped removing one of the two for some of them.