opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[confignet] Change `Transport` from `string` to `TransportType`

Open TylerHelmuth opened this issue 1 year ago • 1 comments

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 https://github.com/open-telemetry/opentelemetry-collector/issues/9364

Documentation: <Describe the documentation added.>

Added godoc comments

TylerHelmuth avatar Jan 24 '24 17:01 TylerHelmuth

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.97%. Comparing base (cc485e0) to head (21608ea).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9385   +/-   ##
=======================================
  Coverage   90.96%   90.97%           
=======================================
  Files         353      353           
  Lines       18626    18640   +14     
=======================================
+ Hits        16943    16957   +14     
  Misses       1356     1356           
  Partials      327      327           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 24 '24 18:01 codecov[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Mar 06 '24 03:03 github-actions[bot]

@bogdandrutu This is waiting on your review

mx-psi avatar Mar 06 '24 11:03 mx-psi

All tests are now passing except Contrib, which I'll fixed after this is merged should we choose to go forward with this breaking change approach.

TylerHelmuth avatar Mar 14 '24 14:03 TylerHelmuth