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

Upgrade to golangci-lint 1.61.0 #592

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Sep 15, 2024

I accidentally upgraded my golangci-lint locally, so figured I may as
well take it up a version in CI as well.

The main new drama queen for this version is fatcontext [1]. It looks
like it's a well-intentioned lint, which tries to avoid having callers
set context in loops and inadvertently create a super deep context that
causes performance issues:

func notOk() {
        ctx := context.Background()

        for i := 0; i < 10; i++ {
                ctx = context.WithValue(ctx, "key", i) // "nested context in loop"
                _ = ctx
        }
}

Unfortunately, as with many well-intentioned golangci-lint additions,
its heuristic is overly broad, and it produces a lot of false positives
(in fact, 100% of lines it found were false positives). I've marked
lines nolint:fatcontext for now, but this may be one that we disable
if it gets too annoying because it's almost certainly more cost than
benefit.

The good news is that we get upgraded version of gosec, and some of
the lines that were marked as nolint and which were false positives
on unsafe integer conversion (they were always safe) can be removed.

[1] https://github.com/Crocmagnon/fatcontext

I accidentally upgraded my golangci-lint locally, so figured I may as
well take it up a version in CI as well.

The main new drama queen for this version is fatcontext [1]. It looks
like it's a well-intentioned lint, which tries to avoid having callers
set context in loops and inadvertently create a super deep context that
causes performance issues:

    func notOk() {
            ctx := context.Background()

            for i := 0; i < 10; i++ {
                    ctx = context.WithValue(ctx, "key", i) // "nested context in loop"
                    _ = ctx
            }
    }

Unfortunately, as with many well-intentioned golangci-lint additions,
its heuristic is overly broad, and it produces a lot of false positives
(in fact, 100% of lines it found were false positives). I've marked
lines `nolint:fatcontext` for now, but this may be one that we disable
if it gets too annoying because it's almost certainly more cost than
benefit.

The good news is that we get upgraded version of `gosec`, and some of
the lines that were marked as `nolint` and which were false positives
on unsafe integer conversion (they were always safe) can be removed.

[1] https://github.com/Crocmagnon/fatcontext
@brandur brandur merged commit 64ca485 into master Sep 16, 2024
14 checks passed
@brandur brandur deleted the brandur-golang-ci-lint-1-60 branch September 16, 2024 13:16
tigrato pushed a commit to gravitational/river that referenced this pull request Dec 18, 2024
I accidentally upgraded my golangci-lint locally, so figured I may as
well take it up a version in CI as well.

The main new drama queen for this version is fatcontext [1]. It looks
like it's a well-intentioned lint, which tries to avoid having callers
set context in loops and inadvertently create a super deep context that
causes performance issues:

    func notOk() {
            ctx := context.Background()

            for i := 0; i < 10; i++ {
                    ctx = context.WithValue(ctx, "key", i) // "nested context in loop"
                    _ = ctx
            }
    }

Unfortunately, as with many well-intentioned golangci-lint additions,
its heuristic is overly broad, and it produces a lot of false positives
(in fact, 100% of lines it found were false positives). I've marked
lines `nolint:fatcontext` for now, but this may be one that we disable
if it gets too annoying because it's almost certainly more cost than
benefit.

The good news is that we get upgraded version of `gosec`, and some of
the lines that were marked as `nolint` and which were false positives
on unsafe integer conversion (they were always safe) can be removed.

[1] https://github.com/Crocmagnon/fatcontext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants