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] Add strict type validation under a feature gate #10400

Merged
merged 12 commits into from
Jun 17, 2024

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jun 13, 2024

Description

  • Add confmap.strictlyTypedInput feature gate that introduces stricter type checks when resolving configuration
  • Make confmap.NewRetrievedFromYAML function public so that external providers have consistent behavior when resolving YAML
  • Adds confmap.Retrieved.AsString method to retrieve string representation for retrieved values

Link to tracking issue

Relates to #9854, updates #8565, #9532

@mx-psi mx-psi changed the title [WIP] Strictly typed input feature gate [confmap] Add strict type validation under a feature gate Jun 13, 2024
@mx-psi mx-psi force-pushed the mx-psi/weaklytypedinput branch 2 times, most recently from 6781a9f to 4cc21d9 Compare June 13, 2024 15:21
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.36%. Comparing base (42b61cc) to head (1308967).

Files Patch % Lines
confmap/expand.go 66.66% 2 Missing and 3 partials ⚠️
confmap/provider.go 92.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10400      +/-   ##
==========================================
- Coverage   92.40%   92.36%   -0.04%     
==========================================
  Files         387      386       -1     
  Lines       18323    18353      +30     
==========================================
+ Hits        16931    16952      +21     
- Misses       1046     1050       +4     
- Partials      346      351       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Instead of adding a new `WithStringRepresentation` public option, we make the `NewRetrievedFromYAML` method public.
This makes it so that people can't add arbitrary string representations for their retrieved values, but instead only if the value is provided as YAML.
@mx-psi mx-psi marked this pull request as ready for review June 13, 2024 16:43
@mx-psi mx-psi requested a review from a team as a code owner June 13, 2024 16:43
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Couple question about tests, but otherwise the e2e tests are making me feel comfortable. the changed logic in expand is confusing to me, I am going to see if I can make a quick PR to remove it.

confmap/expand.go Show resolved Hide resolved
confmap/provider.go Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

Created #10403 to try and simplify some stuff before this PR

@mx-psi
Copy link
Member Author

mx-psi commented Jun 14, 2024

Added a few more tests, @TylerHelmuth PTAL!

@mx-psi mx-psi requested a review from TylerHelmuth June 14, 2024 15:34
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Some nits, LGTM overall

// This list must be kept in sync with checkRawConfType.
input, err := ret.AsRaw()
if err != nil {
return "", err
Copy link
Member

Choose a reason for hiding this comment

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

Is this covered by e2e tests as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no way to hit this path today

return r.rawConf, nil


const StrictlyTypedInputID = "confmap.strictlyTypedInput"

var StrictlyTypedInputGate = featuregate.GlobalRegistry().MustRegister(StrictlyTypedInputID,
Copy link
Member

Choose a reason for hiding this comment

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

nit: curious why this needs to be in internal? There may be contrib packages that want to test against this feature gate

Copy link
Member Author

Choose a reason for hiding this comment

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

No strict need for this to be internal, but I would like to wait until someone asks for it to make it public

@mx-psi
Copy link
Member Author

mx-psi commented Jun 17, 2024

I am going to merge so this lands on the next release, we can discuss making it public once a component requests it

@mx-psi mx-psi merged commit 7a3c35c into open-telemetry:main Jun 17, 2024
48 of 49 checks passed
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.

None yet

3 participants