Handle internal err, return issue with empty codeowners or git dirty …

…state (#130)

internal/check/not_owned_file.go

@@ -35,17 +35,27 @@ func NewNotOwnedFile(cfg NotOwnedFileConfig) *NotOwnedFile {
 }
 
 func (c *NotOwnedFile) Check(ctx context.Context, in Input) (output Output, err error) {
+	if ctxutil.ShouldExit(ctx) {
+		return Output{}, ctx.Err()
+	}
+
 	var bldr OutputBuilder
 
-	patterns, err := c.patternsToBeIgnored(ctx, in.CodeownersEntries)
-	if err != nil {
-		return Output{}, err
+	if len(in.CodeownersEntries) == 0 {
+		bldr.ReportIssue("The CODEOWNERS file is empty. The files in the repository don't have any owner.")
+		return bldr.Output(), nil
 	}
 
-	err = c.GitCheckStatus(in.RepoDir)
+	patterns := c.patternsToBeIgnored(in.CodeownersEntries)
+
+	statusOut, err := c.GitCheckStatus(in.RepoDir)
 	if err != nil {
 		return Output{}, err
 	}
+	if string(statusOut) != "" {
+		bldr.ReportIssue("git state is dirty: commit all changes before executing this check")
+		return bldr.Output(), nil
+	}
 
 	defer func() {
 		errReset := c.GitResetCurrentBranch(in.RepoDir)
@@ -80,20 +90,16 @@ func (c *NotOwnedFile) Check(ctx context.Context, in Input) (output Output, err
 	return bldr.Output(), nil
 }
 
-func (c *NotOwnedFile) patternsToBeIgnored(ctx context.Context, entries []codeowners.Entry) ([]string, error) {
+func (c *NotOwnedFile) patternsToBeIgnored(entries []codeowners.Entry) []string {
 	var patterns []string
 	for _, entry := range entries {
-		if ctxutil.ShouldExit(ctx) {
-			return []string{}, ctx.Err()
-		}
-
 		if _, found := c.skipPatterns[entry.Pattern]; found {
 			continue
 		}
 		patterns = append(patterns, entry.Pattern)
 	}
 
-	return patterns, nil
+	return patterns
 }
 
 func (c *NotOwnedFile) AppendToGitignoreFile(repoDir string, patterns []string) error {
@@ -135,22 +141,18 @@ func (c *NotOwnedFile) GitRemoveIgnoredFiles(repoDir string) error {
 	return nil
 }
 
-func (c *NotOwnedFile) GitCheckStatus(repoDir string) error {
+func (c *NotOwnedFile) GitCheckStatus(repoDir string) ([]byte, error) {
 	gitstate := pipe.Script(
 		pipe.ChDir(repoDir),
 		pipe.Exec("git", "status", "--porcelain"),
 	)
 
 	out, stderr, err := pipe.DividedOutput(gitstate)
 	if err != nil {
-		return errors.Wrap(err, string(stderr))
+		return nil, errors.Wrap(err, string(stderr))
 	}
 
-	if string(out) != "" {
-		return fmt.Errorf("git state is dirty: commit all changes before executing %q", c.Name())
-	}
-
-	return nil
+	return out, nil
 }
 
 func (c *NotOwnedFile) GitResetCurrentBranch(repoDir string) error {

internal/printer/testdata/TestTTYPrinterPrintCheckResult/Should_print_all_reported_issues.golden.txt

@@ -3,3 +3,4 @@
     [war] line 2020: Simulate warning in line 2020
     [err] Error without line number
     [war] Warning without line number
+    [Internal Error] some check internal error

internal/printer/tty.go

@@ -19,13 +19,14 @@ type TTYPrinter struct {
 	m sync.RWMutex
 }
 
-func (tty *TTYPrinter) PrintCheckResult(checkName string, duration time.Duration, checkOut check.Output) {
+func (tty *TTYPrinter) PrintCheckResult(checkName string, duration time.Duration, checkOut check.Output, checkErr error) {
 	tty.m.Lock()
 	defer tty.m.Unlock()
 
 	header := color.New(color.Bold).FprintfFunc()
 	issueBody := color.New(color.FgWhite).FprintfFunc()
 	okCheck := color.New(color.FgGreen).FprintlnFunc()
+	errCheck := color.New(color.FgRed).FprintfFunc()
 
 	header(writer, "==> Executing %s (%v)\n", checkName, duration)
 	for _, i := range checkOut.Issues {
@@ -38,8 +39,12 @@ func (tty *TTYPrinter) PrintCheckResult(checkName string, duration time.Duration
 		issueBody(writer, " %s\n", i.Message)
 	}
 
-	if len(checkOut.Issues) == 0 {
+	switch {
+	case checkErr == nil && len(checkOut.Issues) == 0:
 		okCheck(writer, "    Check OK")
+	case checkErr != nil:
+		errCheck(writer, "    [Internal Error]")
+		issueBody(writer, " %s\n", checkErr)
 	}
 }
 

internal/printer/tty_test.go

@@ -2,6 +2,7 @@ package printer
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"testing"
 	"time"
@@ -43,7 +44,7 @@ func TestTTYPrinterPrintCheckResult(t *testing.T) {
 					Message:  "Warning without line number",
 				},
 			},
-		})
+		}, errors.New("some check internal error"))
 		// then
 		g := goldie.New(t, goldie.WithNameSuffix(".golden.txt"))
 		g.Assert(t, t.Name(), buff.Bytes())
@@ -60,7 +61,7 @@ func TestTTYPrinterPrintCheckResult(t *testing.T) {
 		// when
 		tty.PrintCheckResult("Foo Checker", time.Second, check.Output{
 			Issues: nil,
-		})
+		}, nil)
 
 		// then
 		g := goldie.New(t, goldie.WithNameSuffix(".golden.txt"))

internal/runner/runner_worker.go

@@ -9,7 +9,6 @@ import (
 	"github.com/mszostok/codeowners-validator/internal/printer"
 	"github.com/mszostok/codeowners-validator/pkg/codeowners"
 
-	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
@@ -22,7 +21,7 @@ const (
 
 // Printer prints the checks results
 type Printer interface {
-	PrintCheckResult(checkName string, duration time.Duration, checkOut check.Output)
+	PrintCheckResult(checkName string, duration time.Duration, checkOut check.Output, err error)
 	PrintSummary(allCheck int, failedChecks int)
 }
 
@@ -68,15 +67,9 @@ func (r *CheckRunner) Run(ctx context.Context) {
 				CodeownersEntries: r.codeowners,
 				RepoDir:           r.repoPath,
 			})
-			if err != nil {
-				// TODO(mszostok): add err handling (logging it internally is not enough)
-				r.log.Errorf(errors.Wrapf(err, "while executing checker %s", c.Name()).Error())
-				return
-			}
 
-			r.collectMetrics(out)
-
-			r.printer.PrintCheckResult(c.Name(), time.Since(startTime), out)
+			r.collectMetrics(out, err)
+			r.printer.PrintCheckResult(c.Name(), time.Since(startTime), out, err)
 		}(c)
 	}
 	wg.Wait()
@@ -95,14 +88,14 @@ func (r *CheckRunner) ShouldExitWithCheckFailure() bool {
 	return higherOccurredIssue <= r.treatedAsFailure
 }
 
-func (r *CheckRunner) collectMetrics(checkOut check.Output) {
+func (r *CheckRunner) collectMetrics(checkOut check.Output, err error) {
 	r.m.Lock()
 	defer r.m.Unlock()
 	for _, i := range checkOut.Issues {
 		r.allFoundIssues[i.Severity]++
 	}
 
-	if len(checkOut.Issues) > 0 {
+	if len(checkOut.Issues) > 0 || err != nil {
 		r.notPassedChecksCnt++
 	}
 }