[SCAN-165] Use Err Reporting #3862
Conversation
53e6fba to
3a55b35
Compare
| handleRateLimit func(context.Context, error) bool | ||
| handleRateLimit func(context.Context, error, ...errorReporter) bool |
There was a problem hiding this comment.
Is errorReporter a vararg to allow for omission at the callsite? Are there places that we don't want to report the rate limit error?
| if err := dedupeReporter.UnitErr(ctx, fmt.Errorf("error caching repo info: %w", err)); err != nil { | ||
| ctx.Logger().Error(err, "failed to report unit error") | ||
| } |
There was a problem hiding this comment.
An error returned by UnitErr does not mean it failed to record the error. Instead, it's a signal to stop processing and return. That was the intention, anyway, and in practice I think it really would only happen for a context cancellation, which sources should be context-aware anyway.
I'm thinking of removing the error return from those interfaces as it's kind of confusing and not really providing much value.
| // Normalize the URL to the Gist's pull URL. | ||
| // See https://github.com/trufflesecurity/trufflehog/pull/2625#issuecomment-2025507937 | ||
| repo = gist.GetGitPullURL() |
There was a problem hiding this comment.
Why was this added here? It looks unrelated to error reporting.
| // Only report the error if a reporter was provided | ||
| if len(reporter) > 0 { | ||
| if err := reporter[0].Err(ctx, fmt.Errorf("exceeded %s rate limit", limitType)); err != nil { | ||
| ctx.Logger().Error(err, "failed to report rate limit error") | ||
| } | ||
| } |
There was a problem hiding this comment.
If you want to go with the vararg approach, I'd suggest iterating over all the reporters so at least valid calls with multiple reporters aren't silently ignored. It has the added bonus of not having to check the length of the input too.
for _, reporter := range reporters {
if err := reporter.Err(ctx, ...)
}
Description:
Currently we were not using the error reporting functionality in the GitHub Source. This PR updates that functionality.
Checklist:
make test-community)?make lintthis requires golangci-lint)?