[chore] Make `check-contrib` rebuild and reapply tools #11670

jade-guiton-dd · 2024-11-13T16:59:58Z

Description

There have been issues where changes in mdatagen are not properly tested in CI and end up breaking collector-contrib (cf. #11167). This is because make check-contrib does not rebuild or rerun mdatagen before running contrib tests. This PR fixes that.

I added a flag to disable this when running manually, as it adds significant time (~3 min!) to the run and is only really useful when mdatagen has been modified.

I had to change the make gotidy to make for-all CMD="go mod tidy" because we don't have a way to run generate on a specific module group (and doing so may cause false-positives/negatives if a module group depends on the generated files of another).

Link to tracking issue

Fixes #11167

codecov · 2024-11-13T17:07:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.63%. Comparing base (8e522ad) to head (a5fb33c).
Report is 19 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11670      +/-   ##
==========================================
+ Coverage   91.54%   91.63%   +0.09%     
==========================================
  Files         442      442              
  Lines       23792    23776      -16     
==========================================
+ Hits        21780    21787       +7     
+ Misses       1641     1618      -23     
  Partials      371      371

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

jade-guiton-dd · 2024-11-14T15:06:14Z

So this works (I think), but it increases the duration of the "check contribs" job from about 10 min to about 13 min. This increase is mostly for two reasons:

We're doing more stuff (rebuilding tools and recreating generated files). Is there a way to only run that part in cases where we modify mdatagen?
For technical reasons (no way in the current contrib Makefile to run the go generate command only for a single group of modules), we're also go mod tidying all modules instead of just the current group. This could be fixed by adding a new command to the contrib Makefile. Do we want to bother with that? It may not be worth it if we manage to fix point 1.

(Note: The failure in build-and-test / checks is unrelated; it's because the CI just switched to the latest minor version of Go (1.22.9), and it's complaining that otelcol has a toolchain value of 1.22.8)

mx-psi · 2024-11-14T17:31:17Z

I also modified the go mod edit -replace and -dropreplace commands to use an automatically generated list of modules. This will prevent these lists from getting out of date. (For reference, we currently only replace 70 modules out of the 72 in versions.yaml, and only -dropreplace 63 out of those.)

@jade-guiton-dd This sounds great and we can probably land this change independently. Would you mind splitting this into a separate PR?

#11683) #### Description `make check-contrib` use `go mod edit` to allow running the `opentelemetry-collector-contrib` unit tests against the local version of the core libraries. It also removes the `replace` statements after tests have been run. At the moment, this is done with two long handwritten `go mod edit` commands. But this very easily gets out-of-date: for reference, we currently only `-replace` 70 modules out of the 73 in versions.yaml, and only `-dropreplace` 63 out of _those_. This PR changes the `Makefile` to programatically generate the commands, to avoid needing to keep them up to date. This was split off from PR #11670. You can check the output in the `contrib-tests` CI job output.

mx-psi · 2024-11-15T16:56:53Z

Merged #11683 :)

@mx-psi

#### Description This PR is in the context of [this PR on the core Collector modifying `make check-contrib` to use the latest version of `mdatagen`](open-telemetry/opentelemetry-collector#11670). The `make generate` commands currently starts by compiling build tools in the `.tools` folder, then it `go install`s mdatagen globally for use in `go:generate`. Unfortunately, `go install` does not take into account the version of `mdatagen` referenced in `internal/tools/go.mod`. This means it isn't possible to generate using the local version of `mdatagen` for testing, even with `replace` statements. This PR fixes this by prepending `.tools` to the `$PATH` before running the generate command, which makes `go:generate` use the version of `mdatagen` in that folder instead of a global one. (Originally, this PR also added a new `modgenerate` target in `Makefile.Common`, to allow running generate commands on a specific group of modules instead of all of them for efficiency reasons. After discussing it with @mx-psi, we decided to hold off on that change for a later PR, as it warrants further discussion.)

Makefile

open-telemetry#11683) #### Description `make check-contrib` use `go mod edit` to allow running the `opentelemetry-collector-contrib` unit tests against the local version of the core libraries. It also removes the `replace` statements after tests have been run. At the moment, this is done with two long handwritten `go mod edit` commands. But this very easily gets out-of-date: for reference, we currently only `-replace` 70 modules out of the 73 in versions.yaml, and only `-dropreplace` 63 out of _those_. This PR changes the `Makefile` to programatically generate the commands, to avoid needing to keep them up to date. This was split off from PR open-telemetry#11670. You can check the output in the `contrib-tests` CI job output.

…y#11670) #### Description There have been issues where changes in `mdatagen` are not properly tested in CI and end up breaking collector-contrib (cf. open-telemetry#11167). This is because `make check-contrib` does not rebuild or rerun `mdatagen` before running contrib tests. This PR fixes that. I added a flag to disable this when running manually, as it adds significant time (~3 min!) to the run and is only really useful when `mdatagen` has been modified. I had to change the `make gotidy` to `make for-all CMD="go mod tidy"` because we don't have a way to run `generate` on a specific module group (and doing so may cause false-positives/negatives if a module group depends on the generated files of another). #### Link to tracking issue Fixes open-telemetry#11167

This is a partial revert of open-telemetry#11670 as it is currently blocking the release due to the need to run go mod tidy multiple times. I'm still investigating why tidy is needed multiple times, but for now I'm disabling the generate step, which was not run before and is currently blocking the release process Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

This is a partial revert of #11670 as it is currently blocking the release due to the need to run go mod tidy multiple times. I'm still investigating why tidy is needed multiple times, but for now I'm disabling the generate step, which was not run before and is currently blocking the release process Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

@mx-psi

#### Description This PR is in the context of [this PR on the core Collector modifying `make check-contrib` to use the latest version of `mdatagen`](open-telemetry/opentelemetry-collector#11670). The `make generate` commands currently starts by compiling build tools in the `.tools` folder, then it `go install`s mdatagen globally for use in `go:generate`. Unfortunately, `go install` does not take into account the version of `mdatagen` referenced in `internal/tools/go.mod`. This means it isn't possible to generate using the local version of `mdatagen` for testing, even with `replace` statements. This PR fixes this by prepending `.tools` to the `$PATH` before running the generate command, which makes `go:generate` use the version of `mdatagen` in that folder instead of a global one. (Originally, this PR also added a new `modgenerate` target in `Makefile.Common`, to allow running generate commands on a specific group of modules instead of all of them for efficiency reasons. After discussing it with @mx-psi, we decided to hold off on that change for a later PR, as it warrants further discussion.)

open-telemetry#11683) #### Description `make check-contrib` use `go mod edit` to allow running the `opentelemetry-collector-contrib` unit tests against the local version of the core libraries. It also removes the `replace` statements after tests have been run. At the moment, this is done with two long handwritten `go mod edit` commands. But this very easily gets out-of-date: for reference, we currently only `-replace` 70 modules out of the 73 in versions.yaml, and only `-dropreplace` 63 out of _those_. This PR changes the `Makefile` to programatically generate the commands, to avoid needing to keep them up to date. This was split off from PR open-telemetry#11670. You can check the output in the `contrib-tests` CI job output.

…y#11670) #### Description There have been issues where changes in `mdatagen` are not properly tested in CI and end up breaking collector-contrib (cf. open-telemetry#11167). This is because `make check-contrib` does not rebuild or rerun `mdatagen` before running contrib tests. This PR fixes that. I added a flag to disable this when running manually, as it adds significant time (~3 min!) to the run and is only really useful when `mdatagen` has been modified. I had to change the `make gotidy` to `make for-all CMD="go mod tidy"` because we don't have a way to run `generate` on a specific module group (and doing so may cause false-positives/negatives if a module group depends on the generated files of another). #### Link to tracking issue Fixes open-telemetry#11167

This is a partial revert of open-telemetry#11670 as it is currently blocking the release due to the need to run go mod tidy multiple times. I'm still investigating why tidy is needed multiple times, but for now I'm disabling the generate step, which was not run before and is currently blocking the release process Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>

@mx-psi

#### Description This PR is in the context of [this PR on the core Collector modifying `make check-contrib` to use the latest version of `mdatagen`](open-telemetry/opentelemetry-collector#11670). The `make generate` commands currently starts by compiling build tools in the `.tools` folder, then it `go install`s mdatagen globally for use in `go:generate`. Unfortunately, `go install` does not take into account the version of `mdatagen` referenced in `internal/tools/go.mod`. This means it isn't possible to generate using the local version of `mdatagen` for testing, even with `replace` statements. This PR fixes this by prepending `.tools` to the `$PATH` before running the generate command, which makes `go:generate` use the version of `mdatagen` in that folder instead of a global one. (Originally, this PR also added a new `modgenerate` target in `Makefile.Common`, to allow running generate commands on a specific group of modules instead of all of them for efficiency reasons. After discussing it with @mx-psi, we decided to hold off on that change for a later PR, as it warrants further discussion.)

mx-psi self-requested a review November 14, 2024 00:23

jade-guiton-dd added 2 commits November 14, 2024 12:01

make check-contrib regenerates contrib, including mdatagen and builder

95dbee0

Tidy everything, not just current group (temporary)

300b9dc

jade-guiton-dd force-pushed the better-check-contrib branch from 773fdc2 to 300b9dc Compare November 14, 2024 11:01

jade-guiton-dd changed the title ~~Make check-contrib rebuild and reapply tools~~ [chore] Make check-contrib rebuild and reapply tools Nov 14, 2024

jade-guiton-dd added 4 commits November 14, 2024 12:17

Install mdatagen globally for go:generate

ce90143

Don't forget to build other tools too

75f80c9

gofmt is actually functionally required

c504945

Remove ocb re-run (shouldn't matter for tests)

04a3e50

jade-guiton-dd mentioned this pull request Nov 15, 2024

[chore] Generate replace directives automatically in check-contrib #11683

Merged

jade-guiton-dd added 2 commits November 15, 2024 14:02

Remove auto-generation of go mod edit commands

d5e4321

Merge branch 'main' into better-check-contrib

c0ee5f9

Merge branch 'main' into better-check-contrib

5ff3fc7

jade-guiton-dd mentioned this pull request Nov 18, 2024

[chore] make generate uses .tools/mdatagen open-telemetry/opentelemetry-collector-contrib#36416

Merged

Use improved contrib Makefile

69a3c89

Fix partial make fmt

992df68

jade-guiton-dd marked this pull request as ready for review November 20, 2024 11:28

jade-guiton-dd requested a review from a team as a code owner November 20, 2024 11:28

mx-psi reviewed Nov 20, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

jade-guiton-dd added 2 commits November 20, 2024 15:27

Rename regenerate-contrib to generate-contrib

20325f0

Remove generate skip check

a5fb33c

mx-psi approved these changes Nov 21, 2024

View reviewed changes

mx-psi merged commit c9f8e8b into open-telemetry:main Nov 21, 2024
49 checks passed

github-actions bot added this to the next release milestone Nov 21, 2024

jade-guiton-dd deleted the better-check-contrib branch November 27, 2024 13:36

codeboten mentioned this pull request Dec 3, 2024

[chore] disable generate for now #11794

Merged

jade-guiton-dd mentioned this pull request Dec 9, 2024

[chore] [WIP] Add script that runs go mod tidy in topological order open-telemetry/opentelemetry-collector-contrib#36723

Closed

This was referenced Jan 6, 2025

cicd: check-contrib fails during the release process #11795

Open

[chore] Use crosslink tidylist in make gotidy open-telemetry/opentelemetry-collector-contrib#37142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chore] Make `check-contrib` rebuild and reapply tools #11670

[chore] Make `check-contrib` rebuild and reapply tools #11670

jade-guiton-dd commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

jade-guiton-dd commented Nov 14, 2024 •

edited

Loading

mx-psi commented Nov 14, 2024

mx-psi commented Nov 15, 2024

[chore] Make check-contrib rebuild and reapply tools #11670

[chore] Make check-contrib rebuild and reapply tools #11670

Conversation

jade-guiton-dd commented Nov 13, 2024 • edited Loading

Description

Link to tracking issue

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

jade-guiton-dd commented Nov 14, 2024 • edited Loading

mx-psi commented Nov 14, 2024

mx-psi commented Nov 15, 2024

[chore] Make `check-contrib` rebuild and reapply tools #11670

[chore] Make `check-contrib` rebuild and reapply tools #11670

jade-guiton-dd commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

jade-guiton-dd commented Nov 14, 2024 •

edited

Loading