From 02147c6ac5850f4ba918468770903b52eac12112 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Tue, 10 Dec 2024 11:53:26 +0000 Subject: [PATCH] Validate file/owner comments even if there are no rules --- cmd/pint/bench_test.go | 2 + cmd/pint/ci.go | 11 +++-- cmd/pint/lint.go | 7 ++- cmd/pint/tests/0066_lint_owner.txt | 5 ++ cmd/pint/tests/0122_lint_owner_allowed.txt | 25 +++++++++- cmd/pint/watch.go | 11 +++-- docs/changelog.md | 7 +++ internal/checks/error.go | 13 ++++++ internal/comments/comments.go | 20 ++++++-- internal/comments/comments_test.go | 27 ++++++++--- internal/discovery/discovery.go | 40 ++++++++++++++-- internal/discovery/discovery_test.go | 2 +- internal/discovery/git_branch.go | 26 +++++++---- internal/discovery/git_branch_test.go | 54 ++++++++++++---------- internal/discovery/glob.go | 19 ++++---- internal/discovery/glob_test.go | 36 +++++++-------- internal/parser/read.go | 23 ++++----- internal/parser/read_test.go | 6 +-- 18 files changed, 232 insertions(+), 102 deletions(-) diff --git a/cmd/pint/bench_test.go b/cmd/pint/bench_test.go index 7a86504b..178ef536 100644 --- a/cmd/pint/bench_test.go +++ b/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 { diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go index ba342aab..8f482b30 100644 --- a/cmd/pint/ci.go +++ b/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{} diff --git a/cmd/pint/lint.go b/cmd/pint/lint.go index 0d743703..0c240d30 100644 --- a/cmd/pint/lint.go +++ b/cmd/pint/lint.go @@ -75,6 +75,7 @@ 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( @@ -82,7 +83,9 @@ func actionLint(c *cli.Context) error { 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)) diff --git a/cmd/pint/tests/0066_lint_owner.txt b/cmd/pint/tests/0066_lint_owner.txt index 7ee75509..35b31586 100644 --- a/cmd/pint/tests/0066_lint_owner.txt +++ b/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"] diff --git a/cmd/pint/tests/0122_lint_owner_allowed.txt b/cmd/pint/tests/0122_lint_owner_allowed.txt index f0493539..9951691a 100644 --- a/cmd/pint/tests/0122_lint_owner_allowed.txt +++ b/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"] diff --git a/cmd/pint/watch.go b/cmd/pint/watch.go index 3735b64d..2550cf6c 100644 --- a/cmd/pint/watch.go +++ b/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 diff --git a/docs/changelog.md b/docs/changelog.md index 4f75b5ca..1c5f2eaa 100644 --- a/docs/changelog.md +++ b/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 diff --git a/internal/checks/error.go b/internal/checks/error.go index 9fc7090a..993454d1 100644 --- a/internal/checks/error.go +++ b/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{ diff --git a/internal/comments/comments.go b/internal/comments/comments.go index 61e8211e..c3c51570 100644 --- a/internal/comments/comments.go +++ b/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, diff --git a/internal/comments/comments_test.go b/internal/comments/comments_test.go index 9cae1d03..3bb0858d 100644 --- a/internal/comments/comments_test.go +++ b/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", diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index 7cba8f0d..204b912e 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "log/slog" + "regexp" "slices" "time" @@ -85,8 +86,8 @@ type Entry struct { State ChangeType } -func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool, schema parser.Schema) (entries []Entry, err error) { - content, fileComments, err := parser.ReadContent(r) +func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool, schema parser.Schema, allowedOwners []*regexp.Regexp) (entries []Entry, err error) { + content, err := parser.ReadContent(r) if err != nil { return nil, err } @@ -96,14 +97,19 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool, sche Last: content.TotalLines, } + var badOwners []comments.Comment var fileOwner string var disabledChecks []string - for _, comment := range fileComments { + for _, comment := range content.FileComments { // nolint:exhaustive switch comment.Type { case comments.FileOwnerType: owner := comment.Value.(comments.Owner) - fileOwner = owner.Name + if isValidOwner(owner.Name, allowedOwners) { + fileOwner = owner.Name + } else { + badOwners = append(badOwners, comment) + } case comments.FileDisableType: disable := comment.Value.(comments.Disable) if !slices.Contains(disabledChecks, disable.Match) { @@ -191,6 +197,32 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool, sche }) } + if len(rules) == 0 && len(badOwners) > 0 { + for _, comment := range badOwners { + owner := comment.Value.(comments.Owner) + entries = append(entries, Entry{ + Path: Path{ + Name: sourcePath, + SymlinkTarget: reportedPath, + }, + PathError: comments.OwnerError(owner), + ModifiedLines: contentLines.Expand(), + }) + } + } + slog.Debug("File parsed", slog.String("path", sourcePath), slog.Int("rules", len(entries))) return entries, nil } + +func isValidOwner(s string, valid []*regexp.Regexp) bool { + if len(valid) == 0 { + return true + } + for _, v := range valid { + if v.MatchString(s) { + return true + } + } + return false +} diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go index 9224f910..0a3f5941 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -338,7 +338,7 @@ groups: fmt.Sprintf("rPath=%s sPath=%s strict=%v title=%s", tc.reportedPath, tc.sourcePath, tc.isStrict, tc.title), func(t *testing.T) { r := tc.sourceFunc(t) - entries, err := readRules(tc.reportedPath, tc.sourcePath, r, tc.isStrict, parser.PrometheusSchema) + entries, err := readRules(tc.reportedPath, tc.sourcePath, r, tc.isStrict, parser.PrometheusSchema, nil) if tc.err != "" { require.EqualError(t, err, tc.err) } else { diff --git a/internal/discovery/git_branch.go b/internal/discovery/git_branch.go index 9bc6c916..1c13222c 100644 --- a/internal/discovery/git_branch.go +++ b/internal/discovery/git_branch.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "log/slog" + "regexp" "slices" "sort" "strings" @@ -19,22 +20,25 @@ func NewGitBranchFinder( baseBranch string, maxCommits int, schema parser.Schema, + allowedOwners []*regexp.Regexp, ) GitBranchFinder { return GitBranchFinder{ - gitCmd: gitCmd, - filter: filter, - baseBranch: baseBranch, - maxCommits: maxCommits, - schema: schema, + gitCmd: gitCmd, + filter: filter, + baseBranch: baseBranch, + maxCommits: maxCommits, + schema: schema, + allowedOwners: allowedOwners, } } type GitBranchFinder struct { - gitCmd git.CommandRunner - baseBranch string - filter git.PathFilter - maxCommits int - schema parser.Schema + gitCmd git.CommandRunner + baseBranch string + filter git.PathFilter + allowedOwners []*regexp.Regexp + maxCommits int + schema parser.Schema } func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) { @@ -63,6 +67,7 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) { bytes.NewReader(change.Body.Before), !f.filter.IsRelaxed(change.Path.Before.Name), f.schema, + nil, ) if err != nil { slog.Debug("Cannot read before rules", slog.String("path", change.Path.Before.Name), slog.Any("err", err)) @@ -73,6 +78,7 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) { bytes.NewReader(change.Body.After), !f.filter.IsRelaxed(change.Path.After.Name), f.schema, + f.allowedOwners, ) if err != nil { return nil, fmt.Errorf("invalid file syntax: %w", err) diff --git a/internal/discovery/git_branch_test.go b/internal/discovery/git_branch_test.go index 8a9d000e..8bc1965e 100644 --- a/internal/discovery/git_branch_test.go +++ b/internal/discovery/git_branch_test.go @@ -71,6 +71,7 @@ func TestGitBranchFinder(t *testing.T) { "main", 50, parser.PrometheusSchema, + nil, ), entries: nil, err: "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status main..HEAD]", @@ -86,6 +87,7 @@ func TestGitBranchFinder(t *testing.T) { "master", 50, parser.PrometheusSchema, + nil, ), entries: nil, err: "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status master..HEAD]", @@ -103,7 +105,7 @@ func TestGitBranchFinder(t *testing.T) { commitFile(t, "rules.yml", "# v2-3\n", "v2-3") commitFile(t, "rules.yml", "# v2-4\n", "v2-4") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 3, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 3, parser.PrometheusSchema, nil), entries: nil, err: "number of commits to check (4) is higher than maxCommits (3), exiting", }, @@ -123,6 +125,7 @@ func TestGitBranchFinder(t *testing.T) { "main", 4, parser.PrometheusSchema, + nil, ), entries: nil, err: "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --first-parent --format=%H --name-status main..HEAD]", @@ -143,6 +146,7 @@ func TestGitBranchFinder(t *testing.T) { "main", 4, parser.PrometheusSchema, + nil, ), entries: nil, err: "failed to get commit message for c1: mock git error: [show -s --format=%B c1]", @@ -169,6 +173,7 @@ func TestGitBranchFinder(t *testing.T) { "main", 4, parser.PrometheusSchema, + nil, ), entries: nil, err: "failed to run git blame for rules.yml: mock git error: [blame --line-porcelain c1 -- rules.yml]", @@ -183,7 +188,7 @@ func TestGitBranchFinder(t *testing.T) { commitFile(t, "rules.yml", "# v2\n", "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: nil, }, { @@ -208,7 +213,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -243,7 +248,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -272,7 +277,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -301,7 +306,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(nil, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(nil, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -342,6 +347,7 @@ groups: "main", 4, parser.PrometheusSchema, + nil, ), entries: nil, }, @@ -367,7 +373,7 @@ groups: expr: count(up == 1) `, "v2\nskip this commit\n[skip ci]\n") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: nil, }, { @@ -392,7 +398,7 @@ groups: expr: count(up == 1) `, "v2\nskip this commit\n[no ci]\n") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: nil, }, { @@ -412,7 +418,7 @@ groups: require.NoError(t, err, "git add") gitCommit(t, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Added, @@ -457,7 +463,7 @@ groups: expr: count(up) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -529,7 +535,7 @@ groups: for: 0s `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -569,7 +575,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -609,7 +615,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -653,7 +659,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -707,7 +713,7 @@ groups: expr: count(up) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Added, @@ -759,7 +765,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -821,7 +827,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -890,7 +896,7 @@ groups: foo: bar `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -928,7 +934,7 @@ groups: gitCommit(t, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Moved, @@ -963,7 +969,7 @@ groups: gitCommit(t, "v3") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Moved, @@ -1001,7 +1007,7 @@ groups: gitCommit(t, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Moved, @@ -1061,7 +1067,7 @@ groups: expr: sum(up) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Added, @@ -1105,7 +1111,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Modified, @@ -1161,7 +1167,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4, parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, diff --git a/internal/discovery/glob.go b/internal/discovery/glob.go index 2d1409fb..0286e9b6 100644 --- a/internal/discovery/glob.go +++ b/internal/discovery/glob.go @@ -7,23 +7,26 @@ import ( "log/slog" "os" "path/filepath" + "regexp" "github.com/cloudflare/pint/internal/git" "github.com/cloudflare/pint/internal/parser" ) -func NewGlobFinder(patterns []string, filter git.PathFilter, schema parser.Schema) GlobFinder { +func NewGlobFinder(patterns []string, filter git.PathFilter, schema parser.Schema, allowedOwners []*regexp.Regexp) GlobFinder { return GlobFinder{ - patterns: patterns, - filter: filter, - schema: schema, + patterns: patterns, + filter: filter, + schema: schema, + allowedOwners: allowedOwners, } } type GlobFinder struct { - patterns []string - filter git.PathFilter - schema parser.Schema + filter git.PathFilter + patterns []string + allowedOwners []*regexp.Regexp + schema parser.Schema } func (f GlobFinder) Find() (entries []Entry, err error) { @@ -69,7 +72,7 @@ func (f GlobFinder) Find() (entries []Entry, err error) { if err != nil { return nil, err } - el, err := readRules(fp.target, fp.path, fd, !f.filter.IsRelaxed(fp.target), f.schema) + el, err := readRules(fp.target, fp.path, fd, !f.filter.IsRelaxed(fp.target), f.schema, f.allowedOwners) if err != nil { fd.Close() return nil, fmt.Errorf("invalid file syntax: %w", err) diff --git a/internal/discovery/glob_test.go b/internal/discovery/glob_test.go index 5def1d35..4aacd8f0 100644 --- a/internal/discovery/glob_test.go +++ b/internal/discovery/glob_test.go @@ -34,32 +34,32 @@ func TestGlobPathFinder(t *testing.T) { testCases := []testCaseT{ { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"[]"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"[]"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "failed to expand file path pattern []: syntax error in pattern", }, { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "no matching files", }, { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "no matching files", }, { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"foo/*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"foo/*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "no matching files", }, { files: map[string]string{"bar.yml": testRuleBody}, - finder: discovery.NewGlobFinder([]string{"foo/*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"foo/*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "no matching files", }, { files: map[string]string{"bar.yml": testRuleBody}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -75,7 +75,7 @@ func TestGlobPathFinder(t *testing.T) { }, { files: map[string]string{"foo/bar.yml": testRuleBody + "\n\n# pint file/owner alice\n"}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -91,7 +91,7 @@ func TestGlobPathFinder(t *testing.T) { }, { files: map[string]string{"bar.yml": testRuleBody}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -110,7 +110,7 @@ func TestGlobPathFinder(t *testing.T) { }, { files: map[string]string{"bar.yml": "record:::{}\n expr: sum(foo)\n\n# pint file/owner bob\n"}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -130,7 +130,7 @@ func TestGlobPathFinder(t *testing.T) { { files: map[string]string{"bar.yml": testRuleBody}, symlinks: map[string]string{"link.yml": "bar.yml"}, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -166,7 +166,7 @@ func TestGlobPathFinder(t *testing.T) { "b/link.yml": "../a/bar.yml", "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -215,7 +215,7 @@ func TestGlobPathFinder(t *testing.T) { "b/link.yml": "../a/bar.yml", "b/c/link.yml": "../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "b/c/link.yml is a symlink but target file cannot be evaluated: lstat b/a: no such file or directory", }, { @@ -223,14 +223,14 @@ func TestGlobPathFinder(t *testing.T) { symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema, nil), }, { files: map[string]string{"a/bar.yml": "xxx:\nyyy:\n"}, symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -265,21 +265,21 @@ func TestGlobPathFinder(t *testing.T) { symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema, nil), }, { files: map[string]string{"a/bar.yml": "xxx:\nyyy:\n"}, symlinks: map[string]string{ "b/c/d": "../../a", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")}), parser.PrometheusSchema, nil), }, { files: map[string]string{"a/bar.yml": testRuleBody}, symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), entries: []discovery.Entry{ { State: discovery.Noop, @@ -313,7 +313,7 @@ func TestGlobPathFinder(t *testing.T) { symlinks: map[string]string{ "input.yml": "/xx/ccc/fdd", }, - finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil), parser.PrometheusSchema, nil), err: "input.yml is a symlink but target file cannot be evaluated: lstat /xx: no such file or directory", }, } diff --git a/internal/parser/read.go b/internal/parser/read.go index 13c70986..7d85f1c2 100644 --- a/internal/parser/read.go +++ b/internal/parser/read.go @@ -43,13 +43,14 @@ func emptyLine(line string, comments []comments.Comment, stripComments bool) str } type Content struct { - Body []byte - TotalLines int - IgnoreLine int - Ignored bool + Body []byte + FileComments []comments.Comment + TotalLines int + IgnoreLine int + Ignored bool } -func ReadContent(r io.Reader) (out Content, fileComments []comments.Comment, err error) { +func ReadContent(r io.Reader) (out Content, err error) { reader := bufio.NewReader(r) var ( lineno int @@ -100,21 +101,21 @@ func ReadContent(r io.Reader) (out Content, fileComments []comments.Comment, err skip = skipNextLine found = true case comments.FileOwnerType: - fileComments = append(fileComments, comment) + out.FileComments = append(out.FileComments, comment) case comments.RuleOwnerType: // pass case comments.FileDisableType: - fileComments = append(fileComments, comment) + out.FileComments = append(out.FileComments, comment) case comments.DisableType: // pass case comments.FileSnoozeType: - fileComments = append(fileComments, comment) + out.FileComments = append(out.FileComments, comment) case comments.SnoozeType: // pass case comments.RuleSetType: // pass case comments.InvalidComment: - fileComments = append(fileComments, comment) + out.FileComments = append(out.FileComments, comment) } } switch { @@ -166,8 +167,8 @@ func ReadContent(r io.Reader) (out Content, fileComments []comments.Comment, err } if !errors.Is(err, io.EOF) { - return out, fileComments, err + return out, err } - return out, fileComments, nil + return out, nil } diff --git a/internal/parser/read_test.go b/internal/parser/read_test.go index 7cc070c8..e49d8529 100644 --- a/internal/parser/read_test.go +++ b/internal/parser/read_test.go @@ -151,7 +151,7 @@ func TestReadContent(t *testing.T) { comments: []comments.Comment{ { Type: comments.FileOwnerType, - Value: comments.Owner{Name: "bob"}, + Value: comments.Owner{Name: "bob", Line: 1}, }, }, }, @@ -179,7 +179,7 @@ func TestReadContent(t *testing.T) { for i, tc := range testCases { t.Run(strconv.Itoa(i+1), func(t *testing.T) { r := bytes.NewReader(tc.input) - output, comments, err := parser.ReadContent(r) + output, err := parser.ReadContent(r) hadError := err != nil if hadError != tc.shouldError { @@ -194,7 +194,7 @@ func TestReadContent(t *testing.T) { return } - if diff := cmp.Diff(tc.comments, comments, sameErrorText); diff != "" { + if diff := cmp.Diff(tc.comments, output.FileComments, sameErrorText); diff != "" { t.Errorf("ReadContent() returned wrong comments (-want +got):\n%s", diff) return }