[chore] Unrevert automatic replace generation in builder-integration-test
#11940
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…elemetry#11793) This PR removes the manually maintained `replace` statements in `cmd/builder/test/core.builder.yaml` and generates them automatically in `cmd/builder/test/test.sh`. This ensures that the builder integration test properly uses the local version of core collector modules, even if a `replace` statement is forgotten. This is especially important in release PRs where dependencies have not-yet-valid version numbers (see tracking issue for context). I believe a better implementation of this long-term may be to commit a `go.work` file to the repository as the single source of truth for `replace`s, and let the `go` commands run by `ocb` take it into account automatically. (This would also avoid needing to maintain the `replace`s in every `go.mod`, which can be annoying when `make crosslink` is not sufficient.) But as there were [objections the last time this was discussed](open-telemetry/opentelemetry-collector-contrib#21972 (comment)), I decided to leave this as a future discussion. Fixes open-telemetry#11607 We can simulate a Release PR by setting the `exporter` import in `debugexporter` to an invalid version like `0.999.0`. With the new script, this causes no problems, showing that the build uses the local version without fetching from the proxy. Filtering the `exporter` module from the replace statements by adding this to the script: ```bash core_mods=$(echo "$core_mods" | grep -v "/exporter$") ``` makes `ocb` output the expected `unknown revision` error.
jade-guiton-dd
force-pushed
the
fix-builder-test
branch
from
466d7f2 to
e80493a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11940 +/- ##
=======================================
Coverage 91.62% 91.62%
=======================================
Files 447 447
Lines 23731 23731
=======================================
Hits 21743 21743
Misses 1613 1613
Partials 375 375 ☔ View full report in Codecov by Sentry. |
|
In case it helps: the failed CI during the release is https://github.com/open-telemetry/opentelemetry-collector/actions/runs/12358329683/job/34488569900?pr=11904 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
PR #11793, which added code to automatically generate
replaceclauses in builder-integration-test in order to avoid issues in release PRs, caused issues in a release PR, and was reverted in #11909.The issue ended up being that the
replaceclause for the root collector module (go.opentelemetry.io/collector) was missing, because (cf. previous source code) I was generating a list of module paths relative to the root, with an empty string for the root module, which bash helpfully ignored during the for loop below.This PR unreverts #11793, with a fix for the above issue. (We make sure to only strip the initial dot of the folder path inside the for loop.)
Link to tracking issue
Fixes #11607, hopefully correctly this time.
Testing
I replicated the release PR in which the issues arose with a .1 patch version number, replicated the failure that was observed, and checked that the new script has the expected behavior.