-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Use go workspace for golangci lint #20788
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
Use go workspace for golangci lint #20788
Conversation
Adds workspace_relative_modules, and run_for_all_workspace_modules which runs a given function for all the workspace module from the top-level of the repository, without iterating by module/directory. Signed-off-by: Ivan Valdes <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 29 files with indirect coverage changes @@ Coverage Diff @@
## main #20788 +/- ##
==========================================
+ Coverage 69.14% 69.22% +0.08%
==========================================
Files 422 422
Lines 34821 34824 +3
==========================================
+ Hits 24076 24107 +31
+ Misses 9340 9323 -17
+ Partials 1405 1394 -11 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| # run_for_all_workspace_modules [cmd] | ||
| # run given command across all workspace modules | ||
| # (unless the set is limited using ${PKG} or / ${USERMOD}) | ||
| function run_for_all_workspace_modules { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I was hoping that tools like golangci-lint support workspaces. So we could just call it directly and remove the needs for dedicated scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that golangci-lint receives the list of modules to check. If we don't pass one, then it would only check the top-level module. We'd need to evaluate tool by tool, as each one may work differently. I did this work before in #19314. There are even some tools that still need the old (current) way of changing directories, if I remember correctly.
This is somewhat similar to how k/k runs golangci-lint (https://github.com/kubernetes/kubernetes/blob/adce99702ab66c41b816b13206000b882deaf0e0/hack/verify-golangci-lint.sh#L179-L199).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would removing a top level module help? Do need it with go.sum managing dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove the top-level module, many things will require changes. On top of that, this wouldn't address calling golangci-lint. It fails with the error:
ERRO [linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies
🤕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified lint_pass, but we still need this helper so tools receive the list of modules as an argument.
Signed-off-by: Ivan Valdes <[email protected]>
eae8bd1 to
a2c1df5
Compare
|
/retest |
|
Unrelated to the PR, but look slike the migration to workspaces broke BOM: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivanvc, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Yes, it's the workspace, but it's also an issue with the tool we're using to generate the BOM. I addressed that with 8b02416. The issue is that in a fresh clone, if you run the tool, it will fail the first time because it needs to download dependencies, and it appears the tool is reading stdout. So, the workaround to fix it is to run it first and let it fail. And then rerun it, while capturing the output, and it works fine. I could add a |
With #19423 merged, we can start cleaning up our tangled scripts. This PR aims to run golangci-lint from the top level of the repository.
Part of #18409.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.