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 extension/storage to extension/experimental/storage #4082

Conversation

JamesJHPark
Copy link
Contributor

@JamesJHPark JamesJHPark commented Sep 20, 2021

Description:
This PR is to move the experimental package extension/storage to extension/experimental/storage and follow the extension packages naming conventions.

Link to tracking Issue:
#4059

@JamesJHPark JamesJHPark requested review from a team and codeboten September 20, 2021 21:22
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Change looks good here, please make sure to update the contrib repo as well.

CHANGELOG.md Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@codeboten @Aneurysm9 see comment in the issue, do we follow the same pattern for just interfaces?

@Aneurysm9
Copy link
Member

@codeboten @Aneurysm9 see comment in the issue, do we follow the same pattern for just interfaces?

Which pattern? Having extension at the end of the name or having experimental in the path? I think they're probably both fine, but I could be convinced that the suffix might not be necessary.

@alolita alolita added the ready-to-merge Code review completed; ready to merge by maintainers label Sep 24, 2021
@bogdandrutu
Copy link
Member

Was referring to the suffix since it is not an extension itself

@JamesJHPark
Copy link
Contributor Author

@bogdandrutu storageextension suffix has been removed to storage. Thank you.

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #4082 (90bc8f7) into main (ebb0fbd) will increase coverage by 0.34%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4082      +/-   ##
==========================================
+ Coverage   87.53%   87.87%   +0.34%     
==========================================
  Files         173      173              
  Lines       10211    10181      -30     
==========================================
+ Hits         8938     8947       +9     
+ Misses       1010      984      -26     
+ Partials      263      250      -13     
Impacted Files Coverage Δ
consumer/consumererror/combine.go 75.67% <0.00%> (-6.15%) ⬇️
service/parserprovider/merge.go 84.21% <0.00%> (-0.79%) ⬇️
config/configtest/configtest.go 96.66% <0.00%> (-0.25%) ⬇️
receiver/scraperhelper/scrapercontroller.go 93.02% <0.00%> (-0.09%) ⬇️
receiver/scrapererror/scrapeerror.go 100.00% <0.00%> (ø)
service/internal/fanoutconsumer/logs.go 100.00% <0.00%> (ø)
service/internal/fanoutconsumer/traces.go 100.00% <0.00%> (ø)
service/internal/fanoutconsumer/metrics.go 100.00% <0.00%> (ø)
config/internal/configsource/manager.go 87.42% <0.00%> (+0.81%) ⬆️
service/internal/builder/receivers_builder.go 74.74% <0.00%> (+1.24%) ⬆️
... and 5 more

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 ebb0fbd...90bc8f7. Read the comment docs.

@JamesJHPark JamesJHPark changed the title Move extension/storage to extension/experimental/storageextension Move extension/storage to extension/experimental/storage Sep 28, 2021
@alolita
Copy link
Member

alolita commented Sep 28, 2021

Hi @bogdandrutu can you please do a final review and merge? ty!

@bogdandrutu
Copy link
Member

@JamesJHPark please make sure that the contrib is fixed if needed.

@bogdandrutu bogdandrutu merged commit 6ecb0bd into open-telemetry:main Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants