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

[chore] Use crosslink tidylist in make gotidy (attempt 2) #37418

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Jan 22, 2025

Description

This is a second attempt at PR #37142, see that one for context.

The PR was reverted in #37173, because the result of make tidylist was not a pure function of the repo state. The output of the tool depends on all go.mod files present in the repo directory, including those that may be .gitignored, such as the ones generated in cmd/otelcontribcol and cmd/oteltestbedcol.

Since then, I added a --skip flag to the tool, which allows us to make the tool ignore these problematic go.mod files.

Automatic detection of .gitignored files may be desirable if some developers have other non-gitted Go modules inside their repo directory. As this would require more advanced Git integration in the tool and should be easily avoided, I've decided to keep this more manual approach.

Link to tracking issue

Resolves #37336.

See previous PR for impacted issues in core.

Testing

See previous PR for initial testing.

I've tried running the tool locally, with and without the generated files in cmd/otelcontribcol, and the output was identical, confirming that the previous issue should be fixed.

@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review January 22, 2025 17:18
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner January 22, 2025 17:18
@@ -0,0 +1,12 @@
# This file lists modules that are known to have intra-repository circular dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

not strictly necessary to move these, but maybe they would be better suited under internal/buildscripts or in the .github directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since those files aren't scripts and aren't used as part of building the Collector, and they're used as part of local development rather than CI, those directories didn't feel appropriate, but maybe I can move it into buildscripts if we relax the definition a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codeboten What do you think would be the best location for these files?

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2025

Merging this PR, @codeboten PTAL at #37418 (comment) and if there are any follow ups we can do them in a separate PR

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2025

needs make tidylist

@mx-psi mx-psi merged commit 007b0ae into open-telemetry:main Feb 6, 2025
163 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 6, 2025
chengchuanpeng pushed a commit to chengchuanpeng/opentelemetry-collector-contrib that referenced this pull request Feb 8, 2025
…elemetry#37418)

#### Description

This is a second attempt at PR open-telemetry#37142, see that one for context.

The PR was reverted in open-telemetry#37173, because the result of `make tidylist` was
not a pure function of the repo state. The output of the tool depends on
all `go.mod` files present in the repo directory, including those that
may be `.gitignored`, such as the ones generated in `cmd/otelcontribcol`
and `cmd/oteltestbedcol`.

Since then, I [added a `--skip` flag to the
tool](open-telemetry/opentelemetry-go-build-tools#662),
which allows us to make the tool ignore these problematic `go.mod`
files.

Automatic detection of `.gitignored` files may be desirable if some
developers have other non-gitted Go modules inside their repo directory.
As this would require more advanced Git integration in the tool and
should be easily avoided, I've decided to keep this more manual
approach.

#### Link to tracking issue

Resolves open-telemetry#37336.

See previous PR for impacted issues in core.

#### Testing

See previous PR for initial testing.

I've tried running the tool locally, with and without the generated
files in `cmd/otelcontribcol`, and the output was identical, confirming
that the previous issue should be fixed.

---------

Co-authored-by: Pablo Baeyens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use crosslink tidylist for tidying
5 participants