Skip to content

Move configcheck.ValidateConfigFromFactories to service package #4055

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

Merged
merged 17 commits into from
Sep 21, 2021

Conversation

hyunuk
Copy link
Contributor

@hyunuk hyunuk commented Sep 16, 2021

Description:
This PR moves configcheck.ValidateConfigFromFactories into service package and makes the function unexported one.
Note: This PR is blocked by this PR #3956 so this PR should be merged after the PR #3956 is merged.

Link to tracking Issue:
#3876

@hyunuk hyunuk requested review from a team and Aneurysm9 September 16, 2021 04:07
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Can you please rebase to HEAD so I see only the diff with the current HEAD

@hyunuk
Copy link
Contributor Author

hyunuk commented Sep 16, 2021

@bogdandrutu updated!

@hyunuk hyunuk requested a review from bogdandrutu September 20, 2021 06:16
@bogdandrutu
Copy link
Member

@jpkrohling @Aneurysm9 I think this check is useful, the reason to move it here is to limit the public API, but probably this should be a "test" in the builder instead of a test at runtime? What do you think?

I am happy to merge it as is for the moment, but I would propose that the builder also has some "checks/validations" like this one as a pre-build step. What do you think?

@bogdandrutu bogdandrutu merged commit 1832173 into open-telemetry:main Sep 21, 2021
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.

4 participants