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 local module replacement to ocb template #11653

Conversation

jackgopack4
Copy link
Contributor

Description

Adds ability to replace Providers and Converters with local paths when building custom collector in ocb

Link to tracking issue

Closes #11649

Testing

local build of ocb with locally downloaded converter and provider (can add this type of test to github actions if others think it is necessary)

Documentation

.chloggen file

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.61%. Comparing base (7b31c96) to head (4968e46).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11653      +/-   ##
==========================================
+ Coverage   91.58%   91.61%   +0.02%     
==========================================
  Files         440      440              
  Lines       23768    23746      -22     
==========================================
- Hits        21769    21755      -14     
+ Misses       1627     1620       -7     
+ Partials      372      371       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jackgopack4 jackgopack4 force-pushed the add-local-replace-providers-converters branch from 4066e12 to fcf78ba Compare November 12, 2024 19:18
@jackgopack4 jackgopack4 marked this pull request as ready for review November 12, 2024 19:56
@jackgopack4 jackgopack4 requested a review from a team as a code owner November 12, 2024 19:56
@jackgopack4
Copy link
Contributor Author

for some reason ad8ab82 is still pending for removal even though I force pushed.

@jackgopack4
Copy link
Contributor Author

if the commit doesn't disappear today I'll close this PR and open a new one with a fresh branch. opened a ticket with GitHub because this is weird behavior

@jackgopack4
Copy link
Contributor Author

ok I just re-added and then reverted the commit, seems to be fine now

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 37 to 42
{{- range .Providers}}
{{if ne .Path ""}}replace {{.GoMod}} => {{.Path}}{{end}}
{{- end}}
{{- range .Converters}}
{{if ne .Path ""}}replace {{.GoMod}} => {{.Path}}{{end}}
{{- end}}
Copy link
Member

Choose a reason for hiding this comment

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

Can you do a separate PR and rename .Providers -> .ConfmapProviders same for converter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that makes sense to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you want to wait to merge this one in until that PR is open? seems like it would make sense to merge this and then I can create a new branch from main with this included

cmd/builder/internal/builder/templates/go.mod.tmpl Outdated Show resolved Hide resolved
@songy23 songy23 requested a review from bogdandrutu November 14, 2024 15:30
@bogdandrutu bogdandrutu merged commit de35049 into open-telemetry:main Nov 14, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 14, 2024
bogdandrutu pushed a commit that referenced this pull request Nov 14, 2024
…ers (#11678)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
change name in templates and cmd/builder to refer to Providers and
Converters as ConfmapProviders and ConfmapConverters
<!-- Issue number if applicable -->
#### Link to tracking issue
as requested here:
#11653 (comment)

<!--Describe what testing was performed and which tests were added.-->
#### Testing
existing tests should cover this
<!--Describe the documentation added.-->
#### Documentation
none, internal change and no new code/api
<!--Please delete paragraphs that you did not use before submitting.-->
djaglowski pushed a commit to djaglowski/opentelemetry-collector that referenced this pull request Nov 21, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Adds ability to replace Providers and Converters with local paths when
building custom collector in ocb
<!-- Issue number if applicable -->
#### Link to tracking issue
Closes open-telemetry#11649

<!--Describe what testing was performed and which tests were added.-->
#### Testing
local build of ocb with locally downloaded converter and provider (can
add this type of test to github actions if others think it is necessary)
<!--Describe the documentation added.-->
#### Documentation
.chloggen file
<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
djaglowski pushed a commit to djaglowski/opentelemetry-collector that referenced this pull request Nov 21, 2024
…ers (open-telemetry#11678)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
change name in templates and cmd/builder to refer to Providers and
Converters as ConfmapProviders and ConfmapConverters
<!-- Issue number if applicable -->
#### Link to tracking issue
as requested here:
open-telemetry#11653 (comment)

<!--Describe what testing was performed and which tests were added.-->
#### Testing
existing tests should cover this
<!--Describe the documentation added.-->
#### Documentation
none, internal change and no new code/api
<!--Please delete paragraphs that you did not use before submitting.-->
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this pull request Dec 19, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Adds ability to replace Providers and Converters with local paths when
building custom collector in ocb
<!-- Issue number if applicable -->
#### Link to tracking issue
Closes open-telemetry#11649

<!--Describe what testing was performed and which tests were added.-->
#### Testing
local build of ocb with locally downloaded converter and provider (can
add this type of test to github actions if others think it is necessary)
<!--Describe the documentation added.-->
#### Documentation
.chloggen file
<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this pull request Dec 19, 2024
…ers (open-telemetry#11678)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
change name in templates and cmd/builder to refer to Providers and
Converters as ConfmapProviders and ConfmapConverters
<!-- Issue number if applicable -->
#### Link to tracking issue
as requested here:
open-telemetry#11653 (comment)

<!--Describe what testing was performed and which tests were added.-->
#### Testing
existing tests should cover this
<!--Describe the documentation added.-->
#### Documentation
none, internal change and no new code/api
<!--Please delete paragraphs that you did not use before submitting.-->
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.

ocb should support replacing providers and converters with local codepaths for testing
3 participants