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.

Sign up for GitHub

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

Jump to bottom

Implement Validate() in ReceiverSettings, easy transition #2865

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Implement Validate() in ReceiverSettings, easy transition #2865

merged 1 commit into from
Mar 31, 2021

Conversation

bogdandrutu
Copy link
Member

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

@bogdandrutu bogdandrutu requested a review from a team March 31, 2021 01:28
bogdandrutu
bogdandrutu commented Mar 31, 2021
View reviewed changes
service/internal/builder/receivers_builder_test.go
@@ -334,7 +334,8 @@ func TestBuildReceivers_NotSupportedDataType(t *testing.T) {
t.Run(test.configFile, func(t *testing.T) {

cfg, err := configtest.LoadConfigFile(t, path.Join("testdata", test.configFile), factories)
assert.Error(t, err)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was changed in #2802 because NopConfig.Validate was failing because "bf" (badfactory) was not the same type as "nop"

Copy link
Member

Choose a reason for hiding this comment

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

It was my bad. Thx for correcting it.

@bogdandrutu
Copy link
Member Author

/cc @mx-psi @mxiamxia

@codecov
Copy link

codecov bot commented Mar 31, 2021 •
edited
Loading

Codecov Report

Merging #2865 (4392201) into main (231815c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2865      +/-   ##
==========================================
- Coverage   91.80%   91.80%   -0.01%     
==========================================
  Files         286      286              
  Lines       15146    15141       -5     
==========================================
- Hits        13905    13900       -5     
  Misses        849      849              
  Partials      392      392
Impacted Files Coverage Δ
internal/testcomponents/example_receiver.go 100.00% <ø> (ø)
component/componenttest/nop_receiver.go 100.00% <100.00%> (ø)
config/receiver.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 231815c...4392201. Read the comment docs.

@mxiamxia
Copy link
Member

Hi @bogdandrutu, could you help to elaborate what's the next step making Validate() as mandatory for components?

I can help to make the same changes to the other components after this PR got merged. :)

@bogdandrutu
Copy link
Member Author

@mxiamxia thanks for commenting, sure we should apply the same change to all the configs (including changes from this PR):

  • Add validatable to all the configs and implement Validate in their settings for the moment. Maybe add empty implementations so we can remove the default implementation from the Settings in the next release.
  • After that we should start resolving implementing these Validate funcs. For at least the core component we want to release stable.
  • We also need to update the changelog

mxiamxia
mxiamxia approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

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

LGTM! I'll take over and work on the rest of configs.

mx-psi
mx-psi approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@mx-psi mx-psi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, maybe I should do this too for unmarshaling on #2867 (nope, not possible)

@bogdandrutu bogdandrutu reopened this Mar 31, 2021
@bogdandrutu
Implement Validate() in ReceiverSettings, easy transition
4392201 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu merged commit fe86ab5 into open-telemetry:main Mar 31, 2021
@bogdandrutu bogdandrutu deleted the defaultimpl branch March 31, 2021 15:57
pjanotti pushed a commit to pjanotti/opentelemetry-service that referenced this pull request Apr 6, 2021
@bogdandrutu @pjanotti
Implement Validate() in ReceiverSettings, easy transition (open-telem…
5a2429b 
…etry#2865)

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
@jeffreyc-splunk
Update installer script for amazonlinux 2023 and rhel 9 (open-telemet…
cf4c99b 
…ry#2865)

* Update installer script for amazonlinux 2023 and rhel 9

* Update linux-installer.md

* Update test images

* Create test matrix for distros
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxiamxia mxiamxia mxiamxia approved these changes

@mx-psi mx-psi mx-psi approved these changes

@dmitryax dmitryax dmitryax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bogdandrutu @mxiamxia @mx-psi @dmitryax