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

Upgrade to golangci-lint v1.53.3 #258

Merged
merged 4 commits into from
Jul 21, 2023
Merged

Upgrade to golangci-lint v1.53.3 #258

merged 4 commits into from
Jul 21, 2023

Conversation

roobre
Copy link
Collaborator

@roobre roobre commented Jul 21, 2023

Description

Fixes #232

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

@roobre roobre requested a review from pablochacin July 21, 2023 10:38
@pablochacin
Copy link
Collaborator

Running locally fails:

make lint
docker run --rm -v /home/pablo/go/src/github.com/grafana/xk6-disruptor:/disruptor -w /disruptor golangci/golangci-lint:v1.53.3 golangci-lint run
level=warning msg="[runner] Can't run linter goanalysis_metalinter: musttag: running `go list all`: exit status 1"
level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: musttag: running `go list all`: exit status 1\n\n"
make: *** [Makefile:45: lint] Error 3

@roobre
Copy link
Collaborator Author

roobre commented Jul 21, 2023

@pablochacin Interesting, I thought this issue was on my end due to my source tree not being readable by others. The root cause is that git refuses to run on directories the current user doesn't own. This did not happen in the past, so my best guess is that the updated container image also updates the git binary. Outside of the container, this issue does not occur.

I'll check to see if there's a workaround.

@roobre
Copy link
Collaborator Author

roobre commented Jul 21, 2023

@pablochacin I've added a workaround in bdf85cd. The issue in play is a bit obscure, I've added an extended explanation in the commit message. LMK if it seems clear, or if you have concerns with it.

This works around the issue of `go list all` failing when invoking git operations, as git refuses to run in a directory owned by someone else by default:

```
$> docker run --rm -ti -v `pwd`:/disruptor -w /disruptor golangci/golangci-lint:v1.53.3 go list all
go: downloading github.com/go-openapi/jsonpointer v0.19.6
error obtaining VCS status: exit status 128
	Use -buildvcs=false to disable VCS stamping.
```

```
$> docker run --rm -ti -v `pwd`:/disruptor -w /disruptor golangci/golangci-lint:v1.53.3 git status
fatal: detected dubious ownership in repository at '/disruptor'
To add an exception for this directory, call:

	git config --global --add safe.directory /disruptor
```
Copy link
Collaborator

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

LGTM

@pablochacin pablochacin merged commit 4857a35 into main Jul 21, 2023
7 checks passed
@pablochacin pablochacin deleted the golangci-1-53-3 branch July 21, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update golangci
2 participants