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.

Sign up for GitHub

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

Jump to bottom

[confignet] Should the two structs implement ConfigValidator? #9364

Closed
TylerHelmuth opened this issue Jan 24, 2024 · 1 comment · Fixed by #9385
Closed

[confignet] Should the two structs implement ConfigValidator? #9364

TylerHelmuth opened this issue Jan 24, 2024 · 1 comment · Fixed by #9385
Labels
area:config discussion-needed Community discussion needed enhancement New feature or request
Milestone
go.opentelemetry....

Comments

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jan 24, 2024 •
edited
Loading

Is your feature request related to a problem? Please describe.

Since these structs could be used in configuration, should we add Validate functions to implement ConfigValidator? For NetAddr, for example, we could validate that Transport is one of "tcp", "tcp4", "tcp6", "udp", "udp4", "udp6", "ip", "ip4", "ip6", "unix", "unixgram" and "unixpacket".
@TylerHelmuth TylerHelmuth added enhancement New feature or request area:config labels Jan 24, 2024
@TylerHelmuth TylerHelmuth added the discussion-needed Community discussion needed label Jan 24, 2024
@mx-psi
Copy link
Member

mx-psi commented Jan 24, 2024

Should we make Transport into a type with UnmarshalText?

@TylerHelmuth TylerHelmuth mentioned this issue Jan 24, 2024
Merged
mx-psi added a commit that referenced this issue Mar 15, 2024
@TylerHelmuth @mx-psi
[confignet] Change Transport from string to TransportType (#9385)
e70b5ab 
**Description:** 
Changes `Transport` from a `string` to a new `TransportType`. Implements
`UnmarshalText` for `TransportType` to enforce values.

This PR may be too much - it introduces a breaking change a lot of new
public APIs that may not be worth it for such a small module. If we
don't like the surface area this creates or the breaking change, but we
still want to enforce transport type values, I think implementing
`Validate` keeps the API footprint smaller and isn't breaking.

**Link to tracking Issue:** <Issue number if applicable>

Closes
#9364

**Documentation:** <Describe the documentation added.>

Added godoc comments

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@TylerHelmuth TylerHelmuth moved this to Done in Collector: v1 Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:config discussion-needed Community discussion needed enhancement New feature or request
Projects
Collector: v1
Archived in project
Milestone
go.opentelemetry.io/collector/config/...
Development

Successfully merging a pull request may close this issue.

[confignet] Change Transport from string to TransportType TylerHelmuth/opentelemetry-collector
2 participants
@mx-psi @TylerHelmuth