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

[xconfmap] Create module and add validation facilities #12226

Merged
merged 10 commits into from
Feb 4, 2025

Conversation

evan-bradley
Copy link
Contributor

@evan-bradley evan-bradley commented Jan 31, 2025

Description

Builds on #12224 and starts the move of config validation from component to confmap. We can keep this in xconfmap while we determine whether to add the ability to validate using struct field tags.

I think this has the following advantages:

  1. Everything configuration-related is now in confmap instead of split between confmap and component.
  2. We can share things like the mapstructure tag and config key separator as constants between unmarshaling and validation without creating dependencies between confmap and component.

The one uncertainty this creates is what to do with component.Config, which would now be used as a thin alias for any without any meaningful usage in component.

Link to tracking issue

Fixes #11524

@evan-bradley
Copy link
Contributor Author

evan-bradley commented Jan 31, 2025

The relevant changes are in this commit: aa1b14c (#12226)

#12224 is merged, so this PR only contains relevant changes now.

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 98.49624% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.31%. Comparing base (97cae29) to head (b781e06).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
confmap/xconfmap/config.go 98.43% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12226      +/-   ##
==========================================
+ Coverage   91.27%   91.31%   +0.03%     
==========================================
  Files         466      467       +1     
  Lines       25583    25711     +128     
==========================================
+ Hits        23351    23477     +126     
- Misses       1816     1818       +2     
  Partials      416      416              

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@evan-bradley evan-bradley force-pushed the create-confmap-validate branch from aa1b14c to 65dd4cd Compare January 31, 2025 18:17
@evan-bradley evan-bradley changed the title [confmap] Add validation facilities from component [xconfmap] Create module and add validation facilities from component Feb 4, 2025
@evan-bradley evan-bradley changed the title [xconfmap] Create module and add validation facilities from component [xconfmap] Create module and add validation facilities Feb 4, 2025
const (
// MapstructureTag is the struct field tag used to record marshaling/unmarshaling settings.
// See https://pkg.go.dev/github.com/go-viper/mapstructure/v2 for supported values.
MapstructureTag = "mapstructure"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just hardcode the tag name in xconfmap for now, but I figure it's a good time to make this an exported constant.

Copy link
Member

Choose a reason for hiding this comment

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

We could also move this to an confmap/internal package I guess, if we want to avoid exposing this

@evan-bradley evan-bradley marked this pull request as ready for review February 4, 2025 15:15
@evan-bradley evan-bradley requested review from mx-psi and a team as code owners February 4, 2025 15:15
@bogdandrutu bogdandrutu added this pull request to the merge queue Feb 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2025
@mx-psi mx-psi added this pull request to the merge queue Feb 4, 2025
Merged via the queue into open-telemetry:main with commit 0ac887e Feb 4, 2025
54 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 5, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
This PR is to unblock `make update-otel` in contrib repo

cc @evan-bradley @mx-psi 
<!-- Issue number if applicable -->
#### Link to tracking issue
Relevant #12226 

<!--Describe what testing was performed and which tests were added.-->
#### Testing
n/a

<!--Describe the documentation added.-->
#### Documentation
n/a

<!--Please delete paragraphs that you did not use before submitting.-->
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.

Should ConfigValidator be in confmap?
4 participants