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

Add the config source manager #303

Merged
merged 2 commits into from Apr 19, 2021

Conversation

pjanotti
Copy link
Contributor

A follow-up to #295 that superseded #277.

This brings the config source manager from the core repository that is not publicly exposed. The manager will be used to implement a provider that uses config sources to inject data in configuration. This code was originally reviewed on the collector core repository.

/cc @owais

@codecov
Copy link

codecov bot commented Apr 17, 2021

Codecov Report

Merging #303 (4c3e37d) into main (689eebb) will increase coverage by 0.74%.
The diff coverage is 89.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
+ Coverage   86.82%   87.57%   +0.74%     
==========================================
  Files          24       25       +1     
  Lines        1579     1915     +336     
==========================================
+ Hits         1371     1677     +306     
- Misses        173      194      +21     
- Partials       35       44       +9     
Impacted Files Coverage Δ
internal/configprovider/manager.go 89.88% <89.88%> (ø)
internal/configprovider/helpers.go 36.36% <0.00%> (+36.36%) ⬆️

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 689eebb...4c3e37d. Read the comment docs.

@tigrannajaryan
Copy link
Contributor

I do not quite understand why are we not implementing the configsource support in Collector core. Why do we need the config manager here? I thought it can be internal in Collector core and still do its job. I am confused.

@pjanotti
Copy link
Contributor Author

I do not quite understand why are we not implementing the configsource support in Collector core. Why do we need the config manager here? I thought it can be internal in Collector core and still do its job. I am confused.

It is a good question: internal is not visible on other repositories - so until that is moved outside internal we can't use it. The obvious risk is the code diverging, but, that is the point here per my understanding: we want to experiment before making it public on the core repository.

Copy link
Contributor

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM. Discussed with @pjanotti that this is a temporary approach to speed up the delivery. We will eventually rely on the Manager in the core.

@pjanotti pjanotti merged commit 783ee31 into signalfx:main Apr 19, 2021
@pjanotti pjanotti deleted the add-configsourcemanager branch April 19, 2021 18:34
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

2 participants