nogo: add missing go vet checks #6218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change log: https://tip.golang.org/doc/go1.22 Since musl toolchain in --config=static is not compatible with the latest Go version, disable static workflows for now. Issue is reported in uber/hermetic_cc_toolchain#171
Remove usage of `vet = True` in nogo definition. It's simply a shortcut attribute to appends some additional analyzers to the nogo binary. Instead, run `go tool vet help` and copy all listed analyzers in the doc to our analyzer. This list is more up-to-date than the one in current rules_go. Fix 2 vet issues found with new analyzers added: - Misuse of unbuffered os.Signal channel as argument to signal.Notify The channel should be buffered with minimal size of 1. - time.Since should not be used in defer statement See golang/go#60048 for more information
|
based on #6217 |
sluongng
changed the title
sluongng
requested review from
iain-macdonald,
maggie-lou,
bduffany and
vadimberezniker
maggie-lou
approved these changes
Mar 22, 2024
sluongng
force-pushed
the
sluongng/new-go-vet-analyzers
branch
from
ead5a49 to
5f640d7
Compare
bduffany
approved these changes
Mar 25, 2024
tools/metrics/grafana/grafana.go
Outdated
| @@ -174,9 +174,13 @@ func exportNormalizedDashboard(d *Dashboard) error { | |||
| return err | |||
| } | |||
| outPath := filepath.Join(dashboardsDir, fileName) | |||
| outFile, err := os.ReadFile(outPath) | |||
| if err != nil { | |||
There was a problem hiding this comment.
nit: the original intent was to write the file if it did not exist or if it contains different bytes
Suggested change
| if err != nil { | |
| if err != nil && !os.IsNotExist(err) { |
There was a problem hiding this comment.
In case the file does not exist, the byte slice content return will be nil and after the bytes.Equal result 🤔
I guess we want to write when the previous file was not there (i.e. on the first time exporting)?
There was a problem hiding this comment.
I sent a fix that would handle the nil content case.
vadimberezniker
approved these changes
Mar 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove usage of
vet = Truein nogo definition. It's simply a shortcutattribute to appends some additional analyzers to the nogo binary.
Instead, run
go tool vet helpand copy all listed analyzers in the docto our analyzer. This list is more up-to-date than the one in current
rules_go.
Fix 2 new vet issues found with new analyzers added:
Misuse of unbuffered os.Signal channel as argument to signal.Notify
The channel should be buffered with minimal size of 1.
time.Since should not be used in defer statement
See cmd/vet: time.Since should not be used in defer statement golang/go#60048 for more information
Also re-enabled
nilnessshows a range of issues in our code base that comparesnil == nil,non-nil != nil. Fixed those issues in a separate commit for easier review.