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

chore: use ruleguard to test for missing defer statements #2837

Merged
merged 14 commits into from May 7, 2024

Conversation

willmurphyscode
Copy link
Contributor

@willmurphyscode willmurphyscode commented May 2, 2024

See #2826

Supersedes #2836.

The only hang-up I've seen is that calls to make lint can run an old version of the rules unless ./.tool/golangci-lint cache clean is called first, which could be annoying when PRing future rule changes.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode willmurphyscode changed the title Spike ruleguard spike: use ruleguard to test for missing defer statements May 2, 2024
@willmurphyscode
Copy link
Contributor Author

We see that this branch has the expected failures: https://github.com/anchore/syft/actions/runs/8930279182/job/24530010619?pr=2837#step:4:50

task: [lint] .tool/golangci-lint run --tests=false
Error: syft/linux/identify_release.go:67:4: ruleguard: internal.CloseAndLogError should be deferred right after the error returned from resolver.FileContentsByLocation is checked. (gocritic)
			contentReader, err := resolver.FileContentsByLocation(location)
			^
... snip many more ...
task: Failed to run task "static-analysis": exit status 1

Taking a look at syft/linux/identify_release.go:67:4, we see:

for _, location := range locations {
contentReader, err := resolver.FileContentsByLocation(location)
if err != nil {
logger.WithFields("error", err, "path", location.RealPath).Trace("unable to get contents")
continue
}
content, err := io.ReadAll(contentReader)
internal.CloseAndLogError(contentReader, location.AccessPath)
if err != nil {
logger.WithFields("error", err, "path", location.RealPath).Trace("unable to read contents")
continue
}
release, err := entry.fn(string(content))
if err != nil {
logger.WithFields("error", err, "path", location.RealPath).Trace("unable to parse contents")
continue
}
if release != nil {
return release
}
}
}
clearly has the issue

@willmurphyscode willmurphyscode marked this pull request as ready for review May 2, 2024 21:12
test/rules/rules.go Outdated Show resolved Hide resolved
@willmurphyscode willmurphyscode changed the title spike: use ruleguard to test for missing defer statements chore: use ruleguard to test for missing defer statements May 3, 2024
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

slick!

Copy link
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

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

Approved - Small comment on moving a wrap directive to string for an error value =)

@@ -265,8 +265,9 @@ func fetchCopyrightContents(resolver file.Resolver, dbLocation file.Location, m

reader, err := resolver.FileContentsByLocation(*location)
if err != nil {
log.Warnf("failed to fetch deb copyright contents (package=%s): %w", m.Package, err)
log.Warnf("failed to fetch deb copyright contents (package=%s): %s", m.Package, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we changing the error from %w to %s here?

@willmurphyscode willmurphyscode merged commit 3713d97 into main May 7, 2024
11 checks passed
@willmurphyscode willmurphyscode deleted the spike-ruleguard branch May 7, 2024 09:42
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.

None yet

4 participants