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

*: clean up remaining golangci-lint failures #2962

Merged
merged 2 commits into from
May 25, 2021
Merged

*: clean up remaining golangci-lint failures #2962

merged 2 commits into from
May 25, 2021

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented May 25, 2021

Most of these were false positives or cases where we want to ignore the
lint, but the change to the BPF generation is actually useful.

Follow-up to #2781
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

Most of these were false positives or cases where we want to ignore the
lint, but the change to the BPF generation is actually useful.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@cyphar
Copy link
Member Author

cyphar commented May 25, 2021

Should we give warnings for all lint errors? I can disable that flag again, but I noticed that some of the new errors were added in PRs that I made after we enabled the CI check -- so I wonder if the golangci-lint struggles to deal with giving new warnings if there are existing warnings? If we just make sure we only merge code that passes golangci-lint it probably doesn't hurt to just show all errors for every PR run...

It seems that golangci-lint didn't warn us about new lint errors that
were added after we enabled it, so just run the full thing and give us
all the errors on every PR run -- as long as we keep master lint-clean
it doesn't matter whether we set this or not.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@cyphar cyphar added this to the 1.0.0 milestone May 25, 2021
@cyphar cyphar requested a review from a team May 25, 2021 04:35
@cyphar cyphar requested a review from kolyshkin May 25, 2021 05:04
@hqhq hqhq merged commit bfcbc94 into opencontainers:master May 25, 2021
@cyphar cyphar deleted the golint-fixes branch May 25, 2021 11:25
@cyphar cyphar removed the request for review from kolyshkin May 25, 2021 11:25
@@ -96,7 +96,7 @@ func parseStat(data string) (stat Stat_t, err error) {
// one (PID) and two (Name) in the paren-split.
parts = strings.Split(data[i+2:], " ")
var state int
fmt.Sscanf(parts[3-3], "%c", &state)
fmt.Sscanf(parts[3-3], "%c", &state) //nolint:staticcheck // "3-3" is more readable in this context.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a fix for this #2696 (which started as a fix for this linter issue but ended up as a rewrite of parseStat to be 7x faster and more error-prone).

@kolyshkin
Copy link
Contributor

Should we give warnings for all lint errors? I can disable that flag again, but I noticed that some of the new errors were added in PRs that I made after we enabled the CI check -- so I wonder if the golangci-lint struggles to deal with giving new warnings if there are existing warnings? If we just make sure we only merge code that passes golangci-lint it probably doesn't hurt to just show all errors for every PR run...

The idea was to drop this golangci-lint flag as long as the master branch is clean of any existing issues, which is what this PR does, so yes, sure.

And yes indeed, I occasionally saw it skip some issues, probably because their "limit the scope to what this PR brings" algo is not perfect for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants