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

Add .pre-commit-hooks.yaml file to support https://pre-commit.com/ #284

Closed
aneeshusa opened this issue Nov 8, 2018 · 9 comments
Closed
Labels
area: pre-commit enhancement New feature or improvement

Comments

@aneeshusa
Copy link
Contributor

We're starting to adopt golangci-lint at my workplace and it's been really great to use so far, thanks for making this useful tool. We're currently standardizing on pre-commit (https://pre-commit.com/) as our main entrypoint to run linters, since we need to support other languages like Python and having a single tool provides a unified experience for all developers. It also has a config file that lets us define the version of each linter to run in a structured format amenable to automated updates, unlike our current approach of ad-hoc lines in Dockerfiles or shell scripts, and ensures developers automatically have the correct version locally as well.

In order to run golangci-lint from pre-commit, there needs to be a .pre-commit-hooks.yaml file in the golangci-lint repo. Would it be possible to add one to this repo? I did some testing with a local checkout and it appears that the relevant settings from https://pre-commit.com/#pre-commit-configyaml---hooks would be id, name, description, entry, language, types, and pass_filenames (that one should be false I think).

I would have made a PR myself, but am still going through the legal review process at my workspace to have golangci-lint approved as an upstream (per #267), so I thought I would ask if one of the maintainers could add this file in the meantime. I am happy to review any PRs to do so.

Please include the following information:

  1. Version of golangci-lint: golangci-lint --version (or git commit if you don't use binary distribution)
    golangci-lint has version 1.11.2 built from f5f7701 on 2018-10-29T14:14:06Z
@aneeshusa aneeshusa changed the title Add .pre-commit-hooks file to support https://pre-commit.com/ Add .pre-commit-hooks.yaml file to support https://pre-commit.com/ Nov 8, 2018
@jirfag
Copy link
Contributor

jirfag commented Nov 10, 2018

hi!
it looks nice to implement, I'm not sure that we can implement it soon, pull requests are welcome

@nexeck
Copy link

nexeck commented Nov 15, 2018

Couldn't you just use:

repos:
  - repo: git://github.com/dnephin/pre-commit-golang
    rev: HEAD
    hooks:
      - id: golangci-lint

@aneeshusa
Copy link
Contributor Author

@nexeck the issue with using the hook from https://github.com/dnephin/pre-commit-golang is that it expects golangci-lint to already be installed. One of the main reasons my company are standardizing on pre-commit is that it manages installation and versioning for us, with the version of each linter to use specified in the .pre-commit-config.yaml. This means each of our repositories can have its own version to make it easier to upgrade golangci-lint versions one repo at a time without requiring developers to juggle multiple golangci-lint versions manually on their machines.

The way that the pre-commit support works for linters written in Go (like golangci-lint) is that it will install them from the repo where the hook is hosted at the specified version, hence the desire to have a .pre-commit-hooks.yaml in this repository. You could technically use additional_dependencies but that delegates to go get so I'm not aware of a way to specify a version for that.

@nexeck
Copy link

nexeck commented Nov 16, 2018

Cool, I didn't know that there is a way to not install the binary manually.

So, you already know what should be in the pr, and just the cla is a "problem" at the moment?

If you could give me any hint, I am happy to create the pr.

@nexeck
Copy link

nexeck commented Nov 16, 2018

I am getting the following error

Command

pre-commit try-repo ~/dev/nexeck/golangci-lint golangci-lint -a

.pre-commit-hooks.yaml

-   id: golangci-lint
    name: Golang CI Lint
    description: Run multiple golang linters
    entry: golangci-lint
    language: golang
    types: [go]

Error

An unexpected error has occurred: CalledProcessError: Command: ('/usr/local/bin/go', 'get', './...')
Return code: 2
Expected return code: 0
Output: (none)
Errors:
    go get: warning: modules disabled by GO111MODULE=auto in GOPATH/src;
        ignoring go.mod;
        see 'go help modules'

@aneeshusa
Copy link
Contributor Author

aneeshusa commented Nov 16, 2018

@nexeck yeah, that's right, I've been testing with a .pre-commit-hooks.yaml like

- id: golangci-lint
  name: TODO
  description: TODO
  entry: golangci-lint run
  types: [go]
  language: golang
  pass_filenames: false

(Note that this isn't the canonical formatting for pre-commit YAML files, your example did that better.)

Unfortunately, out of the box I'm not aware of a way to test language: golang hooks from a local checkout in pre-commit. I've been able to check out a local copy of pre-commit and apply a small hack: https://github.com/pre-commit/pre-commit/blob/6b6ebe7e3015ac72bc3c3e4e1515975e1c009a22/pre_commit/languages/golang.py#L36-L49 will always return 'unknown_src_dir' if you use a local repo, so in testing I've had to edit that function to return 'github.com/golangci/golangci-lint', after which it works.
That won't be an issue once the .pre-commit-hooks.yaml is added to this repo, as then we can use https://github.com/golangci/golangci-lint which that function handles correctly.

@nexeck
Copy link

nexeck commented Nov 18, 2018

This seems more to be a problem with golangci-lint using go mod.
Because pre-commit clones the repo into $GOPATH, go mod is not working.

@aneeshusa
Copy link
Contributor Author

@nexeck while it's true that pre-commit hasn't learned to handle repos using go modules (by setting $GO111MODULE or not cloning them into $GOPATH), because golangci-lint vendors its dependencies it will work using the current pre-commit implementation once the .pre-commit-hooks.yaml is added to this repo.

I agree that pre-commit should get updated, but I'd love to get the .pre-commit-hooks.yaml added here in the meantime since it will work without that fix, if you can make a PR.

@jirfag
Copy link
Contributor

jirfag commented Dec 22, 2018

closing because #311 was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: pre-commit enhancement New feature or improvement
Projects
None yet
Development

No branches or pull requests

4 participants