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

Move all utils configmapproviders to internal to prepare interface change #4600

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

bogdandrutu
Copy link
Member

Signed-off-by: Bogdan Drutu [email protected]

@bogdandrutu bogdandrutu requested review from a team and owais December 21, 2021 18:57
@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #4600 (4d99af5) into main (d623354) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4600      +/-   ##
==========================================
- Coverage   90.77%   90.75%   -0.03%     
==========================================
  Files         178      178              
  Lines       10561    10563       +2     
==========================================
- Hits         9587     9586       -1     
- Misses        757      759       +2     
- Partials      217      218       +1     
Impacted Files Coverage Δ
service/config_provider.go 88.23% <ø> (ø)
config/configtest/configtest.go 91.00% <100.00%> (ø)
internal/configprovider/default.go 100.00% <100.00%> (ø)
internal/configprovider/expand.go 78.18% <100.00%> (ø)
internal/configprovider/merge.go 91.17% <100.00%> (ø)
service/internal/telemetrylogs/logger.go 86.66% <0.00%> (-5.84%) ⬇️

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 d623354...4d99af5. Read the comment docs.

@bogdandrutu bogdandrutu merged commit eb6f051 into open-telemetry:main Dec 21, 2021
@bogdandrutu bogdandrutu deleted the mvutilprovider branch December 21, 2021 20:54
codeboten pushed a commit to codeboten/opentelemetry-collector that referenced this pull request Jan 4, 2022
@alolita
Copy link
Member

alolita commented Jan 5, 2022

@bogdandrutu This is a pretty significant breaking change for downstream distributions such as AWS Distro for OpenTelemetry. We should've had community discussions and reviews from other approvers before making these changes. Same with #4636

@Aneurysm9
Copy link
Member

Exported identifiers that are made internal by this change (in addition to the not-at-all-unused InMemory provider removed in #4507) are in use by the ADOT collector.

@bogdandrutu
Copy link
Member Author

@Aneurysm9 if you have reviewed the #4507 which was open for 2 weeks probably you could have seen that, but lucky for you I did implement what ADOT needs in #4574.

So I am not sure where is the breaking change.

@jcchavezs
Copy link
Contributor

jcchavezs commented Jan 13, 2022

Moving NewExpand into an internal package is indeed a significant breaking change and I am not sure if there is a way back to this. Could you please at least link why this happened?

In our case, we provide a config ourselves and we used NewExpand to be able to expand env variables. Now that isn't possible.

@Aneurysm9
Copy link
Member

@jcchavezs #4672 seeks to address this. Please weigh in there.

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.

5 participants