Skip to content

Commit

Permalink
feat: support PrintTextf and PrintErrorf on Reporter (#706)
Browse files Browse the repository at this point in the history
When I originally implemented `Reporter`, IDEs such as GoLand didn't
support custom `Printf` functions so I stuck with plain methods and did
the `fmt` formatting on the string; that's changed as of GoLand 2023.3
via [GO-5841](https://youtrack.jetbrains.com/issue/GO-5841) 🎉

Technically adding to `Reporter` is a breaking change but as covered [in
this
comment](#698 (comment)):

> I believe there are no other implementations (at least public on
github, from a quick code search) of this interface, and there are no
good use cases for implementing this manually instead of using one of
the preset implementations we provide.

Either way I think it's better to land these ASAP to reduce the blast
radius then to carry them around for possibly a lot longer - note that
I'm not strictly against deprecating/removing `PrintText` and
`PrintError` though I don't think there's a lot of value in keeping
them.

As penance, I've also added rich method comments for the interface.

(having said that, since this is a breaking change already maybe we
should just remove `PrintText` and `PrintError` right now)
  • Loading branch information
G-Rath authored Dec 27, 2023
1 parent 1fb5c6c commit 439e5c5
Show file tree
Hide file tree
Showing 16 changed files with 163 additions and 84 deletions.
9 changes: 9 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ linters:
- unused

linters-settings:
govet:
settings:
printf:
funcs:
- (github.com/google/osv-scanner/pkg/reporter.Reporter).PrintErrorf
- (github.com/google/osv-scanner/pkg/reporter.Reporter).PrintTextf
depguard:
rules:
regexp:
Expand All @@ -66,6 +72,9 @@ linters-settings:

issues:
exclude-rules:
- path: pkg/reporter
linters:
- dupl
- path: _test\.go
linters:
- goerr113
Expand Down
6 changes: 3 additions & 3 deletions cmd/osv-reporter/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func run(args []string, stdout, stderr io.Writer) int {
cli.VersionPrinter = func(ctx *cli.Context) {
// Use the app Writer and ErrWriter since they will be the writers to keep parallel tests consistent
tableReporter = reporter.NewTableReporter(ctx.App.Writer, ctx.App.ErrWriter, false, 0)
tableReporter.PrintText(fmt.Sprintf("osv-scanner version: %s\ncommit: %s\nbuilt at: %s\n", ctx.App.Version, commit, date))
tableReporter.PrintTextf("osv-scanner version: %s\ncommit: %s\nbuilt at: %s\n", ctx.App.Version, commit, date)
}

app := &cli.App{
Expand Down Expand Up @@ -179,11 +179,11 @@ func run(args []string, stdout, stderr io.Writer) int {
}

if errors.Is(err, osvscanner.NoPackagesFoundErr) {
tableReporter.PrintError("No package sources found, --help for usage information.\n")
tableReporter.PrintErrorf("No package sources found, --help for usage information.\n")
return 128
}

tableReporter.PrintError(fmt.Sprintf("%v\n", err))
tableReporter.PrintErrorf("%v\n", err)
}

// if we've been told to print an error, and not already exited with
Expand Down
8 changes: 4 additions & 4 deletions cmd/osv-scanner/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func run(args []string, stdout, stderr io.Writer) int {
cli.VersionPrinter = func(ctx *cli.Context) {
// Use the app Writer and ErrWriter since they will be the writers to keep parallel tests consistent
r = reporter.NewTableReporter(ctx.App.Writer, ctx.App.ErrWriter, false, 0)
r.PrintText(fmt.Sprintf("osv-scanner version: %s\ncommit: %s\nbuilt at: %s\n", ctx.App.Version, commit, date))
r.PrintTextf("osv-scanner version: %s\ncommit: %s\nbuilt at: %s\n", ctx.App.Version, commit, date)
}

osv.RequestUserAgent = "osv-scanner/" + version.OSVVersion
Expand Down Expand Up @@ -185,7 +185,7 @@ func run(args []string, stdout, stderr io.Writer) int {
var callAnalysisStates map[string]bool
if context.IsSet("experimental-call-analysis") {
callAnalysisStates = createCallAnalysisStates([]string{"all"}, context.StringSlice("no-call-analysis"))
r.PrintText("Warning: the experimental-call-analysis flag has been replaced. Please use the call-analysis and no-call-analysis flags instead.\n")
r.PrintTextf("Warning: the experimental-call-analysis flag has been replaced. Please use the call-analysis and no-call-analysis flags instead.\n")
} else {
callAnalysisStates = createCallAnalysisStates(context.StringSlice("call-analysis"), context.StringSlice("no-call-analysis"))
}
Expand Down Expand Up @@ -236,10 +236,10 @@ func run(args []string, stdout, stderr io.Writer) int {
case errors.Is(err, osvscanner.VulnerabilitiesFoundErr):
return 1
case errors.Is(err, osvscanner.NoPackagesFoundErr):
r.PrintError("No package sources found, --help for usage information.\n")
r.PrintErrorf("No package sources found, --help for usage information.\n")
return 128
}
r.PrintError(fmt.Sprintf("%v\n", err))
r.PrintErrorf("%v\n", err)
}

// if we've been told to print an error, and not already exited with
Expand Down
8 changes: 4 additions & 4 deletions internal/local/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func MakeRequest(r reporter.Reporter, query osv.BatchedQuery, offline bool, loca
return nil, err
}

r.PrintText(fmt.Sprintf("Loaded %s local db from %s\n", db.Name, db.StoredAt))
r.PrintTextf("Loaded %s local db from %s\n", db.Name, db.StoredAt)

dbs[ecosystem] = db

Expand All @@ -120,7 +120,7 @@ func MakeRequest(r reporter.Reporter, query osv.BatchedQuery, offline bool, loca

if err != nil {
// currently, this will actually only error if the PURL cannot be parses
r.PrintError(fmt.Sprintf("skipping %s as it is not a valid PURL: %v\n", query.Package.PURL, err))
r.PrintErrorf("skipping %s as it is not a valid PURL: %v\n", query.Package.PURL, err)
results = append(results, osv.Response{Vulns: []models.Vulnerability{}})

continue
Expand All @@ -134,7 +134,7 @@ func MakeRequest(r reporter.Reporter, query osv.BatchedQuery, offline bool, loca

// Is a commit based query, skip local scanning
results = append(results, osv.Response{})
r.PrintText(fmt.Sprintf("Skipping commit scanning for: %s\n", pkg.Commit))
r.PrintTextf("Skipping commit scanning for: %s\n", pkg.Commit)

continue
}
Expand All @@ -143,7 +143,7 @@ func MakeRequest(r reporter.Reporter, query osv.BatchedQuery, offline bool, loca

if err != nil {
// currently, this will actually only error if the PURL cannot be parses
r.PrintError(fmt.Sprintf("could not load db for %s ecosystem: %v\n", pkg.Ecosystem, err))
r.PrintErrorf("could not load db for %s ecosystem: %v\n", pkg.Ecosystem, err)
results = append(results, osv.Response{Vulns: []models.Vulnerability{}})

continue
Expand Down
9 changes: 5 additions & 4 deletions internal/sourceanalysis/go.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@ func goAnalysis(r reporter.Reporter, pkgs []models.PackageVulns, source models.S
cmd := exec.Command("go", "version")
_, err := cmd.Output()
if err != nil {
r.PrintText("Skipping call analysis on Go code since Go is not installed.\n")
r.PrintTextf("Skipping call analysis on Go code since Go is not installed.\n")
return
}

vulns, vulnsByID := vulnsFromAllPkgs(pkgs)
res, err := runGovulncheck(filepath.Dir(source.Path), vulns)
if err != nil {
// TODO: Better method to identify the type of error and give advice specific to the error
r.PrintError(
fmt.Sprintf("Failed to run code analysis (govulncheck) on '%s' because %s\n"+
"(the Go toolchain is required)\n", source.Path, err.Error()))
r.PrintErrorf(
"Failed to run code analysis (govulncheck) on '%s' because %s\n"+
"(the Go toolchain is required)\n", source.Path, err.Error(),
)

return
}
Expand Down
14 changes: 7 additions & 7 deletions internal/sourceanalysis/rust.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const (
func rustAnalysis(r reporter.Reporter, pkgs []models.PackageVulns, source models.SourceInfo) {
binaryPaths, err := rustBuildSource(r, source)
if err != nil {
r.PrintError(fmt.Sprintf("failed to build cargo/rust project from source: %s\n", err))
r.PrintErrorf("failed to build cargo/rust project from source: %s\n", err)
return
}

Expand All @@ -50,14 +50,14 @@ func rustAnalysis(r reporter.Reporter, pkgs []models.PackageVulns, source models
// Is a library, so need an extra step to extract the object binary file before passing to parseDWARFData
buf, err := extractRlibArchive(path)
if err != nil {
r.PrintError(fmt.Sprintf("failed to analyse '%s': %s\n", path, err))
r.PrintErrorf("failed to analyse '%s': %s\n", path, err)
continue
}
readAt = bytes.NewReader(buf.Bytes())
} else {
f, err := os.Open(path)
if err != nil {
r.PrintError(fmt.Sprintf("failed to read binary '%s': %s\n", path, err))
r.PrintErrorf("failed to read binary '%s': %s\n", path, err)
continue
}
// This is fine to defer til the end of the function as there's
Expand All @@ -68,7 +68,7 @@ func rustAnalysis(r reporter.Reporter, pkgs []models.PackageVulns, source models

calls, err := functionsFromDWARF(readAt)
if err != nil {
r.PrintError(fmt.Sprintf("failed to analyse '%s': %s\n", path, err))
r.PrintErrorf("failed to analyse '%s': %s\n", path, err)
continue
}

Expand Down Expand Up @@ -233,11 +233,11 @@ func rustBuildSource(r reporter.Reporter, source models.SourceInfo) ([]string, e
cmd.Stdout = &stdoutBuffer
cmd.Stderr = &stderrBuffer

r.PrintText("Begin building rust/cargo project\n")
r.PrintTextf("Begin building rust/cargo project\n")

if err := cmd.Run(); err != nil {
r.PrintError(fmt.Sprintf("cargo stdout:\n%s", stdoutBuffer.String()))
r.PrintError(fmt.Sprintf("cargo stderr:\n%s", stderrBuffer.String()))
r.PrintErrorf("cargo stdout:\n%s", stdoutBuffer.String())
r.PrintErrorf("cargo stderr:\n%s", stderrBuffer.String())

return nil, fmt.Errorf("failed to run `%v`: %w", cmd.String(), err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (c *ConfigManager) Get(r reporter.Reporter, targetPath string) Config {
if err != nil {
// TODO: This can happen when target is not a file (e.g. Docker container, git hash...etc.)
// Figure out a more robust way to load config from non files
// r.PrintError(fmt.Sprintf("Can't find config path: %s\n", err))
// r.PrintErrorf("Can't find config path: %s\n", err)
return Config{}
}

Expand All @@ -83,7 +83,7 @@ func (c *ConfigManager) Get(r reporter.Reporter, targetPath string) Config {

config, configErr := tryLoadConfig(configPath)
if configErr == nil {
r.PrintText(fmt.Sprintf("Loaded filter from: %s\n", config.LoadPath))
r.PrintTextf("Loaded filter from: %s\n", config.LoadPath)
} else {
// If config doesn't exist, use the default config
config = c.DefaultConfig
Expand Down
3 changes: 1 addition & 2 deletions pkg/osvscanner/optional_enricher.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package osvscanner

import (
"fmt"
"os/exec"
"strings"

Expand Down Expand Up @@ -29,7 +28,7 @@ func addCompilerVersion(r reporter.Reporter, parsedLockfile *lockfile.Lockfile)
case "go.mod":
goVer, err := getGoVersion()
if err != nil {
r.PrintText(fmt.Sprintf("cannot get go standard library version, go might not be installed: %s\n", err))
r.PrintTextf("cannot get go standard library version, go might not be installed: %s\n", err)
} else {
parsedLockfile.Packages = append(parsedLockfile.Packages, lockfile.PackageDetails{
Name: "stdlib",
Expand Down
Loading

0 comments on commit 439e5c5

Please sign in to comment.