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

[confmap] Read YAML struct tags #10200

Closed

Conversation

evan-bradley
Copy link
Contributor

Description

Third party configuration structs, such as those used in the Prometheus receiver, use YAML struct tags that are necessary for confmap to understand if we want to be able to marshal YAML. In particular, in (*Conf).ToStringMap, we convert structs to a Map object, where ignoring flags like omitempty or squash causes the map to have a different structure than it would if we called yaml.Marshal on the same struct.

This PR allows ToStringMap to read omitempty and inline when struct fields have been annotated with the yaml tag. These are 2/3 of the flags the yaml tag supports: https://pkg.go.dev/gopkg.in/yaml.v3#Marshal.

Caveats:

  • Things like yaml:",squash" and mapstructure:",inline" will be possible now. I figure that allowing any valid flag regardless of the tag that it applies to is okay (we also support remain as an alias) and won't incur any issues, but if we are concerned about conflicts or stability guarantees, I can enforce a stricter schema for which tags support which flags.
  • I'm leaving out support for the json tag for now since we don't support JSON the way we support YAML, but I can add support for it here if we'd like.

Required for #10139.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Few things I want to understand before reviewing/approving this:

  1. Is the plan to support both long term?
  2. What about yaml unmarshallers like the https://pkg.go.dev/gopkg.in/yaml.v3#Unmarshaler? Another thing is for example encoding.TextUnmarshal which is currently supported by us and yaml, if behavior is different which one we use?
  3. Can we just move to yaml? We have validate Config test in the mdatagen tool which can help with these checks.

@evan-bradley
Copy link
Contributor Author

@bogdandrutu I'm closing this in favor of #10282, but I think your questions are still valid. To give a few answers:

Is the plan to support both long term?

The approach in #10282 is to defer to yaml.Marshal if no mapstructure tags are found. I think this is okay to support in the long term since we are deferring to a third party

What about yaml unmarshallers like the pkg.go.dev/gopkg.in/yaml.v3#Unmarshaler?

We use yaml.Unmarshaler in #10282 now.

Another thing is for example encoding.TextUnmarshal which is currently supported by us and yaml, if behavior is different which one we use?

#10282 uses a list of hooks to implement this. The hook order will dictate whether TextMarshaler is used or if yaml.Marshal is called on the struct.

Can we just move to yaml? We have validate Config test in the mdatagen tool which can help with these checks.

I think this could be feasible, but I will need to look into it. I think we can fall back to yaml tags as an intermediary step before deciding.

@evan-bradley evan-bradley deleted the confmap-yaml-tag branch June 5, 2024 19:56
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.

2 participants