Move Unmarshaler interface to Config instead of Factory #2867
Conversation
If a component wants custom unmarshaling they need to add an Unmarshal method to their Config struct instead of providing it through the factory.
Codecov Report
@@ Coverage Diff @@
## main #2867 +/- ##
==========================================
- Coverage 91.76% 91.72% -0.05%
==========================================
Files 286 286
Lines 15100 15056 -44
==========================================
- Hits 13857 13810 -47
- Misses 850 853 +3
Partials 393 393
Continue to review full report at Codecov.
|
Move customUnmarshaler implementation to Unmarshal
|
@mx-psi not sure if you forgot to push changes or you are still working on them :) |
@bogdandrutu had it half-finished yesterday but I didn't want to ping you. You can take a look now! |
| // load the non-dynamic config normally | ||
| err := componentParser.Unmarshal(cfg) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This (and other error modes on the customUnmarshalers) was untested before, I can add tests for this but I think it can be done on a different PR.
config/config.go
Outdated
| @@ -168,6 +168,14 @@ type validatable interface { | |||
| Validate() error | |||
| } | |||
|
|
|||
| // Unmarshable defines the optional interface for the configuration unmarshaling. | |||
There was a problem hiding this comment.
We should mention what is the default if not implemented. Configs will still be unmarshalled but using the default way. Maybe name the interface CustomUnmarshable (not sure asking for opinion)?
There was a problem hiding this comment.
Maybe name the interface
CustomUnmarshable(not sure asking for opinion)?
I agree, that name is less confusing. I will also update the docs to note that there is a default unmarshaler.
| if componentParser == nil || len(componentParser.AllKeys()) == 0 { | ||
| return fmt.Errorf("empty config for Jaeger receiver") | ||
| } |
There was a problem hiding this comment.
For future PR, maybe this validation belongs to Validate not to unmarshal?
There was a problem hiding this comment.
👍 I will have a look at the custom unmarshalers and move whatever makes sense to the validation method in a future PR
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This is mergeable (maybe @tigrannajaryan can merge since Bogdan seems to be on PTO?) |
…y#2867) Bumps [boto3](https://github.com/boto/boto3) from 1.26.96 to 1.26.97. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.26.96...1.26.97) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description:
component.ConfigUnmarshalerandWithCustomUnmarshaler. Add fallback behavior.config.Unmarshableinterface.component.ConfigUnmarshalerto usingconfig.Unmarshableinterface on existing components.Link to tracking Issue: Fixes #2597
Testing: Amended unit tests
To be tracked on #2819.