diff --git a/.github/spellcheck/wordlist.txt b/.github/spellcheck/wordlist.txt index 4cd48807..2769522e 100644 --- a/.github/spellcheck/wordlist.txt +++ b/.github/spellcheck/wordlist.txt @@ -2,6 +2,7 @@ APIs automaxprocs BitBucket bool +ci changelog Changelog CLI @@ -17,6 +18,7 @@ github GOGC golang GOMAXPROCS +hcl HCL hoc hostname @@ -50,5 +52,6 @@ uptime URI URIs validator +VCS yaml YAML diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6273df66..2afb2828 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,6 +34,6 @@ jobs: run: make build - name: Run pint ci - run: ./pint -c .github/pint/pint.hcl ci + run: ./pint -l debug -c .github/pint/pint.hcl ci env: GITHUB_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go index 8e3728a2..1f7f45cc 100644 --- a/cmd/pint/ci.go +++ b/cmd/pint/ci.go @@ -88,9 +88,17 @@ func actionCI(c *cli.Context) error { return nil } + slog.Info("Finding all rules to check on current git branch", slog.String("base", baseBranch)) + var entries []discovery.Entry - finder := discovery.NewGitBranchFinder(git.RunGit, includeRe, excludeRe, baseBranch, meta.cfg.CI.MaxCommits, meta.cfg.Parser.CompileRelaxed()) - entries, err = finder.Find() + filter := git.NewPathFilter(includeRe, excludeRe, meta.cfg.Parser.CompileRelaxed()) + + entries, err = discovery.NewGlobFinder([]string{"*"}, filter).Find() + if err != nil { + return err + } + + entries, err = discovery.NewGitBranchFinder(git.RunGit, filter, baseBranch, meta.cfg.CI.MaxCommits).Find(entries) if err != nil { return err } diff --git a/cmd/pint/lint.go b/cmd/pint/lint.go index 94691980..eec2952e 100644 --- a/cmd/pint/lint.go +++ b/cmd/pint/lint.go @@ -10,6 +10,7 @@ import ( "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/config" "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/git" "github.com/cloudflare/pint/internal/reporter" "github.com/urfave/cli/v2" @@ -60,7 +61,8 @@ func actionLint(c *cli.Context) error { return fmt.Errorf("at least one file or directory required") } - finder := discovery.NewGlobFinder(paths, meta.cfg.Parser.CompileRelaxed()) + slog.Info("Finding all rules to check", slog.Any("paths", paths)) + finder := discovery.NewGlobFinder(paths, git.NewPathFilter(nil, nil, meta.cfg.Parser.CompileRelaxed())) entries, err := finder.Find() if err != nil { return err diff --git a/cmd/pint/scan.go b/cmd/pint/scan.go index 413a47e1..0b52a371 100644 --- a/cmd/pint/scan.go +++ b/cmd/pint/scan.go @@ -98,7 +98,7 @@ func checkRules(ctx context.Context, workers int, gen *config.PrometheusGenerato wg.Wait() }() - var onlineChecksCount, offlineChecksCount atomic.Int64 + var onlineChecksCount, offlineChecksCount, checkedEntriesCount atomic.Int64 go func() { for _, entry := range entries { switch { @@ -126,6 +126,7 @@ func checkRules(ctx context.Context, workers int, gen *config.PrometheusGenerato ) } + checkedEntriesCount.Inc() checkList := cfg.GetChecksForRule(ctx, gen, entry, entry.DisabledChecks) for _, check := range checkList { checkIterationChecks.Inc() @@ -156,7 +157,8 @@ func checkRules(ctx context.Context, workers int, gen *config.PrometheusGenerato } summary.SortReports() summary.Duration = time.Since(start) - summary.Entries = len(entries) + summary.TotalEntries = len(entries) + summary.CheckedEntries = checkedEntriesCount.Load() summary.OnlineChecks = onlineChecksCount.Load() summary.OfflineChecks = offlineChecksCount.Load() diff --git a/cmd/pint/tests/0018_match_alerting.txt b/cmd/pint/tests/0018_match_alerting.txt index a77d4db8..8b0fff03 100644 --- a/cmd/pint/tests/0018_match_alerting.txt +++ b/cmd/pint/tests/0018_match_alerting.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=colo:recording lines=1-2 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:recording diff --git a/cmd/pint/tests/0019_match_recording.txt b/cmd/pint/tests/0019_match_recording.txt index 1a85d705..f533671d 100644 --- a/cmd/pint/tests/0019_match_recording.txt +++ b/cmd/pint/tests/0019_match_recording.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=colo:recording lines=1-2 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording diff --git a/cmd/pint/tests/0020_ignore_kind.txt b/cmd/pint/tests/0020_ignore_kind.txt index 257dd2f1..48f040c8 100644 --- a/cmd/pint/tests/0020_ignore_kind.txt +++ b/cmd/pint/tests/0020_ignore_kind.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=colo:recording lines=4-5 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording diff --git a/cmd/pint/tests/0028_ci_git_error.txt b/cmd/pint/tests/0028_ci_git_error.txt index ea384c9a..2249560b 100644 --- a/cmd/pint/tests/0028_ci_git_error.txt +++ b/cmd/pint/tests/0028_ci_git_error.txt @@ -24,6 +24,10 @@ level=INFO msg="Loading configuration file" path=.pint.hcl level=DEBUG msg="Running git command" args=["rev-parse","--abbrev-ref","HEAD"] level=DEBUG msg="Got branch information" base=notmain current=v2 level=INFO msg="Finding all rules to check on current git branch" base=notmain +level=DEBUG msg="Excluding git directory from glob results" path=.git glob=* +level=DEBUG msg="File parsed" path=.pint.hcl rules=0 +level=DEBUG msg="File parsed" path=rules.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Running git command" args=["log","--format=%H","--no-abbrev-commit","--reverse","notmain..HEAD"] level=ERROR msg="Fatal error" err="failed to get the list of commits to scan: fatal: ambiguous argument 'notmain..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n" -- src/v1.yml -- diff --git a/cmd/pint/tests/0032_ci_github.txt b/cmd/pint/tests/0032_ci_github.txt index 0852380f..b57e05c8 100644 --- a/cmd/pint/tests/0032_ci_github.txt +++ b/cmd/pint/tests/0032_ci_github.txt @@ -52,7 +52,7 @@ parser { repository { github { baseuri = "http://127.0.0.1:6032" - uploaduri = "http://127.0.0.1:6032" + uploaduri = "http://127.0.0.1:6032" owner = "cloudflare" repo = "pint" } diff --git a/cmd/pint/tests/0037_disable_checks.txt b/cmd/pint/tests/0037_disable_checks.txt index 3695c301..220c8cc4 100644 --- a/cmd/pint/tests/0037_disable_checks.txt +++ b/cmd/pint/tests/0037_disable_checks.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=3 +level=DEBUG msg="Glob finder completed" count=3 level=INFO msg="Configured new Prometheus server" name=prom uris=1 uptime=up tags=[] include=[] exclude=[] level=DEBUG msg="Starting query workers" name=prom uri=http://127.0.0.1 workers=16 level=DEBUG msg="Generated all Prometheus servers" count=1 diff --git a/cmd/pint/tests/0039_prom_selected_path.txt b/cmd/pint/tests/0039_prom_selected_path.txt index 9e0868ca..be5ea151 100644 --- a/cmd/pint/tests/0039_prom_selected_path.txt +++ b/cmd/pint/tests/0039_prom_selected_path.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=3 +level=DEBUG msg="Glob finder completed" count=3 level=INFO msg="Configured new Prometheus server" name=disabled uris=1 uptime=up tags=[] include=["^invalid/.+$"] exclude=["^invalid/rules/.+$"] level=DEBUG msg="Starting query workers" name=disabled uri=http://127.0.0.1:123 workers=16 level=DEBUG msg="Generated all Prometheus servers" count=1 diff --git a/cmd/pint/tests/0040_rule_match_label.txt b/cmd/pint/tests/0040_rule_match_label.txt index a555aead..77d8d2f7 100644 --- a/cmd/pint/tests/0040_rule_match_label.txt +++ b/cmd/pint/tests/0040_rule_match_label.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/rules.yml rules=4 +level=DEBUG msg="Glob finder completed" count=4 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/rules.yml record=ignore lines=1-2 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/rules.yml rule=ignore diff --git a/cmd/pint/tests/0052_match_multiple.txt b/cmd/pint/tests/0052_match_multiple.txt index 30679b20..99b39a95 100644 --- a/cmd/pint/tests/0052_match_multiple.txt +++ b/cmd/pint/tests/0052_match_multiple.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=colo:recording lines=4-5 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording diff --git a/cmd/pint/tests/0053_ignore_multiple.txt b/cmd/pint/tests/0053_ignore_multiple.txt index 9ac839fa..738e4cd0 100644 --- a/cmd/pint/tests/0053_ignore_multiple.txt +++ b/cmd/pint/tests/0053_ignore_multiple.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=colo:recording lines=4-5 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=colo:recording diff --git a/cmd/pint/tests/0092_dir_symlink.txt b/cmd/pint/tests/0092_dir_symlink.txt index 16221d20..bd921c72 100644 --- a/cmd/pint/tests/0092_dir_symlink.txt +++ b/cmd/pint/tests/0092_dir_symlink.txt @@ -10,6 +10,7 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Finding all rules to check" paths=["rules","linked","rules/src/rule.yaml"] level=DEBUG msg="File parsed" path=rules/src/rule.yaml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/src/rule.yaml record=down lines=4-5 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/src/rule.yaml rule=down diff --git a/cmd/pint/tests/0095_rulefmt_symlink.txt b/cmd/pint/tests/0095_rulefmt_symlink.txt index f9e88557..4c20a525 100644 --- a/cmd/pint/tests/0095_rulefmt_symlink.txt +++ b/cmd/pint/tests/0095_rulefmt_symlink.txt @@ -10,6 +10,7 @@ level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/relaxed/1.yml rules=1 level=DEBUG msg="File parsed" path=rules/strict/symlink.yml rules=1 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/relaxed/1.yml record=foo lines=1-2 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/relaxed/1.yml rule=foo diff --git a/cmd/pint/tests/0096_bad_symlink.txt b/cmd/pint/tests/0096_bad_symlink.txt index 1a153108..b836a354 100644 --- a/cmd/pint/tests/0096_bad_symlink.txt +++ b/cmd/pint/tests/0096_bad_symlink.txt @@ -7,4 +7,4 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Finding all rules to check" paths=["rules"] -level=ERROR msg="Fatal error" err="lstat rules/../bad.yml: no such file or directory" +level=ERROR msg="Fatal error" err="rules/symlink.yml is a symlink but target file cannot be evaluated: lstat rules/../bad.yml: no such file or directory" diff --git a/cmd/pint/tests/0099_symlink_outside_glob.txt b/cmd/pint/tests/0099_symlink_outside_glob.txt index 8957806f..f58ee7b4 100644 --- a/cmd/pint/tests/0099_symlink_outside_glob.txt +++ b/cmd/pint/tests/0099_symlink_outside_glob.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules/relaxed"] level=DEBUG msg="File parsed" path=rules/relaxed/1.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/relaxed/1.yml record=foo lines=1-2 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/relaxed/1.yml rule=foo diff --git a/cmd/pint/tests/0103_file_disable.txt b/cmd/pint/tests/0103_file_disable.txt index cd2b7878..b76b9da8 100644 --- a/cmd/pint/tests/0103_file_disable.txt +++ b/cmd/pint/tests/0103_file_disable.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=INFO msg="Configured new Prometheus server" name=prom uris=1 uptime=up tags=[] include=[] exclude=[] level=DEBUG msg="Starting query workers" name=prom uri=http://127.0.0.1:7103 workers=16 level=DEBUG msg="Generated all Prometheus servers" count=1 diff --git a/cmd/pint/tests/0111_snooze.txt b/cmd/pint/tests/0111_snooze.txt index 7d540945..6a5d6e2f 100644 --- a/cmd/pint/tests/0111_snooze.txt +++ b/cmd/pint/tests/0111_snooze.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=2-3 level=DEBUG msg="Check snoozed by comment" check=promql/aggregate(job:true) match=promql/aggregate until="2099-11-28T10:24:18Z" diff --git a/cmd/pint/tests/0112_expired_snooze.txt b/cmd/pint/tests/0112_expired_snooze.txt index 2e37b927..c02a7f86 100644 --- a/cmd/pint/tests/0112_expired_snooze.txt +++ b/cmd/pint/tests/0112_expired_snooze.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=2-3 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=sum-job diff --git a/cmd/pint/tests/0115_file_disable_tag.txt b/cmd/pint/tests/0115_file_disable_tag.txt index ffbc3d35..3b9916c7 100644 --- a/cmd/pint/tests/0115_file_disable_tag.txt +++ b/cmd/pint/tests/0115_file_disable_tag.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=INFO msg="Configured new Prometheus server" name=prom uris=1 uptime=up tags=["foo","bar"] include=[] exclude=[] level=DEBUG msg="Starting query workers" name=prom uri=http://127.0.0.1:7103 workers=16 level=DEBUG msg="Generated all Prometheus servers" count=1 diff --git a/cmd/pint/tests/0116_file_snooze.txt b/cmd/pint/tests/0116_file_snooze.txt index 99d7fd5f..bada200b 100644 --- a/cmd/pint/tests/0116_file_snooze.txt +++ b/cmd/pint/tests/0116_file_snooze.txt @@ -8,6 +8,7 @@ level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="Check snoozed by comment" check=promql/aggregate(job:true) match=promql/aggregate(job:true) until="2099-11-28T10:24:18Z" level=DEBUG msg="Check snoozed by comment" check=alerts/for match=alerts/for until="2099-11-28T10:24:18Z" level=DEBUG msg="File parsed" path=rules/0001.yml rules=2 +level=DEBUG msg="Glob finder completed" count=2 level=DEBUG msg="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=4-5 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=sum-job diff --git a/cmd/pint/tests/0134_ci_base_branch_flag_path.txt b/cmd/pint/tests/0134_ci_base_branch_flag_path.txt index c91733b0..72274821 100644 --- a/cmd/pint/tests/0134_ci_base_branch_flag_path.txt +++ b/cmd/pint/tests/0134_ci_base_branch_flag_path.txt @@ -24,6 +24,10 @@ level=INFO msg="Loading configuration file" path=.pint.hcl level=DEBUG msg="Running git command" args=["rev-parse","--abbrev-ref","HEAD"] level=DEBUG msg="Got branch information" base=origin/main current=v2 level=INFO msg="Finding all rules to check on current git branch" base=origin/main +level=DEBUG msg="Excluding git directory from glob results" path=.git glob=* +level=DEBUG msg="File parsed" path=.pint.hcl rules=0 +level=DEBUG msg="File parsed" path=rules.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Running git command" args=["log","--format=%H","--no-abbrev-commit","--reverse","origin/main..HEAD"] level=ERROR msg="Fatal error" err="failed to get the list of commits to scan: fatal: ambiguous argument 'origin/main..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n" -- src/v1.yml -- diff --git a/cmd/pint/tests/0135_ci_base_branch_config_path.txt b/cmd/pint/tests/0135_ci_base_branch_config_path.txt index a546f801..a22102f5 100644 --- a/cmd/pint/tests/0135_ci_base_branch_config_path.txt +++ b/cmd/pint/tests/0135_ci_base_branch_config_path.txt @@ -24,6 +24,10 @@ level=INFO msg="Loading configuration file" path=.pint.hcl level=DEBUG msg="Running git command" args=["rev-parse","--abbrev-ref","HEAD"] level=DEBUG msg="Got branch information" base=origin/main current=v2 level=INFO msg="Finding all rules to check on current git branch" base=origin/main +level=DEBUG msg="Excluding git directory from glob results" path=.git glob=* +level=DEBUG msg="File parsed" path=.pint.hcl rules=0 +level=DEBUG msg="File parsed" path=rules.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Running git command" args=["log","--format=%H","--no-abbrev-commit","--reverse","origin/main..HEAD"] level=ERROR msg="Fatal error" err="failed to get the list of commits to scan: fatal: ambiguous argument 'origin/main..HEAD': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n" -- src/v1.yml -- diff --git a/cmd/pint/tests/0144_discovery_filepath.txt b/cmd/pint/tests/0144_discovery_filepath.txt index 3d9bfa9c..e8e9de78 100644 --- a/cmd/pint/tests/0144_discovery_filepath.txt +++ b/cmd/pint/tests/0144_discovery_filepath.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=INFO msg="Finding Prometheus servers using file paths" dir=servers match=^(?P\w+).ya?ml$ level=DEBUG msg="Path discovery match" match=^(?P\w+).ya?ml$ path=prom1.yaml level=DEBUG msg="Extracted regexp variables" regexp=^(?P\w+).ya?ml$ vars={"name":"prom1"} diff --git a/cmd/pint/tests/0145_discovery_filepath_dup.txt b/cmd/pint/tests/0145_discovery_filepath_dup.txt index 0065c16a..6cea1829 100644 --- a/cmd/pint/tests/0145_discovery_filepath_dup.txt +++ b/cmd/pint/tests/0145_discovery_filepath_dup.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=INFO msg="Configured new Prometheus server" name=prom2 uris=1 uptime=up tags=[] include=[] exclude=[] level=DEBUG msg="Starting query workers" name=prom2 uri=https://unique.example.com workers=16 level=INFO msg="Finding Prometheus servers using file paths" dir=servers match=^(?P\w+).ya?ml$ diff --git a/cmd/pint/tests/0147_discovery_filepath_error.txt b/cmd/pint/tests/0147_discovery_filepath_error.txt index 62fbfbd4..a6cecc65 100644 --- a/cmd/pint/tests/0147_discovery_filepath_error.txt +++ b/cmd/pint/tests/0147_discovery_filepath_error.txt @@ -6,6 +6,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=INFO msg="Finding Prometheus servers using file paths" dir=notfound match=^(?P\w+).ya?ml$ level=ERROR msg="Fatal error" err="filepath discovery error: lstat notfound: no such file or directory" -- rules/0001.yml -- diff --git a/cmd/pint/tests/0148_discovery_prom_zero.txt b/cmd/pint/tests/0148_discovery_prom_zero.txt index a22ac281..8cd2fc8e 100644 --- a/cmd/pint/tests/0148_discovery_prom_zero.txt +++ b/cmd/pint/tests/0148_discovery_prom_zero.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7148 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7148 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7148 query=prometheus_ready diff --git a/cmd/pint/tests/0149_discovery_prom.txt b/cmd/pint/tests/0149_discovery_prom.txt index e70e419d..82704e49 100644 --- a/cmd/pint/tests/0149_discovery_prom.txt +++ b/cmd/pint/tests/0149_discovery_prom.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7149 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7149 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7149 query=prometheus_ready diff --git a/cmd/pint/tests/0150_discovery_prom_dup_tags.txt b/cmd/pint/tests/0150_discovery_prom_dup_tags.txt index 008719c0..29adeaf6 100644 --- a/cmd/pint/tests/0150_discovery_prom_dup_tags.txt +++ b/cmd/pint/tests/0150_discovery_prom_dup_tags.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7150 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7150 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7150 query=prometheus_ready diff --git a/cmd/pint/tests/0151_discovery_prom_error.txt b/cmd/pint/tests/0151_discovery_prom_error.txt index 1513d5e6..a3520891 100644 --- a/cmd/pint/tests/0151_discovery_prom_error.txt +++ b/cmd/pint/tests/0151_discovery_prom_error.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7151 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7151 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7151 query=prometheus_ready diff --git a/cmd/pint/tests/0152_discovery_prom_dup_uptime.txt b/cmd/pint/tests/0152_discovery_prom_dup_uptime.txt index 9a0b71fc..1e27c4ec 100644 --- a/cmd/pint/tests/0152_discovery_prom_dup_uptime.txt +++ b/cmd/pint/tests/0152_discovery_prom_dup_uptime.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7152 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7152 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7152 query=prometheus_ready diff --git a/cmd/pint/tests/0155_discovery_prom_dup_include.txt b/cmd/pint/tests/0155_discovery_prom_dup_include.txt index b28863e4..e98c3b8d 100644 --- a/cmd/pint/tests/0155_discovery_prom_dup_include.txt +++ b/cmd/pint/tests/0155_discovery_prom_dup_include.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7155 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7155 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7155 query=prometheus_ready diff --git a/cmd/pint/tests/0156_discovery_prom_dup_exclude.txt b/cmd/pint/tests/0156_discovery_prom_dup_exclude.txt index ab8feb02..42b87229 100644 --- a/cmd/pint/tests/0156_discovery_prom_dup_exclude.txt +++ b/cmd/pint/tests/0156_discovery_prom_dup_exclude.txt @@ -9,6 +9,7 @@ cmp stderr stderr.txt level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=1 +level=DEBUG msg="Glob finder completed" count=1 level=DEBUG msg="Starting query workers" name=discovery uri=http://127.0.0.1:7156 workers=1 level=INFO msg="Finding Prometheus servers using Prometheus API query" uri=http://127.0.0.1:7156 query=prometheus_ready level=DEBUG msg="Scheduling prometheus query" uri=http://127.0.0.1:7156 query=prometheus_ready diff --git a/cmd/pint/tests/0162_ci_deleted_dependency.txt b/cmd/pint/tests/0162_ci_deleted_dependency.txt index fb8e4658..277216d6 100644 --- a/cmd/pint/tests/0162_ci_deleted_dependency.txt +++ b/cmd/pint/tests/0162_ci_deleted_dependency.txt @@ -2,6 +2,8 @@ mkdir testrepo cd testrepo exec git init --initial-branch=main . +cp ../src/alert.yml alert.yml +exec ln -s alert.yml symlink.yml cp ../src/v1.yml rules.yml cp ../src/.pint.hcl . env GIT_AUTHOR_NAME=pint @@ -20,10 +22,9 @@ pint.ok -l error --offline --no-color ci cmp stderr ../stderr.txt -- stderr.txt -- -rules.yml:5 Warning: This rule uses a metric produced by recording rule `up:sum` which was removed from rules.yml. (rule/dependency) - 5 | expr: 'up:sum == 0' +rules.yml:4-5 (deleted) Warning: Metric generated by this rule is used by 1 other rule(s). (rule/dependency) --- src/v1.yml -- +-- src/alert.yml -- groups: - name: g1 rules: @@ -33,18 +34,16 @@ groups: summary: 'Service is down' labels: cluster: dev +-- src/v1.yml -- +groups: +- name: g1 + rules: - record: up:sum expr: sum(up) -- src/v2.yml -- groups: - name: g1 - rules: - - alert: Alert - expr: 'up:sum == 0' - annotations: - summary: 'Service is down' - labels: - cluster: dev + rules: [] -- src/.pint.hcl -- ci { baseBranch = "main" diff --git a/cmd/pint/watch.go b/cmd/pint/watch.go index d71cf3c2..62a9b7e4 100644 --- a/cmd/pint/watch.go +++ b/cmd/pint/watch.go @@ -18,6 +18,7 @@ import ( "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/config" "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/git" "github.com/cloudflare/pint/internal/promapi" "github.com/cloudflare/pint/internal/reporter" @@ -255,9 +256,9 @@ func newProblemCollector(cfg config.Config, paths []string, minSeverity checks.S } func (c *problemCollector) scan(ctx context.Context, workers int, gen *config.PrometheusGenerator) error { - finder := discovery.NewGlobFinder(c.paths, c.cfg.Parser.CompileRelaxed()) + slog.Info("Finding all rules to check", slog.Any("paths", c.paths)) // nolint: contextcheck - entries, err := finder.Find() + entries, err := discovery.NewGlobFinder(c.paths, git.NewPathFilter(nil, nil, c.cfg.Parser.CompileRelaxed())).Find() if err != nil { return err } diff --git a/docs/changelog.md b/docs/changelog.md index b5b45553..076ce51f 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -6,6 +6,20 @@ - Added [rule/dependency](checks/rule/dependency.md) check. +### Changed + +- When running `pint ci` pint will now first try to parse all files in current + working directory, before checking for files modified on current branch. + This is to have a full list of all rules, which is needed for checks like + newly added [rule/dependency](checks/rule/dependency.md). + This can slow `pint` runs if there's a lot of files in your repository. + If there are non-rule files these may fail to parse and result in check errors. + To avoid any errors or slowdowns from scanning unrelated files you might need + to add `ci` section to `.pint.hcl` with `include` and/or `exclude` options set. + See [examples/ci.hcl](examples/ci.hcl) for an example config. +- `Bug` and `Fatal` severity problems are now reported as tasks when using + BitBucket. + ## v0.50.1 ### Fixed diff --git a/docs/checks/rule/dependency.md b/docs/checks/rule/dependency.md index a30070a6..80adf341 100644 --- a/docs/checks/rule/dependency.md +++ b/docs/checks/rule/dependency.md @@ -48,29 +48,7 @@ This check doesn't have any configuration options. ## How to enable it -This check is enabled by default for all configured Prometheus servers. - -Example: - -```js -prometheus "prod" { - uri = "https://prometheus-prod.example.com" - timeout = "60s" - include = [ - "rules/prod/.*", - "rules/common/.*", - ] -} - -prometheus "dev" { - uri = "https://prometheus-dev.example.com" - timeout = "30s" - include = [ - "rules/dev/.*", - "rules/common/.*", - ] -} -``` +This check is enabled by default. ## How to disable it @@ -95,21 +73,6 @@ Or you can disable it per rule by adding a comment to it. Example: # pint disable rule/dependency ``` -If you want to disable only individual instances of this check -you can add a more specific comment. - -```yaml -# pint disable rule/dependency($prometheus) -``` - -Where `$prometheus` is the name of Prometheus server to disable. - -Example: - -```yaml -# pint disable rule/dependency(prod) -``` - ## How to snooze it You can disable this check until given time by adding a comment to it. Example: diff --git a/docs/examples/ci.hcl b/docs/examples/ci.hcl new file mode 100644 index 00000000..e151c838 --- /dev/null +++ b/docs/examples/ci.hcl @@ -0,0 +1,17 @@ +# This is an example config to be used when running pint as a CI job +# to validate pull requests. + +ci { + # Check all files inside rules/alerting and rules/recording dirs. + include = ["rules/(alerting|recording)/.+"] + + # Ignore all *.md and *.txt files. + exclude = [".+.md", ".+.txt"] + + # Don't run pint if there are more than 50 commits on current branch. + maxCommits = 50 + + # When running 'pint ci' compare current branch with origin/main + # to get the list of modified files. + baseBranch = "origin/main" +} diff --git a/docs/index.md b/docs/index.md index 7abd87ef..dbd7bb5b 100644 --- a/docs/index.md +++ b/docs/index.md @@ -43,10 +43,17 @@ There are three modes it works in: ### Pull Requests -Run it with `pint ci`. +Run it with `pint ci`. Git is currently the only supported VCS. -It currently supports git for which it will find all commits on the current branch that are not -present in the parent branch and scan all modified files included in those changes. +When `pint ci` runs it will find all files in the current working directory and try to parse +them as Prometheus rules. Then it will look for all commits on the current branch that are not +present in the parent branch and to decide which rules were modified. +Checks are run only on modified rules but they require the full list of all rules to find any +cross-rule dependencies. + +Running `pint ci` doesn't require any configuration but it's recommended to add a pint config file +with `ci` section containing at least the `include` option. This will ensure that pint validates +only Prometheus rules and ignores other files. Results can optionally be reported using [BitBucket API](https://developer.atlassian.com/server/bitbucket/rest/) diff --git a/go.mod b/go.mod index 0f479fd6..f16a2379 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/fatih/color v1.16.0 github.com/gkampitakis/go-snaps v0.4.12 github.com/google/go-cmp v0.6.0 - github.com/google/go-github/v55 v55.0.0 + github.com/google/go-github/v57 v57.0.0 github.com/hashicorp/hcl/v2 v2.19.1 github.com/klauspost/compress v1.17.4 github.com/neilotoole/slogt v1.1.0 @@ -33,14 +33,12 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.4.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/internal v1.5.0 // indirect github.com/AzureAD/microsoft-authentication-library-for-go v1.2.0 // indirect - github.com/ProtonMail/go-crypto v0.0.0-20230923063757-afb1ddc0824c // indirect github.com/agext/levenshtein v1.2.3 // indirect github.com/alecthomas/units v0.0.0-20211218093645-b94a6e3cc137 // indirect github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect github.com/aws/aws-sdk-go v1.47.2 // indirect github.com/benbjohnson/clock v1.3.5 // indirect github.com/beorn7/perks v1.0.1 // indirect - github.com/cloudflare/circl v1.3.6 // indirect github.com/cpuguy83/go-md2man/v2 v2.0.3 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/dennwc/varint v1.0.0 // indirect diff --git a/go.sum b/go.sum index 29982cf8..0411a4e7 100644 --- a/go.sum +++ b/go.sum @@ -48,8 +48,6 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03 github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migciow= github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM= -github.com/ProtonMail/go-crypto v0.0.0-20230923063757-afb1ddc0824c h1:kMFnB0vCcX7IL/m9Y5LO+KQYv+t1CQOiFe6+SV2J7bE= -github.com/ProtonMail/go-crypto v0.0.0-20230923063757-afb1ddc0824c/go.mod h1:EjAoLdwvbIOoOQr3ihjnSoLZRtE8azugULFRteWMNc0= github.com/agext/levenshtein v1.2.3 h1:YB2fHEn0UJagG8T1rrWknE3ZQzWM06O8AMAatNn7lmo= github.com/agext/levenshtein v1.2.3/go.mod h1:JEDfjyjHDjOF/1e4FlBE/PkbqA9OfWu2ki2W0IB5558= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= @@ -72,7 +70,6 @@ github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24 github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/bwesterb/go-ristretto v1.2.3/go.mod h1:fUIoIZaG73pV5biE2Blr2xEzDoMj7NFEuV9ekS419A0= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= @@ -81,9 +78,6 @@ github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWR github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI= github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= -github.com/cloudflare/circl v1.3.3/go.mod h1:5XYMA4rFBvNIrhs50XuiBJ15vF2pZn4nnUKZrLbUZFA= -github.com/cloudflare/circl v1.3.6 h1:/xbKIqSHbZXHwkhbrhrt2YOHIwYJlXH94E3tI/gDlUg= -github.com/cloudflare/circl v1.3.6/go.mod h1:5XYMA4rFBvNIrhs50XuiBJ15vF2pZn4nnUKZrLbUZFA= github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= github.com/cncf/xds/go v0.0.0-20230607035331-e9ce68804cb4 h1:/inchEIKaYC1Akx+H+gqO04wryn5h75LSazbRlnya1k= github.com/cncf/xds/go v0.0.0-20230607035331-e9ce68804cb4/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs= @@ -213,8 +207,8 @@ github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -github.com/google/go-github/v55 v55.0.0 h1:4pp/1tNMB9X/LuAhs5i0KQAE40NmiR/y6prLNb9x9cg= -github.com/google/go-github/v55 v55.0.0/go.mod h1:JLahOTA1DnXzhxEymmFF5PP2tSS9JVNj68mSZNDwskA= +github.com/google/go-github/v57 v57.0.0 h1:L+Y3UPTY8ALM8x+TV0lg+IEBI+upibemtBD8Q9u7zHs= +github.com/google/go-github/v57 v57.0.0/go.mod h1:s0omdnye0hvK/ecLvpsGfJMiRt85PimQh4oygmLIxHw= github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= @@ -470,8 +464,6 @@ golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -golang.org/x/crypto v0.3.1-0.20221117191849-2c476679df9a/go.mod h1:hebNnKkNXi2UzZN1eVRvBB7co0a+JxK6XbPiWVs/3J4= -golang.org/x/crypto v0.7.0/go.mod h1:pYwdfH91IfpZVANVyUOhSIPZaFoJGxTFbZhFTx+dXZU= golang.org/x/crypto v0.16.0 h1:mMMrFzRSCF0GvB7Ne27XVtVAaXLrPmgPC7/v0tkwHaY= golang.org/x/crypto v0.16.0/go.mod h1:gCAAfMLgwOJRpTjQ2zCCt2OcSfYMTeZVSRtQlPC7Nq4= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= @@ -507,7 +499,6 @@ golang.org/x/mod v0.1.1-0.20191107180719-034126e5016b/go.mod h1:QqPTAvyqsEbceGzB golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= -golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.13.0 h1:I/DsJXRlw/8l/0c24sM9yb0T4z9liZTduXvdAWYiysY= golang.org/x/mod v0.13.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -543,9 +534,6 @@ golang.org/x/net v0.0.0-20201110031124-69a78807bb2b/go.mod h1:sp8m0HH+o8qH0wwXwY golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210525063256-abc453219eb5/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= -golang.org/x/net v0.2.0/go.mod h1:KqCZLdyyvdV855qA2rE3GC2aiw5xGR5TEjj8smXukLY= -golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= -golang.org/x/net v0.8.0/go.mod h1:QVkue5JL9kW//ek3r6jTKnTFis1tRmNAW2P1shuFdJc= golang.org/x/net v0.19.0 h1:zTwKpTd2XuCqf8huc7Fo2iSy+4RHPd10s4KzeTnVr1c= golang.org/x/net v0.19.0/go.mod h1:CfAk/cbD4CthTvqiEl8NpboMuiuOYsAr/7NOjZJtv1U= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= @@ -567,7 +555,6 @@ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ= golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -611,17 +598,11 @@ golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.3.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.15.0 h1:h48lPFYpsTvQJZF4EKyI4aLHaev3CxivZmv7yZig9pc= golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= -golang.org/x/term v0.2.0/go.mod h1:TVmDHMZPmdnySmBfhjOoOdhjzdE1h4u1VwSiw2l1Nuc= -golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= -golang.org/x/term v0.6.0/go.mod h1:m6U89DPEgQRMq3DNkDClhWw02AUbt2daBVO4cn4Hv9U= golang.org/x/term v0.15.0 h1:y/Oo/a/q3IXu26lQgl04j/gjuBDOBlx7X6Om1j2CPW4= golang.org/x/term v0.15.0/go.mod h1:BDl952bC7+uMoWR75FIrCDx79TPU9oHkTZ9yRbYOrX0= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -632,9 +613,6 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= golang.org/x/text v0.3.8/go.mod h1:E6s5w1FMmriuDzIBO73fBruAKo1PCIq6d2Q6DHfQ8WQ= -golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= -golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= -golang.org/x/text v0.8.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= @@ -685,7 +663,6 @@ golang.org/x/tools v0.0.0-20200804011535-6c149bb5ef0d/go.mod h1:njjCfa9FT2d7l9Bc golang.org/x/tools v0.0.0-20200825202427-b303f430e36d/go.mod h1:njjCfa9FT2d7l9Bc6FUM5FLjQPp3cFF28FI3qnDFljA= golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= -golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.14.0 h1:jvNa2pY0M4r62jkRQ6RwEZZyPcymeL9XZMLBbV7U2nc= golang.org/x/tools v0.14.0/go.mod h1:uYBEerGOWcJyEORxN+Ek8+TT266gXkNlHdJBwexUsBg= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/checks/base.go b/internal/checks/base.go index 5e827592..fa0d6de5 100644 --- a/internal/checks/base.go +++ b/internal/checks/base.go @@ -96,8 +96,16 @@ const ( type SettingsKey string +type Anchor uint8 + +const ( + AnchorAfter Anchor = iota + AnchorBefore +) + type Problem struct { Fragment string + Anchor Anchor Lines []int Reporter string Text string diff --git a/internal/checks/rule_dependency.go b/internal/checks/rule_dependency.go index 0244a25e..9764c6e6 100644 --- a/internal/checks/rule_dependency.go +++ b/internal/checks/rule_dependency.go @@ -3,24 +3,24 @@ package checks import ( "context" "fmt" + "sort" + "strconv" + "strings" "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/parser/utils" - "github.com/cloudflare/pint/internal/promapi" ) const ( RuleDependencyCheckName = "rule/dependency" ) -func NewRuleDependencyCheck(prom *promapi.FailoverGroup) RuleDependencyCheck { - return RuleDependencyCheck{prom: prom} +func NewRuleDependencyCheck() RuleDependencyCheck { + return RuleDependencyCheck{} } -type RuleDependencyCheck struct { - prom *promapi.FailoverGroup -} +type RuleDependencyCheck struct{} func (c RuleDependencyCheck) Meta() CheckMeta { return CheckMeta{ @@ -32,7 +32,7 @@ func (c RuleDependencyCheck) Meta() CheckMeta { } func (c RuleDependencyCheck) String() string { - return fmt.Sprintf("%s(%s)", RuleDependencyCheckName, c.prom.Name()) + return RuleDependencyCheckName } func (c RuleDependencyCheck) Reporter() string { @@ -44,34 +44,78 @@ func (c RuleDependencyCheck) Check(_ context.Context, path string, rule parser.R return problems } + var broken []brokenDependency for _, entry := range entries { if entry.State == discovery.Removed { continue } - if !c.prom.IsEnabledForPath(entry.SourcePath) { + if entry.PathError != nil { + continue + } + if entry.Rule.Error.Err != nil { continue } if c.usesVector(entry, rule.RecordingRule.Record.Value.Value) { expr := entry.Rule.Expr() - problems = append(problems, Problem{ - Fragment: fmt.Sprintf("%s: %s", expr.Key.Value, expr.Value.Value), - Lines: expr.Lines(), - Reporter: c.Reporter(), - Text: fmt.Sprintf( - "This rule uses a metric produced by %s rule `%s` which was removed from %s.", - rule.Type(), - rule.RecordingRule.Record.Value.Value, - path, - ), - Details: fmt.Sprintf( - "If you remove the recording rule generating `%s` and there is no other source of `%s` metric, then this and other rule depending on `%s` will break.", - rule.RecordingRule.Record.Value.Value, rule.RecordingRule.Record.Value.Value, rule.RecordingRule.Record.Value.Value, - ), - Severity: Warning, - }) + dep := brokenDependency{ + path: entry.ReportedPath, + line: expr.Lines()[0], + name: entry.Rule.Name(), + } + var found bool + for _, b := range broken { + if b.path == dep.path && b.line == dep.line && b.name == dep.name { + found = true + break + } + } + if !found { + broken = append(broken, dep) + } } } + if len(broken) == 0 { + return problems + } + + sort.Slice(broken, func(i, j int) bool { + if broken[i].path != broken[j].path { + return broken[i].path < broken[j].path + } + if broken[i].line != broken[j].line { + return broken[i].line < broken[j].line + } + return broken[i].name < broken[j].name + }) + + var details strings.Builder + details.WriteString("If you remove the recording rule generating `") + details.WriteString(rule.RecordingRule.Record.Value.Value) + details.WriteString("`, and there is no other source of this metric, then any other rule depending on it will break.\n") + details.WriteString("List of found rules that are using `") + details.WriteString(rule.RecordingRule.Record.Value.Value) + details.WriteString("`:\n\n") + for _, b := range broken { + details.WriteString("- `") + details.WriteString(b.name) + details.WriteString("` at `") + details.WriteString(b.path) + details.WriteRune(':') + details.WriteString(strconv.Itoa(b.line)) + details.WriteString("`\n") + } + + problems = append(problems, Problem{ + Fragment: fmt.Sprintf("%s: %s", rule.RecordingRule.Record.Key.Value, rule.RecordingRule.Record.Value.Value), + Anchor: AnchorBefore, + Lines: rule.Lines(), + Reporter: c.Reporter(), + Text: fmt.Sprintf("Metric generated by this rule is used by %d other rule(s).", len(broken)), + Details: details.String(), + Severity: Warning, + }) + return problems } @@ -89,3 +133,9 @@ func (c RuleDependencyCheck) usesVector(entry discovery.Entry, name string) bool return false } + +type brokenDependency struct { + path string + name string + line int +} diff --git a/internal/checks/rule_dependency_test.go b/internal/checks/rule_dependency_test.go index fedf090c..aefaeb2e 100644 --- a/internal/checks/rule_dependency_test.go +++ b/internal/checks/rule_dependency_test.go @@ -1,6 +1,7 @@ package checks_test import ( + "errors" "fmt" "regexp" "testing" @@ -11,21 +12,25 @@ import ( "github.com/cloudflare/pint/internal/promapi" ) -func textDependencyRule(path, name string) string { - return fmt.Sprintf("This rule uses a metric produced by recording rule `%s` which was removed from %s.", name, path) +func textDependencyRule(nr int) string { + return fmt.Sprintf("Metric generated by this rule is used by %d other rule(s).", nr) } -func detailsDependencyRule(name string) string { - return fmt.Sprintf("If you remove the recording rule generating `%s` and there is no other source of `%s` metric, then this and other rule depending on `%s` will break.", name, name, name) +func detailsDependencyRule(name, broken string) string { + return fmt.Sprintf( + "If you remove the recording rule generating `%s`, and there is no other source of this metric, then any other rule depending on it will break.\nList of found rules that are using `%s`:\n\n%s", + name, name, broken, + ) } func TestRuleDependencyCheck(t *testing.T) { - parseWithState := func(input string, state discovery.ChangeType, path string) []discovery.Entry { + parseWithState := func(input string, state discovery.ChangeType, sp, rp string) []discovery.Entry { entries := mustParseContent(input) for i := range entries { entries[i].State = state - entries[i].ReportedPath = path - entries[i].SourcePath = path + entries[i].SourcePath = sp + entries[i].ReportedPath = rp + } return entries } @@ -34,8 +39,8 @@ func TestRuleDependencyCheck(t *testing.T) { { description: "ignores alerting rules", content: "- alert: foo\n expr: up == 0\n", - checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewRuleDependencyCheck(prom) + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() }, prometheus: newSimpleProm, problems: noProblems, @@ -43,8 +48,8 @@ func TestRuleDependencyCheck(t *testing.T) { { description: "ignores rules with syntax errors", content: "- record: foo\n expr: sum(foo) without(\n", - checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewRuleDependencyCheck(prom) + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() }, prometheus: newSimpleProm, problems: noProblems, @@ -52,34 +57,34 @@ func TestRuleDependencyCheck(t *testing.T) { { description: "ignores alerts with expr errors", content: "- record: foo\n expr: sum(foo)\n", - checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewRuleDependencyCheck(prom) + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() }, prometheus: newSimpleProm, problems: noProblems, entries: []discovery.Entry{ - parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml")[0], - parseWithState("- alert: foo\n expr: foo ==\n", discovery.Noop, "foo.yaml")[0], + parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: foo\n expr: foo ==\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], }, }, { description: "ignores alerts without dependencies", content: "- record: foo\n expr: sum(foo)\n", - checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewRuleDependencyCheck(prom) + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() }, prometheus: newSimpleProm, problems: noProblems, entries: []discovery.Entry{ - parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml")[0], - parseWithState("- alert: foo\n expr: up == 0\n", discovery.Noop, "foo.yaml")[0], + parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: foo\n expr: up == 0\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], }, }, { - description: "ignores alerts on other prometheus servers", + description: "includes alerts on other prometheus servers", content: "- record: foo\n expr: sum(foo)\n", - checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewRuleDependencyCheck(prom) + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() }, prometheus: func(uri string) *promapi.FailoverGroup { return promapi.NewFailoverGroup( @@ -95,34 +100,109 @@ func TestRuleDependencyCheck(t *testing.T) { []string{}, ) }, - problems: noProblems, + problems: func(s string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "record: foo", + Anchor: checks.AnchorBefore, + Lines: []int{1, 2}, + Reporter: checks.RuleDependencyCheckName, + Text: textDependencyRule(1), + Details: detailsDependencyRule("foo", "- `alert` at `excluded.yaml:2`\n"), + Severity: checks.Warning, + }, + } + }, entries: []discovery.Entry{ - parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml")[0], - parseWithState("- alert: foo\n expr: foo == 0\n", discovery.Noop, "excluded.yaml")[0], + parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: alert\n expr: foo == 0\n", discovery.Noop, "excluded.yaml", "excluded.yaml")[0], }, }, { description: "warns about removed dependency", content: "- record: foo\n expr: sum(foo)\n", - checker: func(prom *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewRuleDependencyCheck(prom) + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() + }, + prometheus: newSimpleProm, + problems: func(s string) []checks.Problem { + return []checks.Problem{ + { + Fragment: "record: foo", + Anchor: checks.AnchorBefore, + Lines: []int{1, 2}, + Reporter: checks.RuleDependencyCheckName, + Text: textDependencyRule(1), + Details: detailsDependencyRule("foo", "- `alert` at `foo.yaml:2`\n"), + Severity: checks.Warning, + }, + } + }, + entries: []discovery.Entry{ + parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: alert\n expr: foo == 0\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], + }, + }, + { + description: "ignores unparsable files", + content: "- record: foo\n expr: sum(foo)\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() + }, + prometheus: newSimpleProm, + problems: noProblems, + entries: []discovery.Entry{ + { + ReportedPath: "broken.yaml", + SourcePath: "broken.yaml", + PathError: errors.New("bad file"), + }, + parseWithState("- alert: foo\n expr: up == 0\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], + }, + }, + { + description: "ignores rules with errors", + content: "- record: foo\n expr: sum(foo)\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() + }, + prometheus: newSimpleProm, + problems: noProblems, + entries: []discovery.Entry{ + parseWithState("- recordx: foo\n expr: sum(foo)\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: foo\n expr: up == 0\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], + }, + }, + { + description: "deduplicates affected files", + content: "- record: foo\n expr: sum(foo)\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleDependencyCheck() }, prometheus: newSimpleProm, problems: func(s string) []checks.Problem { return []checks.Problem{ { - Fragment: "expr: foo == 0", - Lines: []int{2}, + Fragment: "record: foo", + Anchor: checks.AnchorBefore, + Lines: []int{1, 2}, Reporter: checks.RuleDependencyCheckName, - Text: textDependencyRule("fake.yml", "foo"), - Details: detailsDependencyRule("foo"), + Text: textDependencyRule(5), + Details: detailsDependencyRule("foo", "- `alert` at `alice.yaml:4`\n- `alert` at `alice.yaml:6`\n- `alert` at `bar.yaml:2`\n- `xxx` at `bar.yaml:2`\n- `alert` at `foo.yaml:2`\n"), Severity: checks.Warning, }, } }, entries: []discovery.Entry{ - parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml")[0], - parseWithState("- alert: foo\n expr: foo == 0\n", discovery.Noop, "foo.yaml")[0], + parseWithState("\n\n- alert: alert\n expr: (foo / foo) == 0\n- alert: alert\n expr: (foo / foo) == 0\n", discovery.Noop, "alice.yaml", "alice.yaml")[1], + parseWithState("\n\n- alert: alert\n expr: (foo / foo) == 0\n- alert: alert\n expr: (foo / foo) == 0\n", discovery.Noop, "alice.yaml", "alice.yaml")[0], + parseWithState("- alert: alert\n expr: (foo / foo) == 0\n", discovery.Noop, "symlink3.yaml", "bar.yaml")[0], + parseWithState("- record: foo\n expr: sum(foo)\n", discovery.Removed, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: alert\n expr: foo == 0\n", discovery.Noop, "foo.yaml", "foo.yaml")[0], + parseWithState("- alert: xxx\n expr: (foo / foo) == 0\n", discovery.Noop, "bar.yaml", "bar.yaml")[0], + parseWithState("- alert: alert\n expr: (foo / foo) == 0\n", discovery.Noop, "bar.yaml", "bar.yaml")[0], + parseWithState("- alert: alert\n expr: foo == 0\n", discovery.Noop, "symlink1.yaml", "foo.yaml")[0], + parseWithState("- alert: alert\n expr: foo == 0\n", discovery.Noop, "symlink2.yaml", "foo.yaml")[0], }, }, } diff --git a/internal/config/config.go b/internal/config/config.go index 85ebdb32..685babbe 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -108,6 +108,10 @@ func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerato name: checks.RegexpCheckName, check: checks.NewRegexpCheck(), }, + { + name: checks.RuleDependencyCheckName, + check: checks.NewRuleDependencyCheck(), + }, } proms := gen.ServersForPath(entry.SourcePath) @@ -148,11 +152,6 @@ func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerato check: checks.NewAlertsExternalLabelsCheck(p), tags: p.Tags(), }) - allChecks = append(allChecks, checkMeta{ - name: checks.RuleDependencyCheckName, - check: checks.NewRuleDependencyCheck(p), - tags: p.Tags(), - }) } for _, rule := range cfg.Rules { diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index ef956642..511684a6 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -5,7 +5,6 @@ import ( "errors" "io" "log/slog" - "regexp" "strings" "time" @@ -44,10 +43,6 @@ func isStrictIgnored(err error) bool { return false } -type RuleFinder interface { - Find() ([]Entry, error) -} - type ChangeType uint8 func (c ChangeType) String() string { @@ -211,12 +206,3 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent slog.Debug("File parsed", slog.String("path", sourcePath), slog.Int("rules", len(entries))) return entries, nil } - -func matchesAny(re []*regexp.Regexp, s string) bool { - for _, r := range re { - if v := r.MatchString(s); v { - return true - } - } - return false -} diff --git a/internal/discovery/git_branch.go b/internal/discovery/git_branch.go index 80f6c458..fe26a613 100644 --- a/internal/discovery/git_branch.go +++ b/internal/discovery/git_branch.go @@ -4,7 +4,6 @@ import ( "bytes" "fmt" "log/slog" - "regexp" "strings" "golang.org/x/exp/slices" @@ -16,33 +15,29 @@ import ( func NewGitBranchFinder( gitCmd git.CommandRunner, - include []*regexp.Regexp, - exclude []*regexp.Regexp, + filter git.PathFilter, baseBranch string, maxCommits int, - relaxed []*regexp.Regexp, ) GitBranchFinder { return GitBranchFinder{ gitCmd: gitCmd, - include: include, - exclude: exclude, + filter: filter, baseBranch: baseBranch, maxCommits: maxCommits, - relaxed: relaxed, } } type GitBranchFinder struct { gitCmd git.CommandRunner - include []*regexp.Regexp - exclude []*regexp.Regexp + filter git.PathFilter baseBranch string maxCommits int - relaxed []*regexp.Regexp } -func (f GitBranchFinder) Find() (entries []Entry, err error) { - slog.Info("Finding all rules to check on current git branch", slog.String("base", f.baseBranch)) +func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) { + for i := range allEntries { + allEntries[i].State = Excluded + } cr, err := git.CommitRange(f.gitCmd, f.baseBranch) if err != nil { @@ -54,7 +49,7 @@ func (f GitBranchFinder) Find() (entries []Entry, err error) { return nil, fmt.Errorf("number of commits to check (%d) is higher than maxCommits (%d), exiting", len(cr.Commits), f.maxCommits) } - changes, err := git.Changes(f.gitCmd, cr) + changes, err := git.Changes(f.gitCmd, cr, f.filter) if err != nil { return nil, err } @@ -68,23 +63,18 @@ func (f GitBranchFinder) Find() (entries []Entry, err error) { } for _, change := range changes { - if !f.isPathAllowed(change.Path.After.Name) { - slog.Debug("Skipping file due to include/exclude rules", slog.String("path", change.Path.After.Name)) - continue - } - var entriesBefore, entriesAfter []Entry entriesBefore, _ = readRules( change.Path.Before.EffectivePath(), change.Path.Before.Name, bytes.NewReader(change.Body.Before), - !matchesAny(f.relaxed, change.Path.Before.Name), + !f.filter.IsRelaxed(change.Path.Before.Name), ) entriesAfter, err = readRules( change.Path.After.EffectivePath(), change.Path.After.Name, bytes.NewReader(change.Body.After), - !matchesAny(f.relaxed, change.Path.After.Name), + !f.filter.IsRelaxed(change.Path.After.Name), ) if err != nil { return nil, fmt.Errorf("invalid file syntax: %w", err) @@ -137,6 +127,10 @@ func (f GitBranchFinder) Find() (entries []Entry, err error) { entries = append(entries, me.after) case me.hasBefore && !me.hasAfter: me.before.State = Removed + ml := commonLines(change.Body.ModifiedLines, me.before.ModifiedLines) + if len(ml) > 0 { + me.before.ModifiedLines = ml + } slog.Debug( "Rule removed on HEAD branch", slog.String("name", me.before.Rule.Name()), @@ -164,32 +158,33 @@ func (f GitBranchFinder) Find() (entries []Entry, err error) { } for _, entry := range symlinks { - if f.isPathAllowed(entry.SourcePath) { + if f.filter.IsPathAllowed(entry.SourcePath) { entries = append(entries, entry) } } - return entries, nil -} - -func (f GitBranchFinder) isPathAllowed(path string) bool { - if len(f.include) == 0 && len(f.exclude) == 0 { - return true - } - - for _, pattern := range f.exclude { - if pattern.MatchString(path) { - return false + var found bool + for _, entry := range entries { + found = false + if entry.State == Removed { + goto NEXT } - } - - for _, pattern := range f.include { - if pattern.MatchString(path) { - return true + for i, globEntry := range allEntries { + if entry.SourcePath == globEntry.SourcePath && entry.Rule.IsSame(globEntry.Rule) { + allEntries[i].State = entry.State + allEntries[i].ModifiedLines = entry.ModifiedLines + found = true + break + } + } + NEXT: + if !found { + allEntries = append(allEntries, entry) } } - return false + slog.Debug("Git branch finder completed", slog.Int("count", len(allEntries))) + return allEntries, nil } func (f GitBranchFinder) shouldSkipAllChecks(changes []*git.FileChange) (bool, error) { diff --git a/internal/discovery/git_branch_test.go b/internal/discovery/git_branch_test.go index a05496fa..eea6fe96 100644 --- a/internal/discovery/git_branch_test.go +++ b/internal/discovery/git_branch_test.go @@ -75,11 +75,9 @@ func TestGitBranchFinder(t *testing.T) { func(args ...string) ([]byte, error) { return nil, fmt.Errorf("mock git error: %v", args) }, - includeAll, - nil, + git.NewPathFilter(includeAll, nil, nil), "main", 50, - nil, ), entries: nil, err: "failed to get the list of commits to scan: mock git error: [log --format=%H --no-abbrev-commit --reverse main..HEAD]", @@ -91,11 +89,9 @@ func TestGitBranchFinder(t *testing.T) { func(args ...string) ([]byte, error) { return nil, fmt.Errorf("mock git error: %v", args) }, - includeAll, - nil, + git.NewPathFilter(includeAll, nil, nil), "master", 50, - nil, ), entries: nil, err: "failed to get the list of commits to scan: mock git error: [log --format=%H --no-abbrev-commit --reverse master..HEAD]", @@ -112,11 +108,9 @@ func TestGitBranchFinder(t *testing.T) { return nil, fmt.Errorf("mock git error: %v", args) } }, - includeAll, - nil, + git.NewPathFilter(includeAll, nil, nil), "main", 3, - nil, ), entries: nil, err: "number of commits to check (4) is higher than maxCommits (3), exiting", @@ -133,11 +127,9 @@ func TestGitBranchFinder(t *testing.T) { return nil, fmt.Errorf("mock git error: %v", args) } }, - includeAll, - nil, + git.NewPathFilter(includeAll, nil, nil), "main", 4, - nil, ), entries: nil, err: "failed to get the list of modified files from git: mock git error: [log --reverse --no-merges --format=%H --name-status c1^..c4]", @@ -156,11 +148,9 @@ func TestGitBranchFinder(t *testing.T) { return nil, fmt.Errorf("mock git error: %v", args) } }, - includeAll, - nil, + git.NewPathFilter(includeAll, nil, nil), "main", 4, - nil, ), entries: nil, err: "failed to get commit message for c1: mock git error: [show -s --format=%B c1]", @@ -185,11 +175,9 @@ func TestGitBranchFinder(t *testing.T) { return nil, fmt.Errorf("mock git error: %v", args) } }, - includeAll, - nil, + git.NewPathFilter(includeAll, nil, nil), "main", 4, - nil, ), entries: nil, err: "failed to run git blame for rules.yml: mock git error: [blame --line-porcelain c1 -- rules.yml]", @@ -204,7 +192,7 @@ func TestGitBranchFinder(t *testing.T) { commitFile(t, "rules.yml", "# v2\n", "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: nil, }, { @@ -229,7 +217,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: []discovery.Entry{ { State: discovery.Excluded, @@ -262,7 +250,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: []discovery.Entry{ { State: discovery.Modified, @@ -289,7 +277,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Modified, @@ -316,7 +304,7 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, nil, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(nil, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Modified, @@ -349,7 +337,12 @@ groups: expr: count(up == 1) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, []*regexp.Regexp{regexp.MustCompile("^foo#")}, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder( + git.RunGit, + git.NewPathFilter([]*regexp.Regexp{regexp.MustCompile("^foo#")}, nil, nil), + "main", + 4, + ), entries: nil, }, { @@ -374,7 +367,7 @@ groups: expr: count(up == 1) `, "v2\nskip this commit\n[skip ci]\n") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: nil, }, { @@ -399,7 +392,7 @@ groups: expr: count(up == 1) `, "v2\nskip this commit\n[no ci]\n") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: nil, }, { @@ -419,7 +412,7 @@ groups: require.NoError(t, err, "git add") gitCommit(t, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Added, @@ -462,7 +455,7 @@ groups: expr: count(up) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: []discovery.Entry{ { State: discovery.Modified, @@ -496,7 +489,7 @@ groups: State: discovery.Removed, ReportedPath: "rules.yml", SourcePath: "rules.yml", - ModifiedLines: []int{7, 8}, + ModifiedLines: []int{7}, Rule: mustParse(6, "- record: up:count:2\n expr: count(up)\n"), }, }, @@ -524,7 +517,7 @@ groups: for: 0s `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Modified, @@ -560,7 +553,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Excluded, @@ -596,7 +589,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Excluded, @@ -636,7 +629,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Excluded, @@ -684,7 +677,7 @@ groups: expr: count(up) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, nil), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, nil), "main", 4), entries: []discovery.Entry{ { State: discovery.Added, @@ -697,7 +690,7 @@ groups: State: discovery.Removed, ReportedPath: "rules.yml", SourcePath: "rules.yml", - ModifiedLines: []int{1, 2, 3, 4, 5, 6, 7}, + ModifiedLines: []int{3}, PathError: mustErr(` groups: - name: v2 @@ -733,7 +726,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Excluded, @@ -787,7 +780,7 @@ groups: expr: sum(foo) by(job) `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Modified, @@ -848,7 +841,7 @@ groups: foo: bar `, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Excluded, @@ -882,7 +875,7 @@ groups: gitCommit(t, "v2") }, - finder: discovery.NewGitBranchFinder(git.RunGit, includeAll, nil, "main", 4, includeAll), + finder: discovery.NewGitBranchFinder(git.RunGit, git.NewPathFilter(includeAll, nil, includeAll), "main", 4), entries: []discovery.Entry{ { State: discovery.Moved, @@ -905,7 +898,7 @@ groups: require.NoError(t, err, "git init") tc.setup(t) - entries, err := tc.finder.Find() + entries, err := tc.finder.Find(nil) if tc.err != "" { require.EqualError(t, err, tc.err) } else { diff --git a/internal/discovery/glob.go b/internal/discovery/glob.go index f3c60463..294a5f70 100644 --- a/internal/discovery/glob.go +++ b/internal/discovery/glob.go @@ -6,32 +6,40 @@ import ( "log/slog" "os" "path/filepath" - "regexp" + + "github.com/cloudflare/pint/internal/git" ) -func NewGlobFinder(patterns []string, relaxed []*regexp.Regexp) GlobFinder { +func NewGlobFinder(patterns []string, filter git.PathFilter) GlobFinder { return GlobFinder{ patterns: patterns, - relaxed: relaxed, + filter: filter, } } type GlobFinder struct { patterns []string - relaxed []*regexp.Regexp + filter git.PathFilter } func (f GlobFinder) Find() (entries []Entry, err error) { - slog.Info("Finding all rules to check", slog.Any("paths", f.patterns)) - paths := filePaths{} for _, p := range f.patterns { matches, err := filepath.Glob(p) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to expand file path pattern %s: %w", p, err) } for _, path := range matches { + if path == ".git" && isDir(path) { + slog.Debug( + "Excluding git directory from glob results", + slog.String("path", path), + slog.String("glob", p), + ) + continue + } + subpaths, err := findFiles(path) if err != nil { return nil, err @@ -49,11 +57,15 @@ func (f GlobFinder) Find() (entries []Entry, err error) { } for _, fp := range paths { + if !f.filter.IsPathAllowed(fp.path) { + continue + } + fd, err := os.Open(fp.path) if err != nil { return nil, err } - el, err := readRules(fp.target, fp.path, fd, !matchesAny(f.relaxed, fp.target)) + el, err := readRules(fp.target, fp.path, fd, !f.filter.IsRelaxed(fp.target)) if err != nil { fd.Close() return nil, fmt.Errorf("invalid file syntax: %w", err) @@ -68,9 +80,18 @@ func (f GlobFinder) Find() (entries []Entry, err error) { } } + slog.Debug("Glob finder completed", slog.Int("count", len(entries))) return entries, nil } +func isDir(path string) bool { + info, err := os.Stat(path) + if err != nil { + return false + } + return info.IsDir() +} + type filePath struct { path string target string @@ -88,6 +109,11 @@ func (fps filePaths) hasPath(p string) bool { } func findFiles(path string) (paths filePaths, err error) { + target, err := filepath.EvalSymlinks(path) + if err != nil { + return nil, fmt.Errorf("%s is a symlink but target file cannot be evaluated: %w", path, err) + } + s, err := os.Stat(path) if err != nil { return nil, err @@ -102,10 +128,6 @@ func findFiles(path string) (paths filePaths, err error) { } paths = append(paths, subpaths...) default: - target, err := filepath.EvalSymlinks(path) - if err != nil { - return nil, err - } paths = append(paths, filePath{path: path, target: target}) } @@ -126,7 +148,7 @@ func walkDir(dirname string) (paths filePaths, err error) { default: dest, err := filepath.EvalSymlinks(path) if err != nil { - return err + return fmt.Errorf("%s is a symlink but target file cannot be evaluated: %w", path, err) } s, err := os.Stat(dest) diff --git a/internal/discovery/glob_test.go b/internal/discovery/glob_test.go index 29251f8e..c812307e 100644 --- a/internal/discovery/glob_test.go +++ b/internal/discovery/glob_test.go @@ -5,7 +5,6 @@ import ( "errors" "os" "path" - "path/filepath" "regexp" "strconv" "strings" @@ -15,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/git" "github.com/cloudflare/pint/internal/parser" ) @@ -43,32 +43,32 @@ func TestGlobPathFinder(t *testing.T) { testCases := []testCaseT{ { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"[]"}, nil), - err: filepath.ErrBadPattern.Error(), + finder: discovery.NewGlobFinder([]string{"[]"}, git.NewPathFilter(nil, nil, nil)), + err: "failed to expand file path pattern []: syntax error in pattern", }, { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), err: "no matching files", }, { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), err: "no matching files", }, { files: map[string]string{}, - finder: discovery.NewGlobFinder([]string{"foo/*"}, nil), + finder: discovery.NewGlobFinder([]string{"foo/*"}, git.NewPathFilter(nil, nil, nil)), err: "no matching files", }, { files: map[string]string{"bar.yml": testRuleBody}, - finder: discovery.NewGlobFinder([]string{"foo/*"}, nil), + finder: discovery.NewGlobFinder([]string{"foo/*"}, git.NewPathFilter(nil, nil, nil)), err: "no matching files", }, { files: map[string]string{"bar.yml": testRuleBody}, - finder: discovery.NewGlobFinder([]string{"*"}, []*regexp.Regexp{regexp.MustCompile(".*")}), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")})), entries: []discovery.Entry{ { State: discovery.Noop, @@ -82,7 +82,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{"*"}, []*regexp.Regexp{regexp.MustCompile(".*")}), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")})), entries: []discovery.Entry{ { State: discovery.Noop, @@ -96,7 +96,7 @@ func TestGlobPathFinder(t *testing.T) { }, { files: map[string]string{"bar.yml": testRuleBody}, - finder: discovery.NewGlobFinder([]string{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, 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{"*"}, []*regexp.Regexp{regexp.MustCompile(".*")}), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")})), entries: []discovery.Entry{ { State: discovery.Noop, @@ -125,7 +125,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{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), entries: []discovery.Entry{ { State: discovery.Noop, @@ -151,7 +151,7 @@ func TestGlobPathFinder(t *testing.T) { "b/link.yml": "../a/bar.yml", "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), entries: []discovery.Entry{ { State: discovery.Noop, @@ -185,22 +185,22 @@ func TestGlobPathFinder(t *testing.T) { "b/link.yml": "../a/bar.yml", "b/c/link.yml": "../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, nil), - err: "lstat b/a: no such file or directory", + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), + err: "b/c/link.yml is a symlink but target file cannot be evaluated: lstat b/a: no such file or directory", }, { files: map[string]string{"a/bar.yml": "xxx:\n"}, symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, []*regexp.Regexp{regexp.MustCompile(".*")}), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")})), }, { 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{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), entries: []discovery.Entry{ { State: discovery.Noop, @@ -225,21 +225,21 @@ func TestGlobPathFinder(t *testing.T) { symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, []*regexp.Regexp{regexp.MustCompile(".*")}), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")})), }, { files: map[string]string{"a/bar.yml": "xxx:\nyyy:\n"}, symlinks: map[string]string{ "b/c/d": "../../a", }, - finder: discovery.NewGlobFinder([]string{"*"}, []*regexp.Regexp{regexp.MustCompile(".*")}), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, []*regexp.Regexp{regexp.MustCompile(".*")})), }, { files: map[string]string{"a/bar.yml": testRuleBody}, symlinks: map[string]string{ "b/c/link.yml": "../../a/bar.yml", }, - finder: discovery.NewGlobFinder([]string{"*"}, nil), + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), entries: []discovery.Entry{ { State: discovery.Noop, @@ -263,8 +263,8 @@ func TestGlobPathFinder(t *testing.T) { symlinks: map[string]string{ "input.yml": "/xx/ccc/fdd", }, - finder: discovery.NewGlobFinder([]string{"*"}, nil), - err: "stat input.yml: no such file or directory", + finder: discovery.NewGlobFinder([]string{"*"}, git.NewPathFilter(nil, nil, nil)), + err: "input.yml is a symlink but target file cannot be evaluated: lstat /xx: no such file or directory", }, } diff --git a/internal/git/changes.go b/internal/git/changes.go index 11038e50..f8f90e01 100644 --- a/internal/git/changes.go +++ b/internal/git/changes.go @@ -67,7 +67,7 @@ type FileChange struct { Body BodyDiff } -func Changes(cmd CommandRunner, cr CommitRangeResults) ([]*FileChange, error) { +func Changes(cmd CommandRunner, cr CommitRangeResults, filter PathFilter) ([]*FileChange, error) { out, err := cmd("log", "--reverse", "--no-merges", "--format=%H", "--name-status", cr.String()) if err != nil { return nil, fmt.Errorf("failed to get the list of modified files from git: %w", err) @@ -97,6 +97,11 @@ func Changes(cmd CommandRunner, cr CommitRangeResults) ([]*FileChange, error) { dstPath := parts[len(parts)-1] slog.Debug("Git file change", slog.String("change", parts[0]), slog.String("path", dstPath), slog.String("commit", commit)) + if !filter.IsPathAllowed(dstPath) { + slog.Debug("Skipping file due to include/exclude rules", slog.String("path", dstPath)) + continue + } + // ignore directories // FIXME move all files instead? if isDir, _ := isDirectoryPath(dstPath); isDir { diff --git a/internal/git/changes_test.go b/internal/git/changes_test.go index 281168b0..5ef92ca7 100644 --- a/internal/git/changes_test.go +++ b/internal/git/changes_test.go @@ -565,7 +565,7 @@ func TestChanges(t *testing.T) { require.NoError(t, err, "chdir") cmd, cr := tc.setup(t) - changes, err := git.Changes(cmd, cr) + changes, err := git.Changes(cmd, cr, git.NewPathFilter(nil, nil, nil)) if tc.err != "" { require.EqualError(t, err, tc.err) require.Nil(t, changes) diff --git a/internal/git/filter.go b/internal/git/filter.go new file mode 100644 index 00000000..414a9a50 --- /dev/null +++ b/internal/git/filter.go @@ -0,0 +1,46 @@ +package git + +import "regexp" + +func NewPathFilter(include, exclude, relaxed []*regexp.Regexp) PathFilter { + return PathFilter{ + include: include, + exclude: exclude, + relaxed: relaxed, + } +} + +type PathFilter struct { + include []*regexp.Regexp + exclude []*regexp.Regexp + relaxed []*regexp.Regexp +} + +func (pf PathFilter) IsPathAllowed(path string) bool { + if len(pf.include) == 0 && len(pf.exclude) == 0 { + return true + } + + for _, pattern := range pf.exclude { + if pattern.MatchString(path) { + return false + } + } + + for _, pattern := range pf.include { + if pattern.MatchString(path) { + return true + } + } + + return false +} + +func (pf PathFilter) IsRelaxed(path string) bool { + for _, r := range pf.relaxed { + if v := r.MatchString(path); v { + return true + } + } + return false +} diff --git a/internal/reporter/bitbucket_api.go b/internal/reporter/bitbucket_api.go index ab6c50a6..f57c1b94 100644 --- a/internal/reporter/bitbucket_api.go +++ b/internal/reporter/bitbucket_api.go @@ -126,10 +126,10 @@ type BitBucketFileDiffs struct { type bitBucketComment struct { id int version int - onCommit bool text string - path string - line int + severity string + anchor BitBucketCommentAnchor + replies int } type BitBucketCommentAuthor struct { @@ -137,20 +137,39 @@ type BitBucketCommentAuthor struct { } type BitBucketPullRequestComment struct { - ID int `json:"id"` - Version int `json:"version"` - State string `json:"state"` - Author BitBucketCommentAuthor `json:"author"` - Text string `json:"text"` + ID int `json:"id"` + Version int `json:"version"` + State string `json:"state"` + Author BitBucketCommentAuthor `json:"author"` + Text string `json:"text"` + Severity string `json:"severity"` + Comments []BitBucketPullRequestComment `json:"comments"` } type BitBucketCommentAnchor struct { Orphaned bool `json:"orphaned"` + LineType string `json:"lineType"` DiffType string `json:"diffType"` Path string `json:"path"` Line int `json:"line"` } +func (ba BitBucketCommentAnchor) isEqual(pa BitBucketPendingCommentAnchor) bool { + if ba.Path != pa.Path { + return false + } + if ba.Line != pa.Line { + return false + } + if ba.LineType != pa.LineType { + return false + } + if ba.DiffType != pa.DiffType { + return false + } + return true +} + type BitBucketPullRequestActivity struct { Action string `json:"action"` CommentAction string `json:"commentAction"` @@ -166,9 +185,11 @@ type BitBucketPullRequestActivities struct { } type pendingComment struct { - text string - path string - line int + severity string + text string + path string + line int + anchor checks.Anchor } func (pc pendingComment) toBitBucketComment(changes *bitBucketPRChanges) BitBucketPendingComment { @@ -181,10 +202,12 @@ func (pc pendingComment) toBitBucketComment(changes *bitBucketPRChanges) BitBuck FileType: "FROM", }, Text: pc.text, - Severity: "NORMAL", + Severity: pc.severity, } - if changes != nil { + if pc.anchor == checks.AnchorBefore { + c.Anchor.LineType = "REMOVED" + } else if changes != nil { if lines, ok := changes.pathModifiedLines[pc.path]; ok && slices.Contains(lines, pc.line) { c.Anchor.LineType = "ADDED" c.Anchor.FileType = "TO" @@ -215,6 +238,16 @@ type BitBucketPendingComment struct { Anchor BitBucketPendingCommentAnchor `json:"anchor"` } +type BitBucketCommentStateUpdate struct { + State string `json:"state"` + Version int `json:"version"` +} + +type BitBucketCommentSeverityUpdate struct { + Severity string `json:"severity"` + Version int `json:"version"` +} + func newBitBucketAPI(pintVersion, uri string, timeout time.Duration, token, project, repo string) *bitBucketAPI { return &bitBucketAPI{ pintVersion: pintVersion, @@ -315,7 +348,8 @@ func (bb bitBucketAPI) createReport(summary Summary, commit string) error { Details: BitBucketDescription, Link: "https://cloudflare.github.io/pint/", Data: []BitBucketReportData{ - {Title: "Number of rules checked", Type: NumberType, Value: summary.Entries}, + {Title: "Number of rules parsed", Type: NumberType, Value: summary.TotalEntries}, + {Title: "Number of rules checked", Type: NumberType, Value: summary.CheckedEntries}, {Title: "Number of problems found", Type: NumberType, Value: reportedProblems}, {Title: "Number of offline checks", Type: NumberType, Value: summary.OfflineChecks}, {Title: "Number of online checks", Type: NumberType, Value: summary.OnlineChecks}, @@ -521,10 +555,10 @@ func (bb bitBucketAPI) getPullRequestComments(pr *bitBucketPR) ([]bitBucketComme comments = append(comments, bitBucketComment{ id: act.Comment.ID, version: act.Comment.Version, - onCommit: act.CommentAnchor.DiffType == "COMMIT", text: act.Comment.Text, - path: act.CommentAnchor.Path, - line: act.CommentAnchor.Line, + anchor: act.CommentAnchor, + severity: act.Comment.Severity, + replies: len(act.Comment.Comments), }) } } @@ -546,16 +580,8 @@ func (bb bitBucketAPI) makeComments(summary Summary, changes *bitBucketPRChanges } var buf strings.Builder - var icon string - switch reports[0].Problem.Severity { - case checks.Fatal, checks.Bug: - icon = ":stop_sign:" - case checks.Warning: - icon = ":warning:" - case checks.Information: - icon = ":information_source:" - } - buf.WriteString(icon) + + buf.WriteString(problemIcon(reports[0].Problem.Severity)) buf.WriteString(" **") buf.WriteString(reports[0].Problem.Severity.String()) buf.WriteString("** reported by [pint](https://cloudflare.github.io/pint/) **") @@ -581,10 +607,21 @@ func (bb bitBucketAPI) makeComments(summary Summary, changes *bitBucketPRChanges buf.WriteString(reports[0].Problem.Reporter) buf.WriteString(".html).\n") + var severity string + // nolint:exhaustive + switch reports[0].Problem.Severity { + case checks.Bug, checks.Fatal: + severity = "BLOCKER" + default: + severity = "NORMAL" + } + pending := pendingComment{ - path: reports[0].ReportedPath, - line: reports[0].Problem.Lines[0], - text: buf.String(), + severity: severity, + path: reports[0].ReportedPath, + line: reports[0].Problem.Lines[0], + text: buf.String(), + anchor: reports[0].Problem.Anchor, } comments = append(comments, pending.toBitBucketComment(changes)) } @@ -595,50 +632,122 @@ func (bb bitBucketAPI) pruneComments(pr *bitBucketPR, currentComments []bitBucke for _, cur := range currentComments { var keep bool for _, pend := range pendingComments { - if cur.path == pend.Anchor.Path && cur.line == pend.Anchor.Line && cur.text == pend.Text { + if cur.anchor.isEqual(pend.Anchor) && cur.text == pend.Text { keep = true break } - if cur.onCommit { + if cur.anchor.DiffType == "COMMIT" { keep = false } } if !keep { - slog.Debug( - "Deleting stale comment", - slog.Int("id", cur.id), - slog.String("path", cur.path), - slog.Int("line", cur.line), - ) - _, err := bb.request( - http.MethodDelete, - fmt.Sprintf("/rest/api/1.0/projects/%s/repos/%s/pull-requests/%d/comments/%d?version=%d", - bb.project, bb.repo, - pr.ID, - cur.id, cur.version, - ), - nil, - ) - if err != nil { - slog.Error( - "Failed to delete stale BitBucket pull request comment", - slog.Int("id", cur.id), - slog.Any("err", err), - ) + switch { + case cur.replies == 0: + bb.deleteComment(pr, cur) + case cur.severity == "BLOCKER": + bb.resolveTask(pr, cur) + default: + bb.updateSeverity(pr, cur, "BLOCKER") + bb.resolveTask(pr, cur) } } } } +func (bb bitBucketAPI) deleteComment(pr *bitBucketPR, cur bitBucketComment) { + slog.Debug( + "Deleting stale comment", + slog.Int("id", cur.id), + slog.String("path", cur.anchor.Path), + slog.Int("line", cur.anchor.Line), + ) + _, err := bb.request( + http.MethodDelete, + fmt.Sprintf("/rest/api/1.0/projects/%s/repos/%s/pull-requests/%d/comments/%d?version=%d", + bb.project, bb.repo, + pr.ID, + cur.id, cur.version, + ), + nil, + ) + if err != nil { + slog.Error( + "Failed to delete stale BitBucket pull request comment", + slog.Int("id", cur.id), + slog.Any("err", err), + ) + } +} + +func (bb bitBucketAPI) resolveTask(pr *bitBucketPR, cur bitBucketComment) { + slog.Debug( + "Resolving stale blocker comment", + slog.Int("id", cur.id), + slog.String("path", cur.anchor.Path), + slog.Int("line", cur.anchor.Line), + ) + payload, _ := json.Marshal(BitBucketCommentStateUpdate{ + State: "RESOLVED", + Version: cur.version, + }) + _, err := bb.request( + http.MethodPut, + fmt.Sprintf("/rest/api/1.0/projects/%s/repos/%s/pull-requests/%d/comments/%d", + bb.project, bb.repo, + pr.ID, + cur.id, + ), + bytes.NewReader(payload), + ) + if err != nil { + slog.Error( + "Failed to resolve stale blocker BitBucket pull request comment", + slog.Int("id", cur.id), + slog.Any("err", err), + ) + } +} + +func (bb bitBucketAPI) updateSeverity(pr *bitBucketPR, cur bitBucketComment, severity string) { + slog.Debug( + "Updating comment severity", + slog.Int("id", cur.id), + slog.String("path", cur.anchor.Path), + slog.Int("line", cur.anchor.Line), + slog.String("from", cur.severity), + slog.String("to", severity), + ) + payload, _ := json.Marshal(BitBucketCommentSeverityUpdate{ + Severity: severity, + Version: cur.version, + }) + _, err := bb.request( + http.MethodPut, + fmt.Sprintf("/rest/api/1.0/projects/%s/repos/%s/pull-requests/%d/comments/%d", + bb.project, bb.repo, + pr.ID, + cur.id, + ), + bytes.NewReader(payload), + ) + if err != nil { + slog.Error( + "Failed to update BitBucket pull request comment severity", + slog.Int("id", cur.id), + slog.Any("err", err), + ) + } +} + func (bb bitBucketAPI) addComments(pr *bitBucketPR, currentComments []bitBucketComment, pendingComments []BitBucketPendingComment) error { var added int for _, pend := range pendingComments { add := true for _, cur := range currentComments { - if cur.path == pend.Anchor.Path && cur.line == pend.Anchor.Line && cur.text == pend.Text { + if cur.anchor.isEqual(pend.Anchor) && cur.text == pend.Text { add = false } - if cur.onCommit { + if cur.anchor.DiffType == "COMMIT" { add = true } } @@ -725,6 +834,9 @@ func dedupReports(src []Report) (dst [][]Report) { if d[0].Problem.Lines[0] != report.Problem.Lines[0] { continue } + if d[0].Problem.Anchor != report.Problem.Anchor { + continue + } index = i break } @@ -736,3 +848,15 @@ func dedupReports(src []Report) (dst [][]Report) { } return dst } + +func problemIcon(s checks.Severity) string { + // nolint:exhaustive + switch s { + case checks.Warning: + return ":warning:" + case checks.Information: + return ":information_source:" + default: + return ":stop_sign:" + } +} diff --git a/internal/reporter/bitbucket_api_test.go b/internal/reporter/bitbucket_api_test.go index de3318d3..0a5ad2f1 100644 --- a/internal/reporter/bitbucket_api_test.go +++ b/internal/reporter/bitbucket_api_test.go @@ -22,9 +22,10 @@ func TestPendingCommentToBitBucketComment(t *testing.T) { { description: "nil changes", input: pendingComment{ - text: "this is text", - path: "foo.yaml", - line: 5, + severity: "NORMAL", + text: "this is text", + path: "foo.yaml", + line: 5, }, output: BitBucketPendingComment{ Text: "this is text", @@ -42,9 +43,10 @@ func TestPendingCommentToBitBucketComment(t *testing.T) { { description: "path not found in changes", input: pendingComment{ - text: "this is text", - path: "foo.yaml", - line: 5, + severity: "NORMAL", + text: "this is text", + path: "foo.yaml", + line: 5, }, output: BitBucketPendingComment{ Text: "this is text", @@ -65,9 +67,10 @@ func TestPendingCommentToBitBucketComment(t *testing.T) { { description: "path found in changes", input: pendingComment{ - text: "this is text", - path: "foo.yaml", - line: 5, + severity: "NORMAL", + text: "this is text", + path: "foo.yaml", + line: 5, }, output: BitBucketPendingComment{ Text: "this is text", diff --git a/internal/reporter/bitbucket_test.go b/internal/reporter/bitbucket_test.go index 4a28eb45..73eb40ae 100644 --- a/internal/reporter/bitbucket_test.go +++ b/internal/reporter/bitbucket_test.go @@ -64,6 +64,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "PASS", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -307,6 +308,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "FAIL", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(3)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -549,6 +551,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "FAIL", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(4)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -624,6 +627,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "FAIL", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(1)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -725,6 +729,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "FAIL", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(4)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -880,6 +885,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "FAIL", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(4)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -890,7 +896,7 @@ func TestBitBucketReporter(t *testing.T) { pullRequestComments: []reporter.BitBucketPendingComment{ { Text: ":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nthis should be ignored, line is not part of the diff\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock.html).\n", - Severity: "NORMAL", + Severity: "BLOCKER", Anchor: reporter.BitBucketPendingCommentAnchor{ Path: "foo.txt", Line: 1, @@ -901,7 +907,7 @@ func TestBitBucketReporter(t *testing.T) { }, { Text: ":stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nbad name\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock.html).\n", - Severity: "NORMAL", + Severity: "BLOCKER", Anchor: reporter.BitBucketPendingCommentAnchor{ Path: "foo.txt", Line: 2, @@ -912,7 +918,7 @@ func TestBitBucketReporter(t *testing.T) { }, { Text: ":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nmock text\n\nmock details\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock.html).\n", - Severity: "NORMAL", + Severity: "BLOCKER", Anchor: reporter.BitBucketPendingCommentAnchor{ Path: "foo.txt", Line: 2, @@ -1035,6 +1041,7 @@ func TestBitBucketReporter(t *testing.T) { CommentAction: "ADDED", CommentAnchor: reporter.BitBucketCommentAnchor{ Orphaned: true, + LineType: "CONTEXT", DiffType: "EFFECTIVE", Path: "foo.txt", Line: 3, @@ -1066,6 +1073,26 @@ func TestBitBucketReporter(t *testing.T) { }, }, }, + { + Action: "COMMENTED", + CommentAction: "ADDED", + CommentAnchor: reporter.BitBucketCommentAnchor{ + Orphaned: true, + LineType: "REMOVED", + DiffType: "COMMIT", + Path: "foo.txt", + Line: 14, + }, + Comment: reporter.BitBucketPullRequestComment{ + ID: 1003, + Version: 1, + State: "OPEN", + Severity: "BLOCKER", + Author: reporter.BitBucketCommentAuthor{ + Name: "pint_user", + }, + }, + }, { Action: "COMMENTED", CommentAction: "ADDED", @@ -1592,6 +1619,7 @@ func TestBitBucketReporter(t *testing.T) { Link: "https://cloudflare.github.io/pint/", Result: "FAIL", Data: []reporter.BitBucketReportData{ + {Title: "Number of rules parsed", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(5)}, {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, @@ -1602,7 +1630,7 @@ func TestBitBucketReporter(t *testing.T) { pullRequestComments: []reporter.BitBucketPendingComment{ { Text: ":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nthis should be ignored, line is not part of the diff\n\n------\n\nthis should be ignored, line is not part of the diff\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock.html).\n", - Severity: "NORMAL", + Severity: "BLOCKER", Anchor: reporter.BitBucketPendingCommentAnchor{ Path: "foo.txt", Line: 1, diff --git a/internal/reporter/console.go b/internal/reporter/console.go index ab76b30f..135eafc8 100644 --- a/internal/reporter/console.go +++ b/internal/reporter/console.go @@ -24,7 +24,7 @@ type ConsoleReporter struct { minSeverity checks.Severity } -func (cr ConsoleReporter) Submit(summary Summary) error { +func (cr ConsoleReporter) Submit(summary Summary) (err error) { reports := summary.Reports() sort.Slice(reports, func(i, j int) bool { if reports[i].SourcePath < reports[j].SourcePath { @@ -58,9 +58,12 @@ func (cr ConsoleReporter) Submit(summary Summary) error { perFile[report.SourcePath] = []string{} } - content, err := readFile(report.SourcePath) - if err != nil { - return err + var content string + if report.Problem.Anchor == checks.AnchorAfter { + content, err = readFile(report.SourcePath) + if err != nil { + return err + } } msg := []string{} @@ -71,8 +74,13 @@ func (cr ConsoleReporter) Submit(summary Summary) error { if report.SourcePath != report.ReportedPath { path = fmt.Sprintf("%s ~> %s", report.SourcePath, report.ReportedPath) } + path = color.CyanString("%s:%s", path, printLineRange(firstLine, lastLine)) + if report.Problem.Anchor == checks.AnchorBefore { + path += " " + color.RedString("(deleted)") + } + path += " " - msg = append(msg, color.CyanString("%s:%s ", path, printLineRange(firstLine, lastLine))) + msg = append(msg, path) switch report.Problem.Severity { case checks.Bug, checks.Fatal: msg = append(msg, color.RedString("%s: %s", report.Problem.Severity, report.Problem.Text)) @@ -83,29 +91,32 @@ func (cr ConsoleReporter) Submit(summary Summary) error { } msg = append(msg, color.MagentaString(" (%s)\n", report.Problem.Reporter)) - lines := strings.Split(content, "\n") - if lastLine > len(lines)-1 { - lastLine = len(lines) - 1 - slog.Warn( - "Tried to read more lines than present in the source file, this is likely due to '\n' usage in some rules, see https://github.com/cloudflare/pint/issues/20 for details", - slog.String("path", report.SourcePath), - ) - } + if report.Problem.Anchor == checks.AnchorAfter { + lines := strings.Split(content, "\n") + if lastLine > len(lines)-1 { + lastLine = len(lines) - 1 + slog.Warn( + "Tried to read more lines than present in the source file, this is likely due to '\n' usage in some rules, see https://github.com/cloudflare/pint/issues/20 for details", + slog.String("path", report.SourcePath), + ) + } - nrFmt := fmt.Sprintf("%%%dd", countDigits(lastLine)+1) - var inPlaceholder bool - for i := firstLine; i <= lastLine; i++ { - switch { - case slices.Contains(report.Problem.Lines, i): - msg = append(msg, color.WhiteString(nrFmt+" | %s\n", i, lines[i-1])) - inPlaceholder = false - case inPlaceholder: - // - default: - msg = append(msg, color.WhiteString(" %s\n", strings.Repeat(".", countDigits(lastLine)))) - inPlaceholder = true + nrFmt := fmt.Sprintf("%%%dd", countDigits(lastLine)+1) + var inPlaceholder bool + for i := firstLine; i <= lastLine; i++ { + switch { + case slices.Contains(report.Problem.Lines, i): + msg = append(msg, color.WhiteString(nrFmt+" | %s\n", i, lines[i-1])) + inPlaceholder = false + case inPlaceholder: + // + default: + msg = append(msg, color.WhiteString(" %s\n", strings.Repeat(".", countDigits(lastLine)))) + inPlaceholder = true + } } } + perFile[report.SourcePath] = append(perFile[report.SourcePath], strings.Join(msg, "")) } diff --git a/internal/reporter/github.go b/internal/reporter/github.go index 825063bd..50dca7bf 100644 --- a/internal/reporter/github.go +++ b/internal/reporter/github.go @@ -9,7 +9,7 @@ import ( "strings" "time" - "github.com/google/go-github/v55/github" + "github.com/google/go-github/v57/github" "golang.org/x/oauth2" "github.com/cloudflare/pint/internal/checks" @@ -137,13 +137,20 @@ func (gr GithubReporter) addReviewComments(headCommit string, summary Summary) e var found bool for _, ec := range existingComments { - if ec.GetBody() == comment.GetBody() && ec.GetCommitID() == comment.GetCommitID() { + if ec.GetBody() == comment.GetBody() && + ec.GetCommitID() == comment.GetCommitID() && + ec.GetLine() == comment.GetLine() { found = true break } } if found { - slog.Debug("Comment already exist", slog.String("body", comment.GetBody()), slog.String("commit", comment.GetCommitID())) + slog.Debug("Comment already exist", + slog.String("path", comment.GetPath()), + slog.Int("line", comment.GetLine()), + slog.String("commit", comment.GetCommitID()), + slog.String("body", comment.GetBody()), + ) continue } @@ -229,8 +236,12 @@ func formatGHReviewBody(version string, summary Summary) string { b.WriteString(version) b.WriteString(" |\n") + b.WriteString("| Number of rules parsed | ") + b.WriteString(strconv.Itoa(summary.TotalEntries)) + b.WriteString(" |\n") + b.WriteString("| Number of rules checked | ") - b.WriteString(strconv.Itoa(summary.Entries)) + b.WriteString(strconv.FormatInt(summary.CheckedEntries, 10)) b.WriteString(" |\n") b.WriteString("| Number of problems found | ") @@ -252,7 +263,7 @@ func formatGHReviewBody(version string, summary Summary) string { b.WriteString("\n

\n\n\n") b.WriteString("
Problems\n

\n\n") - if summary.Entries > 0 { + if len(summary.Reports()) > 0 { buf := bytes.NewBuffer(nil) cr := NewConsoleReporter(buf, checks.Information) err := cr.Submit(summary) @@ -281,11 +292,19 @@ func reportToGitHubComment(headCommit string, rep Report) *github.PullRequestCom msgSuffix = "\n\n" + rep.Problem.Details } + var side string + if rep.Problem.Anchor == checks.AnchorBefore { + side = "LEFT" + } else { + side = "RIGHT" + } + c := github.PullRequestComment{ CommitID: github.String(headCommit), Path: github.String(rep.ReportedPath), Body: github.String(fmt.Sprintf( - "[%s](https://cloudflare.github.io/pint/checks/%s.html): %s%s%s", + "%s [%s](https://cloudflare.github.io/pint/checks/%s.html): %s%s%s", + problemIcon(rep.Problem.Severity), rep.Problem.Reporter, rep.Problem.Reporter, msgPrefix, @@ -293,6 +312,7 @@ func reportToGitHubComment(headCommit string, rep Report) *github.PullRequestCom msgSuffix, )), Line: github.Int(reportLine), + Side: github.String(side), } return &c diff --git a/internal/reporter/reporter.go b/internal/reporter/reporter.go index 0679388d..d6b10109 100644 --- a/internal/reporter/reporter.go +++ b/internal/reporter/reporter.go @@ -48,11 +48,12 @@ func (r Report) isEqual(nr Report) bool { } type Summary struct { - OfflineChecks int64 - OnlineChecks int64 - Duration time.Duration - Entries int - reports []Report + OfflineChecks int64 + OnlineChecks int64 + Duration time.Duration + TotalEntries int + CheckedEntries int64 + reports []Report } func NewSummary(reports []Report) Summary {