Introduce golangci-lint and fix findings #821
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
oxzi
added
enhancement
lippserd
requested changes
Oct 7, 2024
| @@ -5,6 +5,7 @@ import ( | |||
| "encoding" | |||
| "github.com/icinga/icinga-go-library/types" | |||
| "github.com/pkg/errors" | |||
| "math" | |||
There was a problem hiding this comment.
notification_type does not have UnmarshalJSON() like the other types and I'm not quite sure whether that's intended. Maybe we should streamline all the types so the changes here aren't necessary.
There was a problem hiding this comment.
I asked myself the same, but as it implements encoding.TextUnmarshaler, Go's JSON unmarshaller is also happy.
Otherwise, if the value implements encoding.TextUnmarshaler and the input is a JSON quoted string, Unmarshal calls encoding.TextUnmarshaler.UnmarshalText with the unquoted form of the string.
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
from
b451873 to
da13cbd
Compare
|
@lippserd: Thanks for your comment. I have addressed the changes or wrote rationales above. The commit was also split, as you suggested, just with the GitHub Action commit being the last, as otherwise - when checking the CI for each commit independently - there would be failures before these were addressed. |
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
2 times, most recently
from
6195249 to
1cd597b
Compare
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
from
1cd597b to
f3fdc16
Compare
lippserd
previously approved these changes
Oct 8, 2024
julianbrost
requested changes
Oct 8, 2024
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
from
f3fdc16 to
65cdadf
Compare
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
from
65cdadf to
07ad635
Compare
julianbrost
requested changes
Oct 9, 2024
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
2 times, most recently
from
a778e5b to
a11ba1b
Compare
oxzi
added a commit
that referenced
this pull request
Oct 10, 2024
As discussed in #821, allowing huge retention values - as the type uint64 allows - may result in overflows. Especially Go's time.AddDate() method silently overflows for huge values, resulting in unexpected comparison times. > $ ./icingadb --config <(echo 'retention: {history-days: -1}') > can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint16 ( overflow ) > > $ ./icingadb --config <(echo 'retention: {history-days: -100000}') > can't parse YAML file /proc/self/fd/11: cannot unmarshal -100000 into Go value of type uint16 ( overflow )
oxzi
added a commit
that referenced
this pull request
Oct 10, 2024
As discussed in #821, allowing huge retention values - as the type uint64 allows - may result in overflows. Especially Go's time.AddDate() method silently overflows for huge values, resulting in unexpected comparison times. > $ ./icingadb --config <(echo 'retention: {history-days: -1}') > can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint16 ( overflow ) > > $ ./icingadb --config <(echo 'retention: {history-days: 100000}') > can't parse YAML file /proc/self/fd/11: cannot unmarshal 100000 into Go value of type uint16 ( overflow )
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
from
a11ba1b to
5ede004
Compare
julianbrost
reviewed
Oct 10, 2024
The same GitHub Action already used in the icinga-go-library was added to the Go workflow.
oxzi
force-pushed
the
golangci-lint-init-and-fix
branch
from
5ede004 to
14d4070
Compare
julianbrost
approved these changes
Oct 11, 2024
lippserd
approved these changes
Oct 11, 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.
First, the same GitHub Action used in the icinga-go-library was added to the Go workflow.
Then, the findings were addressed. In general, these were mostly missing return check - when being unused anyway -, potential overflows for integer type conversions, and SHA1 warnings.
If applicable, the code was slightly modified to perform the necessary checks. Sometimes the linter has detected the checks. Sometimes not and a linter comment was added.