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

build(deps): bump github.com/securego/gosec/v2 from 2.20.0 to 5f0084eb01a9 #4748

Merged
merged 2 commits into from
May 25, 2024
Merged

build(deps): bump github.com/securego/gosec/v2 from 2.20.0 to 5f0084eb01a9 #4748

merged 2 commits into from
May 25, 2024

Conversation

ldez
Copy link
Member

@ldez ldez commented May 25, 2024 •
edited

The update is done by hand because gosec is not released yet, and this is an important performance issue.

This update is safe because it only contains the Go version fix.

securego/gosec@v2.20.0...5f0084e

Comparison with v1.58.1:

Benchmark 1: ./golangci-lint run --print-issued-lines=false --enable-only gosec
  Time (mean ± σ):     569.2 ms ±  21.2 ms    [User: 2152.1 ms, System: 956.9 ms]
  Range (min … max):   540.0 ms … 613.3 ms    10 runs
 
Benchmark 2: ./golangci-lint-v1.58.1 run --print-issued-lines=false --enable-only gosec
  Time (mean ± σ):     536.4 ms ±  28.1 ms    [User: 2158.7 ms, System: 886.2 ms]
  Range (min … max):   503.9 ms … 595.6 ms    10 runs

Comparison with v1.58.2:

Benchmark 1: ./golangci-lint run --print-issued-lines=false --enable-only gosec
  Time (mean ± σ):     548.2 ms ±  19.9 ms    [User: 2160.2 ms, System: 840.1 ms]
  Range (min … max):   524.6 ms … 578.9 ms    10 runs
 
Benchmark 2: ./golangci-lint-v1.58.2 run --print-issued-lines=false --enable-only gosec
  Time (mean ± σ):      2.668 s ±  0.078 s    [User: 7.854 s, System: 4.924 s]
  Range (min … max):    2.607 s …  2.862 s    10 runs

Those benchmarks are done with a golangci-lint cache clean before each benchmark.

Fixes #4735

@ldez ldez added dependencies Relates to an upstream dependency linter: update version Update version of linter go Pull requests that update Go code labels May 25, 2024
@ldez ldez added this to the next milestone May 25, 2024
@ldez ldez requested a review from bombsimon May 25, 2024 10:40
@ldez ldez mentioned this pull request May 25, 2024
Closed
@ldez ldez merged commit 2059b18 into golangci:master May 25, 2024
13 checks passed
@ldez ldez deleted the fix/gosec branch May 25, 2024 11:38
@ldez ldez modified the milestones: next, v1.59 May 26, 2024
This was referenced Jun 14, 2024
Merged
Merged
codeboten pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2024
@crobert-1
[chore][golangci-lint] Remove gosec excludes (#33574)
d15830d 
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
The upstream issue was fixed by
golangci/golangci-lint#4748, which was included
in release
[`v1.59.0`](https://github.com/golangci/golangci-lint/releases/tag/v1.59.0)
of `golangci-lint`. From local testing, we're pulling version `v1.59.1`
of `golangci-lint`, so the issue should be resolved.

Local runtime with excludes:
```
$ .tools/golangci-lint run -v --enable-only gosec
...
INFO Execution took 10.927544867s
INFO Execution took 8.011302204s
INFO Execution took 7.716441258s
INFO Execution took 7.441336833s
```
Local runtime without excludes:
```
$ .tools/golangci-lint run -v --enable-only gosec
...
INFO Execution took 9.780250262s
INFO Execution took 8.175492516s
INFO Execution took 7.550060974s
INFO Execution took 7.526585686s
```

Note: I ran `.tools/golangci-lint cache clean` between each test to
clean the cache and keep results as consistent as possible. I admit that
I don't know why the values keep going down with every run, the cache
cleaning command may not entirely be working.

**Link to tracking Issue:** <Issue number if applicable>
These excludes were introduced in
#33192
I've opened a PR in core for this issue as well:
open-telemetry/opentelemetry-collector#10411
codeboten pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Jun 14, 2024
@crobert-1
[chore][golangci-lint] Remove gosec excludes (#10411)
48af1b8 
#### Description
The upstream issue was fixed by
golangci/golangci-lint#4748, which was included
in release
[`v1.59.0`](https://github.com/golangci/golangci-lint/releases/tag/v1.59.0)
of `golangci-lint`. From local testing, we're pulling version `v1.59.0`
of `golangci-lint`, so the issue should be resolved.

Local runtime with excludes:
```
$ .tools/golangci-lint run -v --enable-only gosec
...
INFO Execution took 1.866075148s
INFO Execution took 1.218805785s
INFO Execution took 1.09527985s
```
Local runtime without excludes:
```
$ .tools/golangci-lint run -v --enable-only gosec
...
INFO Execution took 2.244716429s
INFO Execution took 1.539717296s
INFO Execution took 1.530163777s
```

Note: I ran `.tools/golangci-lint cache clean` between each test to
clean the cache and keep results as consistent as possible.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes
#10213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bombsimon bombsimon bombsimon approved these changes

Assignees
No one assigned
Labels
dependencies Relates to an upstream dependency go Pull requests that update Go code linter: update version Update version of linter
Projects
None yet
Milestone
v1.59
Development

Successfully merging this pull request may close these issues.

gosec: impact on performances (v1.58.2)
2 participants
@ldez @bombsimon