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

[configtls] mark module as stable

Open atoulme opened this issue 1 year ago • 9 comments
trafficstars

Description

Mark module as stable.

Link to tracking issue

Fixes #9377

atoulme avatar Jun 06 '24 03:06 atoulme

Codecov Report

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

Project coverage is 92.36%. Comparing base (72c23aa) to head (addd9c4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10344   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         386      386           
  Lines       18402    18402           
=======================================
  Hits        16997    16997           
  Misses       1052     1052           
  Partials      353      353           

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

codecov[bot] avatar Jun 06 '24 04:06 codecov[bot]

Reviewing this PR inspired https://github.com/open-telemetry/opentelemetry-collector/pull/10358, that removes the dependency on confmap from this configtls

codeboten avatar Jun 06 '24 19:06 codeboten

Thank you @codeboten , indeed the dependency on confmap was only through tests.

atoulme avatar Jun 06 '24 23:06 atoulme

@mx-psi @songy23 @atoulme One thing to keep in mind is that this package does expose mapstructure tags for the struct, which would mean that we'd have to keep support for them around for the life of this package. Is this ok? @TylerHelmuth reminded me that this was the reason we'd held on stabilizing other config packages until confmap is stable

codeboten avatar Jun 07 '24 15:06 codeboten

@codeboten Right, I forgot about this. I don't see us changing mapstructure tags for something else but it makes sense to discuss explicitly. Should we file an issue about this? (We don't have one, do we?)

mx-psi avatar Jun 07 '24 15:06 mx-psi

@mx-psi i think only issue that comes close is this one around how mapstructure is unmarshaled https://github.com/open-telemetry/opentelemetry-collector/issues/9532, i don't know if we want another issue that specific talks about which tags should be supported moving forward?

codeboten avatar Jun 07 '24 17:06 codeboten

Is changing tags on a struct field a breaking change? The go API is unchanged, right?

atoulme avatar Jun 07 '24 20:06 atoulme

@atoulme it's currently listed in versioning.md as breaking change https://github.com/open-telemetry/opentelemetry-collector/blob/main/VERSIONING.md#configuration-structures

codeboten avatar Jun 12 '24 15:06 codeboten

We had some discussion about bumping MinTLS to 1.3. I think this is something that is not a breaking change given that the Go team themselves have proposed changes of this sort in the past (see golang/go#45428) and this being considered a security best practice.

I am not sure how we should best clarify this on VERSIONING.md, but I think this can be dealt with independently from this PR and that we can merge this now.

mx-psi avatar Jun 24 '24 09:06 mx-psi