Merge pull request #1223 from cloudflare/file/owner

Validate file/owner comments even if there are no rules

Author: prymitive

cmd/pint/bench_test.go

@@ -26,6 +26,7 @@ func BenchmarkFindEntries(b *testing.B) {
 		[]string{"bench/rules"},
 		git.NewPathFilter(nil, nil, nil),
 		parser.PrometheusSchema,
+		nil,
 	)
 	for n := 0; n < b.N; n++ {
 		_, _ = finder.Find()
@@ -39,6 +40,7 @@ func BenchmarkCheckRules(b *testing.B) {
 		[]string{"bench/rules"},
 		git.NewPathFilter(nil, nil, nil),
 		parser.PrometheusSchema,
+		nil,
 	)
 	entries, err := finder.Find()
 	if err != nil {

cmd/pint/ci.go

@@ -95,20 +95,21 @@ func actionCI(c *cli.Context) error {
 
 	slog.Info("Finding all rules to check on current git branch", slog.String("base", baseBranch))
 
-	var entries []discovery.Entry
 	filter := git.NewPathFilter(
 		config.MustCompileRegexes(meta.cfg.Parser.Include...),
 		config.MustCompileRegexes(meta.cfg.Parser.Exclude...),
 		config.MustCompileRegexes(meta.cfg.Parser.Relaxed...),
 	)
-	schema := parseSchema(meta.cfg.Parser.Schema)
 
-	entries, err = discovery.NewGlobFinder([]string{"*"}, filter, schema).Find()
+	schema := parseSchema(meta.cfg.Parser.Schema)
+	allowedOwners := meta.cfg.Owners.CompileAllowed()
+	var entries []discovery.Entry
+	entries, err = discovery.NewGlobFinder([]string{"*"}, filter, schema, allowedOwners).Find()
 	if err != nil {
 		return err
 	}
 
-	entries, err = discovery.NewGitBranchFinder(git.RunGit, filter, baseBranch, meta.cfg.CI.MaxCommits, schema).Find(entries)
+	entries, err = discovery.NewGitBranchFinder(git.RunGit, filter, baseBranch, meta.cfg.CI.MaxCommits, schema, allowedOwners).Find(entries)
 	if err != nil {
 		return err
 	}
@@ -130,7 +131,7 @@ func actionCI(c *cli.Context) error {
 	}
 
 	if c.Bool(requireOwnerFlag) {
-		summary.Report(verifyOwners(entries, meta.cfg.Owners.CompileAllowed())...)
+		summary.Report(verifyOwners(entries, allowedOwners)...)
 	}
 
 	reps := []reporter.Reporter{}

cmd/pint/lint.go

@@ -75,14 +75,17 @@ func actionLint(c *cli.Context) error {
 	}
 
 	slog.Info("Finding all rules to check", slog.Any("paths", paths))
+	allowedOwners := meta.cfg.Owners.CompileAllowed()
 	finder := discovery.NewGlobFinder(
 		paths,
 		git.NewPathFilter(
 			config.MustCompileRegexes(meta.cfg.Parser.Include...),
 			config.MustCompileRegexes(meta.cfg.Parser.Exclude...),
 			config.MustCompileRegexes(meta.cfg.Parser.Relaxed...),
 		),
-		parseSchema(meta.cfg.Parser.Schema))
+		parseSchema(meta.cfg.Parser.Schema),
+		allowedOwners,
+	)
 	entries, err := finder.Find()
 	if err != nil {
 		return err
@@ -103,7 +106,7 @@ func actionLint(c *cli.Context) error {
 	}
 
 	if c.Bool(requireOwnerFlag) {
-		summary.Report(verifyOwners(entries, meta.cfg.Owners.CompileAllowed())...)
+		summary.Report(verifyOwners(entries, allowedOwners)...)
 	}
 
 	minSeverity, err := checks.ParseSeverity(c.String(minSeverityFlag))

cmd/pint/tests/0066_lint_owner.txt

@@ -46,6 +46,11 @@ groups:
 
 # pint rule/owner bob
 
+-- rules/4.yml --
+groups:
+- name: foo
+  rules: []
+
 -- .pint.hcl --
 parser {
   relaxed = ["foo", "bar", "rules/3.yml"]

cmd/pint/tests/0122_lint_owner_allowed.txt

@@ -13,7 +13,14 @@ rules/1.yml:7-8 Bug: This rule is set as owned by `bob` but `bob` doesn't match
  7 |   - alert: Invalid
  8 |     expr: up == 0
 
-level=INFO msg="Problems found" Bug=2
+rules/2.yml:4-5 Bug: `rule/owner` comments are required in all files, please add a `# pint file/owner $owner` somewhere in this file and/or `# pint rule/owner $owner` on top of each rule. (rule/owner)
+ 4 |   - alert: No Owner
+ 5 |     expr: up > 0
+
+rules/3.yml:1 Bug: This file is set as owned by `ax` but `ax` doesn't match any of the allowed owner values. (rule/owner)
+ 1 | # pint file/owner ax
+
+level=INFO msg="Problems found" Bug=4
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
 -- rules/1.yml --
 groups:
@@ -28,6 +35,22 @@ groups:
   - alert: Owner Alice
     expr: up > 0
 
+-- rules/2.yml --
+groups:
+- name: foo
+  rules:
+  - alert: No Owner
+    expr: up > 0
+
+# pint file/owner ax
+
+-- rules/3.yml --
+# pint file/owner ax
+
+groups:
+- name: foo
+  rules: []
+
 -- .pint.hcl --
 owners {
   allowed = ["alice", "max", "not-bob"]

cmd/pint/watch.go

@@ -9,6 +9,7 @@ import (
 	_ "net/http/pprof"
 	"os"
 	"os/signal"
+	"regexp"
 	"slices"
 	"strings"
 	"sync"
@@ -204,11 +205,12 @@ func actionWatch(c *cli.Context, meta actionMeta, f pathFinderFunc) error {
 	}
 
 	schema := parseSchema(meta.cfg.Parser.Schema)
+	allowedOwners := meta.cfg.Owners.CompileAllowed()
 
 	// start timer to run every $interval
 	ack := make(chan bool, 1)
 	mainCtx, mainCancel := context.WithCancel(context.WithValue(context.Background(), config.CommandKey, config.WatchCommand))
-	stop := startTimer(mainCtx, meta.workers, meta.isOffline, gen, schema, interval, ack, collector)
+	stop := startTimer(mainCtx, meta.workers, meta.isOffline, gen, schema, allowedOwners, interval, ack, collector)
 
 	quit := make(chan os.Signal, 1)
 	signal.Notify(quit, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
@@ -232,7 +234,7 @@ func actionWatch(c *cli.Context, meta actionMeta, f pathFinderFunc) error {
 	return nil
 }
 
-func startTimer(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, interval time.Duration, ack chan bool, collector *problemCollector) chan bool {
+func startTimer(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, allowedOwners []*regexp.Regexp, interval time.Duration, ack chan bool, collector *problemCollector) chan bool {
 	ticker := time.NewTicker(time.Second)
 	stop := make(chan bool, 1)
 	wasBootstrapped := false
@@ -246,7 +248,7 @@ func startTimer(ctx context.Context, workers int, isOffline bool, gen *config.Pr
 					ticker.Reset(interval)
 					wasBootstrapped = true
 				}
-				if err := collector.scan(ctx, workers, isOffline, gen, schema); err != nil {
+				if err := collector.scan(ctx, workers, isOffline, gen, schema, allowedOwners); err != nil {
 					slog.Error("Got an error when running checks", slog.Any("err", err))
 				}
 				checkIterationsTotal.Inc()
@@ -304,7 +306,7 @@ func newProblemCollector(cfg config.Config, f pathFinderFunc, minSeverity checks
 	}
 }
 
-func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema) error {
+func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, allowedOwners []*regexp.Regexp) error {
 	paths, err := c.finder(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to get the list of paths to check: %w", err)
@@ -319,6 +321,7 @@ func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool
 			config.MustCompileRegexes(c.cfg.Parser.Relaxed...),
 		),
 		schema,
+		allowedOwners,
 	).Find()
 	if err != nil {
 		return err

docs/changelog.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## v0.69.1
+
+### Fixed
+
+- `# pint file/owner` comments were not validated properly for files with no rules.
+  This is now fixed.
+
 ## v0.69.0
 
 ### Added

internal/checks/error.go

@@ -59,6 +59,7 @@ func (c ErrorCheck) Check(_ context.Context, _ discovery.Path, _ parser.Rule, _
 
 func parseRuleError(rule parser.Rule, err error) Problem {
 	var commentErr comments.CommentError
+	var ownerErr comments.OwnerError
 	var ignoreErr discovery.FileIgnoreError
 	var parseErr parser.ParseError
 
@@ -87,6 +88,18 @@ func parseRuleError(rule parser.Rule, err error) Problem {
 			Severity: Warning,
 		}
 
+	case errors.As(err, &ownerErr):
+		slog.Debug("invalid owner report", slog.Any("err", ownerErr))
+		return Problem{
+			Lines: parser.LineRange{
+				First: ownerErr.Line,
+				Last:  ownerErr.Line,
+			},
+			Reporter: discovery.RuleOwnerComment,
+			Text:     fmt.Sprintf("This file is set as owned by `%s` but `%s` doesn't match any of the allowed owner values.", ownerErr.Name, ownerErr.Name),
+			Severity: Bug,
+		}
+
 	case errors.As(err, &parseErr):
 		slog.Debug("parse error", slog.Any("err", parseErr))
 		return Problem{

internal/comments/comments.go

@@ -94,6 +94,15 @@ func (ce CommentError) Error() string {
 	return ce.Err.Error()
 }
 
+type OwnerError struct {
+	Name string
+	Line int
+}
+
+func (oe OwnerError) Error() string {
+	return oe.Name
+}
+
 type Invalid struct {
 	Err CommentError
 }
@@ -104,6 +113,7 @@ func (i Invalid) String() string {
 
 type Owner struct {
 	Name string
+	Line int
 }
 
 func (o Owner) String() string {
@@ -152,7 +162,7 @@ func parseSnooze(s string) (snz Snooze, err error) {
 	return snz, nil
 }
 
-func parseValue(typ Type, s string) (CommentValue, error) {
+func parseValue(typ Type, s string, line int) (CommentValue, error) {
 	switch typ {
 	case IgnoreFileType, IgnoreLineType, IgnoreBeginType, IgnoreEndType, IgnoreNextLineType:
 		if s != "" {
@@ -162,7 +172,7 @@ func parseValue(typ Type, s string) (CommentValue, error) {
 		if s == "" {
 			return nil, fmt.Errorf("missing %s value", FileOwnerComment)
 		}
-		return Owner{Name: s}, nil
+		return Owner{Name: s, Line: line}, nil
 	case RuleOwnerType:
 		if s == "" {
 			return nil, fmt.Errorf("missing %s value", RuleOwnerComment)
@@ -209,7 +219,7 @@ const (
 	readsValue
 )
 
-func parseComment(s string) (parsed []Comment, err error) {
+func parseComment(s string, line int) (parsed []Comment, err error) {
 	var buf strings.Builder
 	var c Comment
 
@@ -291,7 +301,7 @@ func parseComment(s string) (parsed []Comment, err error) {
 	}
 
 	if c.Type != UnknownType {
-		c.Value, err = parseValue(c.Type, strings.TrimSpace(buf.String()))
+		c.Value, err = parseValue(c.Type, strings.TrimSpace(buf.String()), line)
 		parsed = append(parsed, c)
 	}
 
@@ -303,7 +313,7 @@ func Parse(lineno int, text string) (comments []Comment) {
 	var index int
 	for sc.Scan() {
 		line := sc.Text()
-		parsed, err := parseComment(line)
+		parsed, err := parseComment(line, lineno+index)
 		if err != nil {
 			comments = append(comments, Comment{
 				Type:  InvalidComment,

internal/comments/comments_test.go

@@ -154,8 +154,11 @@ func TestParse(t *testing.T) {
 			input: "# pint file/owner bob and alice",
 			output: []comments.Comment{
 				{
-					Type:  comments.FileOwnerType,
-					Value: comments.Owner{Name: "bob and alice"},
+					Type: comments.FileOwnerType,
+					Value: comments.Owner{
+						Name: "bob and alice",
+						Line: 1,
+					},
 				},
 			},
 		},
@@ -175,8 +178,10 @@ func TestParse(t *testing.T) {
 			input: "# pint rule/owner bob and alice",
 			output: []comments.Comment{
 				{
-					Type:  comments.RuleOwnerType,
-					Value: comments.Owner{Name: "bob and alice"},
+					Type: comments.RuleOwnerType,
+					Value: comments.Owner{
+						Name: "bob and alice",
+					},
 				},
 			},
 		},
@@ -403,8 +408,11 @@ func TestParse(t *testing.T) {
 					Offset: len("code "),
 				},
 				{
-					Type:   comments.FileOwnerType,
-					Value:  comments.Owner{Name: "bob"},
+					Type: comments.FileOwnerType,
+					Value: comments.Owner{
+						Name: "bob",
+						Line: 2,
+					},
 					Offset: 1,
 				},
 			},
@@ -496,6 +504,13 @@ func TestCommentValueString(t *testing.T) {
 			}},
 			expected: "foo bar",
 		},
+		{
+			comment: comments.Invalid{Err: comments.CommentError{
+				Line: 1,
+				Err:  comments.OwnerError{Name: "foo bar"},
+			}},
+			expected: "foo bar",
+		},
 		{
 			comment:  comments.Owner{Name: "bob & alice"},
 			expected: "bob & alice",