Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[chore] Create RFC for Optional config types #12596
base: main
Are you sure you want to change the base?
[chore] Create RFC for Optional config types #12596
Changes from all commits
4afe23f
295b93b
806f3ba
94e8a78
640ec50
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Drawback numbers 2 and 5 appear contradictory. Is there a reason why pointers are "conventionally" pointers, aside from mutability?
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.
This may not really apply to config structs, but outside mutability, another reason to use a pointer type for a field in Go is so you don't have to pass around a large struct by value.
The specific point I was making with item 2 is that some types in popular libraries are pointers, e.g.
*zap.Logger
,*grpc.Server
, etc. If pointers are used to indicate optional values, if you have a field that is a*zap.Logger
type, it's difficult to tell if it's intentionally optional, or if that type is almost always used as a pointer type (zap.Logger
as a non-pointer is rare from what I've seen). Obviously*zap.Logger
isn't a perfect example because nobody is setting a logger in YAML, but hopefully my point still makes sense.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.
More than golang code, would be good to show how users in yaml will use it.
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.
there's no expected impact on YAML
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.
What is the intended implementation plan for Optional config types? In the above example, it says:
So is the intention to only modify fields who meet some required use case? Or is it to modify all fields that are currently pointers?
Changing the field type is a breaking API change. Is there a different approach depending on the stability level of a component? Or is the expectation that anyone importing and using a config struct from an otel component will have to make any required changes given the new struct field types?
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.
@kristinapathak Do you have examples of components/modules that are 1.x and need to use this optional type? We are blocking
confighttp
/configgrpc
on this so that we can apply this pattern on these modules.The intent is to apply this new type everywhere, go through an (API) breaking change, and ensure this type is used everywhere going forward.
Check warning on line 273 in docs/rfcs/optional-config-type.md