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

logging#Config: Fix Options can no longer be parsed by go-yaml #84

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

lippserd
Copy link
Member

Previously, commit ccf002a introduced UnmarshalText() to allow Options to be parsed by the env package. With this change, the go-yaml package detects that Options should be decoded by that unmarshaller 1 2. However, since Options is not a text type, go-yaml does not use the unmarshaller 3, resulting in unmarshalling failures 4. This change implements UnmarshalYAML() on Options, which is tried before UnmarshalText() 5, restoring unmarshalling functionality.

Additionally, it's essential that Options is no longer inlined within Config, ensuring that UnmarshalYAML() receives only the relevant options section instead of the entire YAML. Interestingly, this used to work previously.

Footnotes

  1. https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L703

  2. https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L833

  3. https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L788

  4. https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L818

  5. https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L763

We use `require.Equal()` for comparison anyway so the type can be relaxed. This
is particularly useful for testing types like `logging#Config`, which contains
a map and thus would not satisfy comparable.
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Oct 25, 2024
Previously, logging output was configured in `Validate()` if not set, which is
inappropriate. It is now handled by implementing the `SetDefaults()` interface
from the `defaults` package.
Previously, commit ccf002a introduced `UnmarshalText()` to allow `Options` to
be parsed by the `env` package. With this change, the `go-yaml` package detects
that `Options` should be decoded by that unmarshaller [1, 2]. However, since
`Options` is not a text type, `go-yaml` does not use the unmarshaller [3],
resulting in unmarshalling failures [4]. This change implements
`UnmarshalYAML()` on `Options`, which is tried before `UnmarshalText()` [5],
restoring unmarshalling functionality.

Additionally, it's essential that `Options` is no longer inlined within
`Config`, ensuring that `UnmarshalYAML()` receives only the relevant `options`
section instead of the entire YAML.  Interestingly, this used to work
previously.

[1] https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L703
[2] https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L833
[3] https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L788
[4] https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L818
[5] https://github.com/goccy/go-yaml/blob/v1.12.0/decode.go#L763
@lippserd lippserd merged commit 074a348 into main Oct 29, 2024
15 checks passed
@lippserd lippserd deleted the go-yaml-compat branch October 29, 2024 08:41
@oxzi oxzi added this to the 0.4.0 milestone Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants