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

[release] builder integration test succeeds on main but fails on release prep #11607

Open
Tracked by #11625
djaglowski opened this issue Nov 5, 2024 · 3 comments · Fixed by #11793 · May be fixed by #11940
Open
Tracked by #11625

[release] builder integration test succeeds on main but fails on release prep #11607

djaglowski opened this issue Nov 5, 2024 · 3 comments · Fixed by #11793 · May be fixed by #11940
Assignees
Labels
bug Something isn't working release:retro Issues discussed in a release retrospective

Comments

@djaglowski
Copy link
Member

Describe the bug

When a new module was added since the last release, the Builder - Integration tests github action succeeds on main but fails on the release prep PR.

What did you expect to see?

CI should be able to catch this issue immediately and appropriate replace statements should be required in the codebase.

What did you see instead?

The release process fails.


Is it possible to detect and require a solution to this problem at the time the module was added? What new tests or changes to existing tests are necessary to ensure releases are not blocked by this in the future?

@djaglowski djaglowski added bug Something isn't working release:blocker The issue must be resolved before cutting the next release labels Nov 5, 2024
@mx-psi mx-psi added the release:retro Issues discussed in a release retrospective label Nov 5, 2024
@djaglowski djaglowski removed the release:blocker The issue must be resolved before cutting the next release label Nov 5, 2024
@djaglowski
Copy link
Member Author

The immediate problem was fixed by adding #11604, but we need a better solution to ensure these issues are caught prior to the release process.

@jade-guiton-dd jade-guiton-dd moved this from Todo to In Progress in Collector: v1 Dec 2, 2024
@jade-guiton-dd jade-guiton-dd moved this from In Progress to Waiting for reviews in Collector: v1 Dec 4, 2024
mx-psi pushed a commit that referenced this issue Dec 5, 2024
#### Description

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).

#### Note

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.

#### Link to tracking issue
Fixes #11607

#### Testing
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.
@github-project-automation github-project-automation bot moved this from Waiting for reviews to Done in Collector: v1 Dec 5, 2024
@songy23
Copy link
Member

songy23 commented Dec 16, 2024

reopened with #11909

@songy23 songy23 reopened this Dec 16, 2024
@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Dec 17, 2024

I replicated the release PR on my fork here, but I can't reproduce the test failure locally... Can someone else give it a try, perhaps on a Linux machine in case it's a platform-dependent bug?

Edit: I'm silly, of course it's going to work now that the version has been published...

jade-guiton-dd added a commit to jade-guiton-dd/opentelemetry-collector that referenced this issue Dec 17, 2024
…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 added a commit to jade-guiton-dd/opentelemetry-collector that referenced this issue Dec 17, 2024
…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.
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this issue Dec 19, 2024
…elemetry#11793)

#### Description

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).

#### Note

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.

#### Link to tracking issue
Fixes open-telemetry#11607

#### Testing
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release:retro Issues discussed in a release retrospective
Projects
Archived in project
4 participants