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

[otelcoltest] Move otelcoltest to its own module #10417

Merged
merged 11 commits into from
Jun 21, 2024

Conversation

TylerHelmuth
Copy link
Member

Description

As part of #10290, move otelcoltest to its own module.

Link to tracking issue

Related to #10290

@TylerHelmuth TylerHelmuth force-pushed the move-otelcoltest branch 2 times, most recently from a935651 to ceb3636 Compare June 17, 2024 18:22
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 18.86792% with 43 lines in your changes missing coverage. Please review.

Project coverage is 92.27%. Comparing base (b3c781b) to head (79865d7).

Current head 79865d7 differs from pull request most recent head 950bcd5

Please upload reports for the commit 950bcd5 to get more accurate results.

Files Patch % Lines
otelcoltest/config.go 15.38% 31 Missing and 2 partials ⚠️
otelcoltest/nop_factories.go 28.57% 5 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10417      +/-   ##
==========================================
- Coverage   92.35%   92.27%   -0.09%     
==========================================
  Files         388      388              
  Lines       18445    18461      +16     
==========================================
  Hits        17035    17035              
- Misses       1055     1065      +10     
- Partials      355      361       +6     

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

@TylerHelmuth TylerHelmuth force-pushed the move-otelcoltest branch 2 times, most recently from a590ebf to d2344aa Compare June 18, 2024 19:04
@TylerHelmuth TylerHelmuth marked this pull request as ready for review June 18, 2024 19:04
@TylerHelmuth TylerHelmuth requested review from a team and mx-psi June 18, 2024 19:04
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

I think there was a reason for this, but I don't remember why we need to change the path and we can't just leave it in its current folder. Could you explain the motivation for that?

.chloggen/move-otelcoltest-2.yaml Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Jun 19, 2024

@mx-psi Turns out that LoadConfig was used a lot in Contrib and it would be nice to keep using the version that depends on the providers/converters. We are trying to remove them from otelcol dependencies, but in this case the function is useful for our Contrib tests.

We could create an internal package in Contrib and duplicate this logic, but this way felt better.

If we want to do the Contrib solution, I think we should remove LoadConfig and LoadConfigAndValidate entirely.

@mx-psi
Copy link
Member

mx-psi commented Jun 19, 2024

@mx-psi Turns out that LoadConfig was used a lot in Contrib and it would be nice to keep using the version that depends on the providers/converters. We are trying to remove them from otelcol dependencies, but in this case the function is useful for our Contrib tests.

We could create an internal package in Contrib and duplicate this logic, but this way felt better.

If we want to do the Contrib solution, I think we should remove LoadConfig and LoadConfigAndValidate entirely.

@TylerHelmuth What I was asking is: why don't we add a go.mod and go.sum files to the current otelcol/otelcoltest folder instead of moving things to a top-level otelcoltest? Is there anything that prevents that?

@TylerHelmuth
Copy link
Member Author

@mx-psi oh I see what you mean. I dont see an issue with doing it that way, I'll update the PR

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Jun 20, 2024

@mx-psi crosslink doesn't like having the nested structure. The replace statements need to use ../../ but it generates ../

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@mx-psi
Copy link
Member

mx-psi commented Jun 20, 2024

@TylerHelmuth We do have that structure on pdata and pdata/pprofile/pdata/testdata though 🤔

@TylerHelmuth
Copy link
Member Author

@mx-psi you're right, I needed to get the module name nested correctly.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Almost there!

.chloggen/move-otelcoltest-2.yaml Outdated Show resolved Hide resolved
.chloggen/move-otelcoltest.yaml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
otelcol/otelcoltest/config.go Show resolved Hide resolved
otelcol/otelcoltest/go.mod Outdated Show resolved Hide resolved
otelcol/otelcoltest/go.mod Outdated Show resolved Hide resolved
@codeboten codeboten merged commit 6c2108d into open-telemetry:main Jun 21, 2024
48 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 21, 2024
mx-psi pushed a commit that referenced this pull request Jun 28, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Removed deprecated `NewCommand`. Deprecates `NewCommandMustSetProvider`.
Adds `NewCommand` that requires at least one provider be set.

Removes usage of imported providers/converters from otelcol (but they
still live in otelcoltest until
#10417 is
merged.

<!-- Issue number if applicable -->
#### Link to tracking issue
closes
#10290.

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests
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.

3 participants