Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix golangci-lint installation path in dev container #4729

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3f422f
Fix golangci-lint installation path in dev container
kyu08 Jul 12, 2025
32e7b92
fixup! Fix golangci-lint installation path in dev container
kyu08 Jul 13, 2025
718ed5c
fixup! Fix golangci-lint installation path in dev container
kyu08 Jul 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .devcontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ ARG VARIANT=1-bullseye
FROM golang:${VARIANT}

RUN go install mvdan.cc/gofumpt@latest
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
Comment on lines -8 to -9
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.)

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

@kyu08 kyu08 Jul 13, 2025 •
edited
Loading

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.

Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping @kyu08: any more input on this? I'm going to merge my PR on the weekend, in case you want to look at it before that.

I tested my branch with a dev container now, and everything seems to work fine.

The only thing that puzzles me a bit is the logs you posted above, with the go: downloading ... lines; I'm not seeing these, neither locally nor in the dev container. Does go have some global verbosity setting that might influence this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanhaller
Sorry for the late reply. At first, thanks for the adding me as a co-author of your commit.

any more input on this? I'm going to merge my PR on the weekend, in case you want to look at it before that.
I tested my branch with a dev container now, and everything seems to work fine.

No. I was thinking of testing your PR but, if you had already tested it, I think it's ready to merge. I'm sorry I couldn't reply and test it.

The only thing that puzzles me a bit is the logs you posted above, with the go: downloading ... lines; I'm not seeing these, neither locally nor in the dev container. Does go have some global verbosity setting that might influence this?

Hmm, TBH, I don't know why. But it shouldn't be a problem, as you probably think so as well.


# [Optional] Uncomment this section to install additional OS packages.
# RUN apt-get update && export DEBIAN_FRONTEND=noninteractive \
Expand Down
2 changes: 1 addition & 1 deletion .devcontainer/devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }",
Copy link
Contributor Author

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.


// Set `remoteUser` to `root` to connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root.
"remoteUser": "vscode"
Expand Down
Loading