-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix golangci-lint installation path in dev container #4729
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
base: master
Are you sure you want to change the base?
Conversation
RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.50.0 | ||
RUN golangci-lint --version |
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.
At first, I fixed this line like below:
RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b ./.bin
But golangci-lint binary is not installed to ./.bin
for some reason.
So I use postStartCommand
in devcontainer.json
and it worked.
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.
After some testing, I deleted golangci-lint installation from Dockerfile
in 32e7b92.
I found that major golang dev tool is installed with this specify. (dlv
golangci-lint
golint
gomodifytags
goplay
gopls
gotests
impl
revive
staticcheck
)
https://github.com/kyu08/lazygit/blob/32e7b92311b933ab76e160d11b622ee190d64023/.devcontainer/devcontainer.json#L15-L17
So we don't need install golangci-lint in Dockerfile
.
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.
It would still be good to install the specific version that we are using on CI (2.2.1 at this time), to avoid false positives when golanci-lint gets updated and adds new checks that are enabled by default. This was the very point of introducing the .bin folder.
Of course, if we then later bump the golangci-lint version that we are using, people will still have to fix their installed version manually, but you also have to do that when working locally, and I don't see a good way around that. (We could add a version check to scripts/lint.sh
and warn about this, but this wouldn't help with the lint-on-save function in vscode, which is the main use of linting for me.)
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 see.
Then go tool may be suitable solution.
It allows us to manage versions of tools which are written in Go using go.mod
.
So we can unify versions of dev tools such as golang-lint or gofumpt easily.
I'll take some time to try it out next week.
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.
No experience with that, but on https://golangci-lint.run/welcome/install/ it says We don't recommend using go tool.
They don't really say why, though.
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.
This is a great idea. I made a separate PR (#4733) to take this a bit further and also make it work for lint-on-save in vscode. I didn't test if this works in a dev container, but it should; if you have time to test this, that would be great.
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.
Thanks.
if you have time to test this, that would be great.
Just to clarify, are you asking me to test #4733 specifically, or to test changing the make lint target to go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.2.1 run
?
If it's second option, I confirmed it worked.
This is a head of the command.
And this is the tail of it.

About your PR, it'll take a while because I'm not familiar with VSCode. Maybe sometime in next week.
And how should we divide the roles between this PR and your
PR(#4733 )?
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.
No, I meant to test #4733. If I'm not missing anything, that PR can completely replace this one.
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.
OK.
If you don't mind, could you add me as co-author of your first commit of #4733?
I'd be happy if my contribution remained in a clear and visible form.
Anyway, I'll take time for it next week.
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 you don't mind, could you add me as co-author of your first commit of #4733?
Sure. Let me know if you'd like to appear with a different name or email address than what I added. Also mentioning you in the PR description now.
.gitignore
Outdated
@@ -14,7 +14,8 @@ coverage.txt | |||
.idea/ | |||
|
|||
# Binaries | |||
.bin/ | |||
.bin/* |
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.
This is necessary to ignoring binaries.
.gitignore
Outdated
@@ -14,7 +14,8 @@ coverage.txt | |||
.idea/ | |||
|
|||
# Binaries | |||
.bin/ | |||
.bin/* | |||
!.bin/.gitkeep |
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.
This line is necessary to manage .bin directory under Git.
a5c3f63
to
e3f422f
Compare
Thanks. I'm very unfamiliar with containers in general, and with dev containers in VS Code in particular, but I played with it a little bit just now to get a feeling for how all this works. But I don't think this solution works well enough. When I spin up my existing working copy in a container, it will overwrite my Mac binary with a linux binary. If I then switch back to local development, linting now longer works on the Mac. For the If anybody has ideas how to improve all this, I'm all ears. |
Thanks for the input. I understand your concern. As far as I know, many of dev containers users don't switch frequently between dev containers and local develop environment. I think one of the purposes of using dev containers is for avoiding construct develop environment locally. So this might not be a problem in practice. |
Sounds reasonable, but when I played with this, I thought this might be useful for cases where I need to debug a linux-specific problem. Instead of spinning up my linux VM I could simply switch to a dev container with the existing working copy, might be easier. So then I would run into this problem. But what if we change it to keep the installation of golangci-lint in Dockerfile (just change it to install 2.2.1 so that it matches what we use on CI), and then in the postStartCommand add something like With the mkdir we can also drop the .gitkeep change, which I find a bit ugly. |
That makes sense. As you said, dev containers is useful for debugging on Linux.
I'm not sure if your approach resolves the issue.
I think the same issue could occur in reverse scenerio(linux -> macOS) as well. If I miss or misunderstand something, please let me know.
I have no objections about this. |
Of course, it doesn't solve it perfectly, but I think it's a reasonable compromise that works well enough in the important cases:
This would be perfectly acceptable for me. |
Ah I understand. Thanks for the information. I'll fix it tomorrow as your suggestion. |
ccb97e6
to
32e7b92
Compare
.devcontainer/devcontainer.json
Outdated
@@ -62,7 +62,7 @@ | |||
|
|||
// See https://www.kenmuse.com/blog/avoiding-dubious-ownership-in-dev-containers/ for the safe.directory part | |||
// The defaultBranch part is required for our deprecated integration tests. | |||
"postStartCommand": "git config --global --add safe.directory ${containerWorkspaceFolder} && git config --global init.defaultBranch master", | |||
"postStartCommand": "git config --global --add safe.directory ${containerWorkspaceFolder} && git config --global init.defaultBranch master && test -e .bin/golangci-lint || { mkdir -p .bin && ln -s $(go env GOPATH)/bin/golangci-lint .bin/golangci-lint; }", |
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 added just ;
. And it works.
@stefanhaller Please check the comments and diff. |
Problem I encountered
Currently,
make lint
is not executable in Dev Containers or Codespaces because installation path of golangci-lint is not a expected location ofscripts/lint.sh
like below.$(go env GOPATH)/bin
scripts/lint.sh
expects./.bin
How I solved and tested
So I've fixed installation path of golangci-lint to under
./.bin
.I have tested that I can run
make lint
after launching dev container like below.Please check if the PR fulfills these requirements
Cheatsheets are up-to-date (rungo generate ./...
)Code has been formatted (see here)Tests have been added/updated (see here for the integration test guide)Text is internationalised (see here)If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)Docs have been updated if necessaryYou've read through your own file changes for silly mistakes etc