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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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?

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
4 participants