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.

Sign up for GitHub

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

Jump to bottom

[CI] Contrib tests should also test updated mdatagen #11167

Open
mx-psi opened this issue Sep 13, 2024 · 6 comments · Fixed by #11223 or #11670
Open

[CI] Contrib tests should also test updated mdatagen #11167

mx-psi opened this issue Sep 13, 2024 · 6 comments · Fixed by #11223 or #11670
Assignees
@jade-guiton-dd
Labels
ci-cd CI, CD, testing, build issues cmd/mdatagen priority:p1 High release:retro Issues discussed in a release retrospective

Comments

@mx-psi
Copy link
Member

mx-psi commented Sep 13, 2024

When we run make update-otel we update the mdatagen version as well. To avoid issues in contrib, we have the contrib tests but they are not testing this.

I believe in order to do this it would be as simple as adding a -replace option on check-contrib here https://github.com/open-telemetry/opentelemetry-collector/blob/5fc39ba6334f396c3b5497bf6d29db12823cb50c/Makefile#L260C1-L260C14 to also replace the version of mdatagen
@mx-psi mx-psi added good first issue Good for newcomers priority:p2 Medium ci-cd CI, CD, testing, build issues cmd/mdatagen labels Sep 13, 2024
@jade-guiton-dd
Copy link
Contributor

For fully testing the implications of updating mdatagen, I think it will also be necessary to forcefully rebuild .tools/mdatagen and run make generate in contrib. Adding a matching -dropreplace in the restore-contrib target will also be necessary.

@PrateekKumar1709
Copy link
Contributor

PrateekKumar1709 commented Sep 17, 2024 •
edited
Loading

Hi @mx-psi, I would like to work on this issue. Can you please assign this to me?

@TylerHelmuth TylerHelmuth mentioned this issue Sep 18, 2024
Merged
@PrateekKumar1709 PrateekKumar1709 mentioned this issue Sep 19, 2024
Merged
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this issue Oct 8, 2024
@PrateekKumar1709 @jackgopack4
Add mdatagen replace option to check-contrib target (open-telemetry#1…
daa63d7 
…1223)

#### Description
This PR addresses the issue of contrib tests not testing the updated
mdatagen version. It modifies the `check-contrib` and `restore-contrib`
targets in the Makefile to ensure that the contrib tests use the local
version of the `mdatagen` package.

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

#### Testing
All existing tests pass with `make test`

Signed-off-by: Prateek Kumar <[email protected]>
@jade-guiton-dd jade-guiton-dd mentioned this issue Oct 14, 2024
Closed
6 tasks
@mx-psi mx-psi reopened this Nov 6, 2024
@mx-psi mx-psi added release:retro Issues discussed in a release retrospective and removed good first issue Good for newcomers labels Nov 6, 2024
@mx-psi
Copy link
Member Author

mx-psi commented Nov 6, 2024

#11519 added an additional check on mdatagen:

opentelemetry-collector/cmd/mdatagen/internal/metadata.go

Line 55 in e76145a

errs = errors.Join(errs, errors.New("status must be empty for subcomponents"))
which did not make CI fail. Reopening this.

@mx-psi mx-psi mentioned this issue Nov 6, 2024
Merged
@mx-psi mx-psi added priority:p1 High and removed priority:p2 Medium labels Nov 6, 2024
This was referenced Nov 8, 2024
Open
Open
@jade-guiton-dd
Copy link
Contributor

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

I'll take a look at this.

@jade-guiton-dd jade-guiton-dd mentioned this issue Nov 13, 2024
Merged
@mx-psi mx-psi moved this from Todo to In Progress in Collector: v1 Nov 18, 2024
mx-psi pushed a commit that referenced this issue Nov 21, 2024
@jade-guiton-dd
[chore] Make check-contrib rebuild and reapply tools (#11670)
c9f8e8b 
#### 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
@github-project-automation github-project-automation bot moved this from Waiting for reviews to Done in Collector: v1 Nov 21, 2024
djaglowski pushed a commit to djaglowski/opentelemetry-collector that referenced this issue Nov 21, 2024
@jade-guiton-dd @djaglowski
[chore] Make check-contrib rebuild and reapply tools (open-telemetr…
a46fe93 
…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
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this issue Dec 19, 2024
@PrateekKumar1709 @HongChenTW
Add mdatagen replace option to check-contrib target (open-telemetry#1…
8bd6514 
…1223)

#### Description
This PR addresses the issue of contrib tests not testing the updated
mdatagen version. It modifies the `check-contrib` and `restore-contrib`
targets in the Makefile to ensure that the contrib tests use the local
version of the `mdatagen` package.

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

#### Testing
All existing tests pass with `make test`

Signed-off-by: Prateek Kumar <[email protected]>
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this issue Dec 19, 2024
@jade-guiton-dd @HongChenTW
[chore] Make check-contrib rebuild and reapply tools (open-telemetr…
c7ae432 
…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
@mx-psi
Copy link
Member Author

mx-psi commented Feb 6, 2025

The errors I am seeing on CI in #12305 make me think this is broken again :(

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Feb 6, 2025 •
edited
Loading

@mx-psi That's because my fix involving generate-contrib was disabled as part of #11795, as it sometimes failed because of non-convergent go mod tidy. I'll un-revert it when contrib#37418 gets merged. I'll reopen this issue and set it as blocked for now.

@jade-guiton-dd jade-guiton-dd reopened this Feb 6, 2025
@jade-guiton-dd jade-guiton-dd moved this from Done to Blocked in Collector: v1 Feb 6, 2025
@jade-guiton-dd jade-guiton-dd mentioned this issue Feb 6, 2025
Open
@jade-guiton-dd jade-guiton-dd moved this from Blocked to In Progress in Collector: v1 Feb 7, 2025
@jade-guiton-dd jade-guiton-dd moved this from In Progress to Blocked in Collector: v1 Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jade-guiton-dd jade-guiton-dd

Labels
ci-cd CI, CD, testing, build issues cmd/mdatagen priority:p1 High release:retro Issues discussed in a release retrospective
Projects
Collector: v1
Status: Blocked
Milestone
No milestone
3 participants
@mx-psi @PrateekKumar1709 @jade-guiton-dd