make golangci-lint run locally with correct version #5266

grosser · 2024-07-28T21:02:59Z

What type of PR is this?

/kind cleanup
/kind failing-test

Issues:

What this PR does / why we need it:

currently verify-staticcheck does not work for me:

./hack/verify-staticcheck.sh
Installing golangci-lint v1.59.0
golangci/golangci-lint info checking GitHub for tag 'v1.59.0'
golangci/golangci-lint info found version: 1.59.0 for v1.59.0/darwin/arm64
golangci/golangci-lint info installed /Users/mgrosser/go/bin/golangci-lint
./hack/verify-staticcheck.sh: line 36: golangci-lint: command not found

Please review the above warnings.
Tips: The golangci-lint might help you fix some issues, try with the command "golangci-lint run --fix".
If the above warnings do not make sense, feel free to file an issue.

also it uses whatever random local version a user might have installed which is unreliable

Does this PR introduce a user-facing change?:

NONE

karmada-bot · 2024-07-28T21:03:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rainbowmango for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Michael Grosser <[email protected]>

codecov-commenter · 2024-07-28T21:18:24Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.24%. Comparing base (21b330c) to head (7605b69).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5266      +/-   ##
==========================================
+ Coverage   28.23%   28.24%   +0.01%     
==========================================
  Files         632      632              
  Lines       43753    43753              
==========================================
+ Hits        12355    12360       +5     
+ Misses      30496    30493       -3     
+ Partials      902      900       -2

Flag	Coverage Δ
unittests	`28.24% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

XiShanYongYe-Chang · 2024-07-29T01:56:09Z

/retest
/cc @chaosi-zju

zhzhuang-zju · 2024-07-29T01:58:03Z

./hack/verify-staticcheck.sh: line 36: golangci-lint: command not found

Refer to

karmada/hack/verify-staticcheck.sh

Line 33 in 21b330c

    
           curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin ${GOLANGCI_LINT_VER}

In fact, when golang-lint is not present, a specific version of golang-lint is automatically downloaded to the path $(go env GOPATH)/bin. Based on the output of your script, it appears that the download was successful, but ultimately resulted in a "command not found" error. This may be because the path $(go env GOPATH) is not included in your $PATH. You can check what paths are included in your current $PATH by running the command echo $PATH.

liangyuanpeng · 2024-07-29T02:03:00Z

hack/verify-staticcheck.sh

 REPO_ROOT=$(dirname "${BASH_SOURCE[0]}")/..
-GOLANGCI_LINT_VER="v1.59.0"
+GOLANGCI_LINT_VER="1.59.0"
+GOLANGCI_LINT="./bin/golangci-lint"


seems like you want to download the tools to the project folder and not GOPATH, I think this makes sense, but currently karmada downloads tools into GOPATH, which might be worth discussing.

yeah, other tools would need to follow that pattern, so better get consensus
idk of any reason not to do it 🤷

I don't have much background here, I think it would be possible to add a parameter to have the tool download to the specifically directory, defaulting to gopath, what do you think?

Maybe:

GO_PATH=$(go env GOPATH) TOOLS_PATH=${TOOLS_PATH:-$GO_PATH}

cc @zhzhuang-zju @XiShanYongYe-Chang @RainbowMango

that would fix the error

if we also want the correct version it would need to switch and replace the binaries in gopath depending on what version the current project needs, should mostly be fine just weird when using different projects with different version requirements

I think we can provide the capability to install to a specific path, but considering that minimizing the impact on users who may have already installed their own golang-lint, the default path could be set to ./bin/

grosser · 2024-07-29T02:28:26Z

I know why the error happened, but I don't want it to happen + I want it to use the correct version

chaosi-zju · 2024-07-29T02:46:32Z

also it uses whatever random local version a user might have installed which is unreliable

I think this makes sense.

Maybe the user himself has installed an unexpected version of binary under /usr/local/bin. When we execute the golangci-lint run command, we may use an unexpected version.

No matter where we place the binary we download, it would be better to use absolute paths.

zhzhuang-zju · 2024-07-29T03:04:35Z

.gitignore

+# Folders used by make
 _tmp/
 _output/
+bin/


Suggested change

bin/

_tool

How about mimicking the _output directory and naming it _tool or _bin? This directory can be specifically used to place tools for maintaining Karmada.

the beauty of bin/ is that if users have PATH=./bin then things "just work" :)

not against _tool either to be consistent with the other _ folders
never really saw a project use _ folders 🤷
everything could be in tmp/ (tmp/output tmp/bin/ etc)

but either way is fine for me

the beauty of bin/ is that if users have PATH=./bin then things "just work" :)

agree

chaosi-zju · 2024-07-29T03:10:10Z

hack/verify-staticcheck.sh

 fi

-if golangci-lint run; then
+if ${GOLANGCI_LINT} run; then


Suggested change

if ${GOLANGCI_LINT} run; then

if $(go env GOPATH)/bin/golangci-lint run; then

maybe this is enough to solve your problem.

As for where to put the downloaded files, we can continue to discuss in the above comment.

XiShanYongYe-Chang · 2024-08-03T02:25:07Z

Hi @grosser, how is it going now?

grosser · 2024-08-03T02:28:26Z

afaik this is ready to merge
... then can make more PRs to move the other local tools to ./bin (did not check how many more there are)

XiShanYongYe-Chang · 2024-08-03T03:02:00Z

There seems to be some discussion going on.

grosser · 2024-08-03T04:00:51Z

yeah, not sure where that discussion is going or what the exact proposal is
... still think bin/ is the best and would work for everyone

karmada-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. labels Jul 28, 2024

karmada-bot requested review from XiShanYongYe-Chang and zhzhuang-zju July 28, 2024 21:03

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 28, 2024

make golangci-lint run locally with correct version

7605b69

Signed-off-by: Michael Grosser <[email protected]>

grosser force-pushed the grosser/lint branch from 47292ab to 7605b69 Compare July 28, 2024 21:07

karmada-bot requested a review from chaosi-zju July 29, 2024 01:56

liangyuanpeng reviewed Jul 29, 2024

View reviewed changes

zhzhuang-zju reviewed Jul 29, 2024

View reviewed changes

chaosi-zju reviewed Jul 29, 2024

View reviewed changes

	if ${GOLANGCI_LINT} run; then
	if $(go env GOPATH)/bin/golangci-lint run; then

make golangci-lint run locally with correct version #5266

Are you sure you want to change the base?

make golangci-lint run locally with correct version #5266

Uh oh!

Conversation

grosser commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karmada-bot commented Jul 28, 2024

Uh oh!

codecov-commenter commented Jul 28, 2024

Codecov Report

Uh oh!

XiShanYongYe-Chang commented Jul 29, 2024

Uh oh!

zhzhuang-zju commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangyuanpeng Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grosser commented Jul 29, 2024

Uh oh!

chaosi-zju commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XiShanYongYe-Chang commented Aug 3, 2024

Uh oh!

grosser commented Aug 3, 2024

Uh oh!

XiShanYongYe-Chang commented Aug 3, 2024

Uh oh!

grosser commented Aug 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

grosser commented Jul 28, 2024 •

edited

Loading

liangyuanpeng Jul 29, 2024 •

edited

Loading