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.

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

Jump to bottom

*: use latest release of staticcheck #5057

Merged
merged 2 commits into from Jan 4, 2019
Merged

*: use latest release of staticcheck #5057

merged 2 commits into from Jan 4, 2019

Conversation

simonpasquier
Copy link
Member

The latest release of staticcheck (2019.1) now supports Go modules so we don't have to rely on the next branch anymore. Note that this version introduces more checks as staticcheck also includes the gosimple, unused, and megacheck programs (see changelog). It is also possible to tune the configuration with a local staticcheck.conf file as well as defining exceptions directly in the code (using //lint:ignore ... comments).

The PR also fixes a couple of things in the code flagged by the additional checks.

discordianfish
discordianfish reviewed Jan 3, 2019
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM except the federation warning vs error

@discordianfish discordianfish mentioned this pull request Jan 3, 2019
Merged
@simonpasquier
*: use latest release of staticcheck
85ea168 
It also fixes a couple of things in the code flagged by the additional
checks.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
discordianfish
discordianfish approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM

SuperQ
SuperQ approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@simonpasquier
Use official release of staticcheck
d7501c7 
Also run 'go list' before staticcheck to avoid failures when downloading packages.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier
Copy link
Member Author

The latest commit gets the staticcheck binary from upstream instead of rebuilding it locally. This is similar to what we do for promu. Also because staticcheck isn't SemVer compliant (and doesn't plan to be any time soon), the go get command pulls the master branch instead of the latest tag which can be annoying as new checks being added without notice.

I've also added a hack to avoid the spurious errors (as reported in prometheus/node_exporter#1214 and prometheus/statsd_exporter#173): before running staticcheck, we run go list to populate the local cache.

@SuperQ
Copy link
Member

SuperQ commented Jan 4, 2019

@simonpasquier Thanks, I was going to propose doing the same thing. 😁

SuperQ
SuperQ reviewed Jan 4, 2019
View reviewed changes
Makefile.common
ifdef GO111MODULE
GO111MODULE=$(GO111MODULE) $(STATICCHECK) -ignore "$(STATICCHECK_IGNORE)" -checks "SA*" $(pkgs)
# 'go list' needs to be executed before staticcheck to prepopulate the modules cache.
Copy link
Member

Choose a reason for hiding this comment

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

Can you indent these comments to match the lines below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can but they show in the output then.

@simonpasquier simonpasquier mentioned this pull request Jan 4, 2019
Merged
@simonpasquier simonpasquier merged commit f678e27 into prometheus:master Jan 4, 2019
@simonpasquier simonpasquier deleted the update-staticcheck branch January 4, 2019 13:48
SuperQ added a commit to prometheus/node_exporter that referenced this pull request Jan 4, 2019
@SuperQ
Update Makefile.common
76d916c 
Update to new staticcheck method[0].

[0]: prometheus/prometheus#5057

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ mentioned this pull request Jan 4, 2019
Merged
discordianfish pushed a commit to prometheus/node_exporter that referenced this pull request Jan 4, 2019
@SuperQ @discordianfish
Update Makefile.common (#1220)
070e4b2 
* Update Makefile.common

Update to new staticcheck method[0].

[0]: prometheus/prometheus#5057

Signed-off-by: Ben Kochie <superq@gmail.com>

* Fix staticcheck errors.

Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@SuperQ @oblitorum
Update Makefile.common (prometheus#1220)
3c366a8 
* Update Makefile.common

Update to new staticcheck method[0].

[0]: prometheus/prometheus#5057

Signed-off-by: Ben Kochie <superq@gmail.com>

* Fix staticcheck errors.

Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@SuperQ @oblitorum
Update Makefile.common (prometheus#1220)
212be69 
* Update Makefile.common

Update to new staticcheck method[0].

[0]: prometheus/prometheus#5057

Signed-off-by: Ben Kochie <superq@gmail.com>

* Fix staticcheck errors.

Signed-off-by: Ben Kochie <superq@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

@brian-brazil brian-brazil brian-brazil left review comments

@discordianfish discordianfish discordianfish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@simonpasquier @SuperQ @discordianfish @brian-brazil