Simplify DetailLogger interface

checker/check_runner.go

@@ -87,15 +87,18 @@ func (r *Runner) Run(ctx context.Context, c Check) CheckResult {
 		checkRequest.Dlogger = &l
 		res = c.Fn(&checkRequest)
 		if res.Error2 != nil && errors.Is(res.Error2, sce.ErrRepoUnreachable) {
-			checkRequest.Dlogger.Warn("%v", res.Error2)
+			checkRequest.Dlogger.Warn(&LogMessage{
+				Text: fmt.Sprintf("%v", res.Error2),
+			})
 			continue
 		}
 		break
 	}
 
 	// Set details.
-	res.Details2 = l.messages2
-	for _, d := range l.messages2 {
+	// TODO(#1393): Remove.
+	res.Details2 = l.Flush()
+	for _, d := range res.Details2 {
 		res.Details = append(res.Details, d.Msg.Text)
 	}
 

checker/detail_logger.go

@@ -16,13 +16,9 @@ package checker
 
 // DetailLogger logs a CheckDetail struct.
 type DetailLogger interface {
-	Info(desc string, args ...interface{})
-	Warn(desc string, args ...interface{})
-	Debug(desc string, args ...interface{})
-
-	// Functions to use for moving to SARIF format.
-	// UPGRADEv3: to rename.
-	Info3(msg *LogMessage)
-	Warn3(msg *LogMessage)
-	Debug3(msg *LogMessage)
+	Info(msg *LogMessage)
+	Warn(msg *LogMessage)
+	Debug(msg *LogMessage)
+	// Flush resets the logger state and returns collected logs.
+	Flush() []CheckDetail
 }

checker/detail_logger_impl.go

@@ -14,55 +14,36 @@
 
 package checker
 
-import (
-	"fmt"
-)
-
-// UPGRADEv2: messages2 will ultimately
-// be renamed to messages.
 type logger struct {
-	messages2 []CheckDetail
-}
-
-func (l *logger) Info(desc string, args ...interface{}) {
-	cd := CheckDetail{Type: DetailInfo, Msg: LogMessage{Text: fmt.Sprintf(desc, args...)}}
-	l.messages2 = append(l.messages2, cd)
-}
-
-func (l *logger) Warn(desc string, args ...interface{}) {
-	cd := CheckDetail{Type: DetailWarn, Msg: LogMessage{Text: fmt.Sprintf(desc, args...)}}
-	l.messages2 = append(l.messages2, cd)
+	logs []CheckDetail
 }
 
-func (l *logger) Debug(desc string, args ...interface{}) {
-	cd := CheckDetail{Type: DetailDebug, Msg: LogMessage{Text: fmt.Sprintf(desc, args...)}}
-	l.messages2 = append(l.messages2, cd)
-}
-
-// UPGRADEv3: to rename.
-func (l *logger) Info3(msg *LogMessage) {
+func (l *logger) Info(msg *LogMessage) {
 	cd := CheckDetail{
 		Type: DetailInfo,
 		Msg:  *msg,
 	}
-	cd.Msg.Version = 3
-	l.messages2 = append(l.messages2, cd)
+	l.logs = append(l.logs, cd)
 }
 
-func (l *logger) Warn3(msg *LogMessage) {
+func (l *logger) Warn(msg *LogMessage) {
 	cd := CheckDetail{
 		Type: DetailWarn,
 		Msg:  *msg,
 	}
-	cd.Msg.Version = 3
-	l.messages2 = append(l.messages2, cd)
+	l.logs = append(l.logs, cd)
 }
 
-func (l *logger) Debug3(msg *LogMessage) {
+func (l *logger) Debug(msg *LogMessage) {
 	cd := CheckDetail{
 		Type: DetailDebug,
 		Msg:  *msg,
 	}
-	cd.Msg.Version = 3
-	l.messages2 = append(l.messages2, cd)
+	l.logs = append(l.logs, cd)
+}
+
+func (l *logger) Flush() []CheckDetail {
+	ret := l.logs
+	l.logs = nil
+	return ret
 }

checks/ci_tests.go

@@ -83,8 +83,9 @@ func CITests(c *checker.CheckRequest) checker.CheckResult {
 		}
 
 		if !foundCI {
-			c.Dlogger.Debug3(&checker.LogMessage{
-				Text: fmt.Sprintf("merged PR without CI test: %d", pr.Number),
+			c.Dlogger.Debug(&checker.LogMessage{
+				Text:    fmt.Sprintf("merged PR without CI test: %d", pr.Number),
+				Version: 3,
 			})
 		}
 	}
@@ -109,11 +110,12 @@ func prHasSuccessStatus(pr *clients.PullRequest, c *checker.CheckRequest) (bool,
 			continue
 		}
 		if isTest(status.Context) || isTest(status.TargetURL) {
-			c.Dlogger.Debug3(&checker.LogMessage{
+			c.Dlogger.Debug(&checker.LogMessage{
 				Path: status.URL,
 				Type: checker.FileTypeURL,
 				Text: fmt.Sprintf("CI test found: pr: %d, context: %s", pr.Number,
 					status.Context),
+				Version: 3,
 			})
 			return true, nil
 		}
@@ -136,11 +138,12 @@ func prHasSuccessfulCheck(pr *clients.PullRequest, c *checker.CheckRequest) (boo
 			continue
 		}
 		if isTest(cr.App.Slug) {
-			c.Dlogger.Debug3(&checker.LogMessage{
+			c.Dlogger.Debug(&checker.LogMessage{
 				Path: cr.URL,
 				Type: checker.FileTypeURL,
 				Text: fmt.Sprintf("CI test found: pr: %d, context: %s", pr.Number,
 					cr.App.Slug),
+				Version: 3,
 			})
 			return true, nil
 		}

checks/contributors.go

@@ -73,8 +73,9 @@ func Contributors(c *checker.CheckRequest) checker.CheckResult {
 		names = append(names, c)
 	}
 
-	c.Dlogger.Info3(&checker.LogMessage{
-		Text: fmt.Sprintf("contributors work for: %v", strings.Join(names, ",")),
+	c.Dlogger.Info(&checker.LogMessage{
+		Text:    fmt.Sprintf("contributors work for: %v", strings.Join(names, ",")),
+		Version: 3,
 	})
 
 	reason := fmt.Sprintf("%d different companies found", len(companies))

checks/dangerous_workflow.go

@@ -298,11 +298,12 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
 		}
 		if strings.Contains(ref.Value.Value, "github.event.pull_request") {
 			line := fileparser.GetLineNumber(step.Pos)
-			dl.Warn3(&checker.LogMessage{
-				Path:   path,
-				Type:   checker.FileTypeSource,
-				Offset: line,
-				Text:   fmt.Sprintf("untrusted code checkout '%v'", ref.Value.Value),
+			dl.Warn(&checker.LogMessage{
+				Path:    path,
+				Type:    checker.FileTypeSource,
+				Offset:  line,
+				Text:    fmt.Sprintf("untrusted code checkout '%v'", ref.Value.Value),
+				Version: 3,
 				// TODO: set Snippet.
 			})
 			// Detected untrusted checkout.
@@ -411,11 +412,12 @@ func checkSecretInScript(script string, pos *actionlint.Pos, path string,
 		variable := strings.Trim(script[s:s+e+2], " ")
 		if strings.Contains(variable, "secrets.") {
 			line := fileparser.GetLineNumber(pos)
-			dl.Warn3(&checker.LogMessage{
-				Path:   path,
-				Type:   checker.FileTypeSource,
-				Offset: line,
-				Text:   fmt.Sprintf("secret accessible to pull requests '%v'", variable),
+			dl.Warn(&checker.LogMessage{
+				Path:    path,
+				Type:    checker.FileTypeSource,
+				Offset:  line,
+				Text:    fmt.Sprintf("secret accessible to pull requests '%v'", variable),
+				Version: 3,
 				// TODO: set Snippet.
 			})
 			pdata.workflowPattern[secretsViaPullRequests] = true
@@ -442,11 +444,12 @@ func checkVariablesInScript(script string, pos *actionlint.Pos, path string,
 		variable := script[s+3 : s+e]
 		if containsUntrustedContextPattern(variable) {
 			line := fileparser.GetLineNumber(pos)
-			dl.Warn3(&checker.LogMessage{
-				Path:   path,
-				Type:   checker.FileTypeSource,
-				Offset: line,
-				Text:   fmt.Sprintf("script injection with untrusted input '%v'", variable),
+			dl.Warn(&checker.LogMessage{
+				Path:    path,
+				Type:    checker.FileTypeSource,
+				Offset:  line,
+				Text:    fmt.Sprintf("script injection with untrusted input '%v'", variable),
+				Version: 3,
 				// TODO: set Snippet.
 			})
 			pdata.workflowPattern[scriptInjection] = true

checks/evaluation/binary_artifacts.go

@@ -34,10 +34,11 @@ func BinaryArtifacts(name string, dl checker.DetailLogger,
 
 	score := checker.MaxResultScore
 	for _, f := range r.Files {
-		dl.Warn3(&checker.LogMessage{
+		dl.Warn(&checker.LogMessage{
 			Path: f.Path, Type: checker.FileTypeBinary,
-			Offset: f.Offset,
-			Text:   "binary detected",
+			Offset:  f.Offset,
+			Text:    "binary detected",
+			Version: 3,
 		})
 		// We remove one point for each binary.
 		score--

checks/evaluation/branch_protection.go

@@ -15,6 +15,8 @@
 package evaluation
 
 import (
+	"fmt"
+
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
@@ -60,7 +62,10 @@ func BranchProtection(name string, dl checker.DetailLogger,
 		// so it does not provide any guarantees.
 		protected := !(b.Protected != nil && !*b.Protected)
 		if !protected {
-			dl.Warn("branch protection not enabled for branch '%s'", b.Name)
+
+			dl.Warn(&checker.LogMessage{
+				Text: fmt.Sprintf("branch protection not enabled for branch '%s'", b.Name),
+			})
 		}
 		score.scores.basic, score.maxes.basic = basicNonAdminProtection(&b, dl)
 		score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(&b, dl)
@@ -223,23 +228,29 @@ func info(dl checker.DetailLogger, doLogging bool, desc string, args ...interfac
 		return
 	}
 
-	dl.Info(desc, args...)
+	dl.Info(&checker.LogMessage{
+		Text: fmt.Sprintf(desc, args...),
+	})
 }
 
 func debug(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) {
 	if !doLogging {
 		return
 	}
 
-	dl.Debug(desc, args...)
+	dl.Debug(&checker.LogMessage{
+		Text: fmt.Sprintf(desc, args...),
+	})
 }
 
 func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) {
 	if !doLogging {
 		return
 	}
 
-	dl.Warn(desc, args...)
+	dl.Warn(&checker.LogMessage{
+		Text: fmt.Sprintf(desc, args...),
+	})
 }
 
 func basicNonAdminProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) {

checks/evaluation/code_review.go

@@ -69,8 +69,9 @@ func CodeReview(name string, dl checker.DetailLogger,
 		// Only show all warnings if all fail.
 		// We should not show warning if at least one succeeds, as this is confusing.
 		for k := range totalReviewed {
-			dl.Warn3(&checker.LogMessage{
-				Text: fmt.Sprintf("no %s reviews found", k),
+			dl.Warn(&checker.LogMessage{
+				Text:    fmt.Sprintf("no %s reviews found", k),
+				Version: 3,
 			})
 		}
 
@@ -132,9 +133,10 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 
 	for _, r := range mr.Reviews {
 		if r.State == "APPROVED" {
-			dl.Debug3(&checker.LogMessage{
+			dl.Debug(&checker.LogMessage{
 				Text: fmt.Sprintf("%s #%d merge request approved",
 					reviewPlatformGitHub, mr.Number),
+				Version: 3,
 			})
 			return true
 		}
@@ -145,9 +147,10 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 	// time on clicking the approve button.
 	if c.Committer.Login != "" &&
 		c.Committer.Login != mr.Author.Login {
-		dl.Debug3(&checker.LogMessage{
+		dl.Debug(&checker.LogMessage{
 			Text: fmt.Sprintf("%s #%d merge request approved",
 				reviewPlatformGitHub, mr.Number),
+			Version: 3,
 		})
 		return true
 	}
@@ -157,18 +160,20 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 
 func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
 	if isBot(c.Committer.Login) {
-		dl.Debug3(&checker.LogMessage{
-			Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
+		dl.Debug(&checker.LogMessage{
+			Text:    fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
+			Version: 3,
 		})
 		return true
 	}
 
 	if c.MergeRequest != nil && !c.MergeRequest.MergedAt.IsZero() {
 		for _, l := range c.MergeRequest.Labels {
 			if l == "lgtm" || l == "approved" {
-				dl.Debug3(&checker.LogMessage{
+				dl.Debug(&checker.LogMessage{
 					Text: fmt.Sprintf("%s #%d merge request approved",
 						reviewPlatformProw, c.MergeRequest.Number),
+					Version: 3,
 				})
 				return true
 			}
@@ -179,17 +184,19 @@ func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) b
 
 func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
 	if isBot(c.Committer.Login) {
-		dl.Debug3(&checker.LogMessage{
-			Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
+		dl.Debug(&checker.LogMessage{
+			Text:    fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
+			Version: 3,
 		})
 		return true
 	}
 
 	m := c.CommitMessage
 	if strings.Contains(m, "\nReviewed-on: ") &&
 		strings.Contains(m, "\nReviewed-by: ") {
-		dl.Debug3(&checker.LogMessage{
-			Text: fmt.Sprintf("%s commit approved", reviewPlatformGerrit),
+		dl.Debug(&checker.LogMessage{
+			Text:    fmt.Sprintf("%s commit approved", reviewPlatformGerrit),
+			Version: 3,
 		})
 		return true
 	}