Use golangci-lint - Githubissues

kubernetes-sigs / prometheus-adapter

An implementation of the custom.metrics.k8s.io API using Prometheus

Apache License 2.0

1.9k stars 551 forks source link

Use golangci-lint #540

Closed olivierlemasle closed 1 year ago

olivierlemasle commented 1 year ago

Since Go 1.19, gofmt formats comments. This caused make verify fail when using Go 1.19 with some license header comments.

gofmt output format also changes with Go 1.19, so hack/gofmt-all.sh is modified to be more robust. I took this opportunity to slighty improve this script.

Also, I modified Makefile:

verify should not depend of test (it should be static analysis only)
verify-generated should depend of update-generated (otherwise, it always succeeds)

PR updated: use golangci-lint

dgrisonnet commented 1 year ago

Same comment as in https://github.com/kubernetes-sigs/custom-metrics-apiserver/pull/111#issuecomment-1323555943, I'd very much prefer if we used golangci-lint.

olivierlemasle commented 1 year ago

PR updated to use golangci-lint.

In the Makefile, two changes are made in addition to adding golangci-lint:

target verify does not depend on test any more;
target verify-generated depends on update-generated.

Most other changes are formatting / style / naming.

Notable changes are:

Adding MinVersion: tls.VersionTLS12 to TLSClientConfig. Current code was reported by gosec as G402 (CWE-295): TLS MinVersion too low I used MinVersion: tls.VersionTLS12 because it's what client-go uses.
In pkg/client/api.go, req.WithContext(ctx) did not set the request's context, because WithContext is a pure function. This is fixed by using http.NewRequestWithContext.

dgrisonnet commented 1 year ago

Thank you @olivierlemasle, the PR looks good to me overall. Just a small nitpick, would you mind doing the TLS MinVersion change in a separate PR? It is important enough in my opinion to have its own changelog entry. In the meantime, you can silence the report in this PR.

olivierlemasle commented 1 year ago

@dgrisonnet Done. I'll create the follow-up PR with the TLS change after this is merged.

dgrisonnet commented 1 year ago

Thanks!

/lgtm /approve

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, olivierlemasle

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/prometheus-adapter/blob/master/OWNERS)~~ [dgrisonnet] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment