Merge pull request #939 from cloudflare/blame

Better git blame handling

cmd/pint/lint.go

@@ -143,8 +143,10 @@ func verifyOwners(entries []discovery.Entry, allowedOwners []*regexp.Regexp) (re
 		}
 		if entry.Owner == "" {
 			reports = append(reports, reporter.Report{
-				ReportedPath:  entry.ReportedPath,
-				SourcePath:    entry.SourcePath,
+				Path: discovery.Path{
+					Name:          entry.Path.Name,
+					SymlinkTarget: entry.Path.SymlinkTarget,
+				},
 				ModifiedLines: entry.ModifiedLines,
 				Rule:          entry.Rule,
 				Problem: checks.Problem{
@@ -163,8 +165,10 @@ func verifyOwners(entries []discovery.Entry, allowedOwners []*regexp.Regexp) (re
 			}
 		}
 		reports = append(reports, reporter.Report{
-			ReportedPath:  entry.ReportedPath,
-			SourcePath:    entry.SourcePath,
+			Path: discovery.Path{
+				Name:          entry.Path.Name,
+				SymlinkTarget: entry.Path.SymlinkTarget,
+			},
 			ModifiedLines: entry.ModifiedLines,
 			Rule:          entry.Rule,
 			Problem: checks.Problem{

cmd/pint/scan.go

@@ -118,15 +118,15 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr
 				if entry.Rule.RecordingRule != nil {
 					rulesParsedTotal.WithLabelValues(config.RecordingRuleType).Inc()
 					slog.Debug("Found recording rule",
-						slog.String("path", entry.SourcePath),
+						slog.String("path", entry.Path.Name),
 						slog.String("record", entry.Rule.RecordingRule.Record.Value),
 						slog.String("lines", entry.Rule.Lines.String()),
 					)
 				}
 				if entry.Rule.AlertingRule != nil {
 					rulesParsedTotal.WithLabelValues(config.AlertingRuleType).Inc()
 					slog.Debug("Found alerting rule",
-						slog.String("path", entry.SourcePath),
+						slog.String("path", entry.Path.Name),
 						slog.String("alert", entry.Rule.AlertingRule.Alert.Value),
 						slog.String("lines", entry.Rule.Lines.String()),
 					)
@@ -146,7 +146,7 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr
 			default:
 				if entry.Rule.Error.Err != nil {
 					slog.Debug("Found invalid rule",
-						slog.String("path", entry.SourcePath),
+						slog.String("path", entry.Path.Name),
 						slog.String("lines", entry.Rule.Lines.String()),
 					)
 					rulesParsedTotal.WithLabelValues(config.InvalidRuleType).Inc()
@@ -189,8 +189,10 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte
 			switch {
 			case errors.As(job.entry.PathError, &ignoreErr):
 				results <- reporter.Report{
-					ReportedPath:  job.entry.ReportedPath,
-					SourcePath:    job.entry.SourcePath,
+					Path: discovery.Path{
+						Name:          job.entry.Path.Name,
+						SymlinkTarget: job.entry.Path.SymlinkTarget,
+					},
 					ModifiedLines: job.entry.ModifiedLines,
 					Problem: checks.Problem{
 						Lines: parser.LineRange{
@@ -205,8 +207,10 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte
 				}
 			case errors.As(job.entry.PathError, &commentErr):
 				results <- reporter.Report{
-					ReportedPath:  job.entry.ReportedPath,
-					SourcePath:    job.entry.SourcePath,
+					Path: discovery.Path{
+						Name:          job.entry.Path.Name,
+						SymlinkTarget: job.entry.Path.SymlinkTarget,
+					},
 					ModifiedLines: job.entry.ModifiedLines,
 					Problem: checks.Problem{
 						Lines: parser.LineRange{
@@ -222,8 +226,10 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte
 			case job.entry.PathError != nil:
 				line, e := tryDecodingYamlError(job.entry.PathError)
 				results <- reporter.Report{
-					ReportedPath:  job.entry.ReportedPath,
-					SourcePath:    job.entry.SourcePath,
+					Path: discovery.Path{
+						Name:          job.entry.Path.Name,
+						SymlinkTarget: job.entry.Path.SymlinkTarget,
+					},
 					ModifiedLines: job.entry.ModifiedLines,
 					Problem: checks.Problem{
 						Lines: parser.LineRange{
@@ -242,8 +248,10 @@ If this file is a template that will be rendered into valid YAML then you can in
 				}
 			case job.entry.Rule.Error.Err != nil:
 				results <- reporter.Report{
-					ReportedPath:  job.entry.ReportedPath,
-					SourcePath:    job.entry.SourcePath,
+					Path: discovery.Path{
+						Name:          job.entry.Path.Name,
+						SymlinkTarget: job.entry.Path.SymlinkTarget,
+					},
 					ModifiedLines: job.entry.ModifiedLines,
 					Rule:          job.entry.Rule,
 					Problem: checks.Problem{
@@ -263,19 +271,21 @@ This usually means that it's missing some required fields.`,
 				if job.entry.State == discovery.Unknown {
 					slog.Warn(
 						"Bug: unknown rule state",
-						slog.String("path", job.entry.ReportedPath),
+						slog.String("path", job.entry.Path.String()),
 						slog.Int("line", job.entry.Rule.Lines.First),
 						slog.String("name", job.entry.Rule.Name()),
 					)
 				}
 
 				start := time.Now()
-				problems := job.check.Check(ctx, job.entry.ReportedPath, job.entry.Rule, job.allEntries)
+				problems := job.check.Check(ctx, job.entry.Path, job.entry.Rule, job.allEntries)
 				checkDuration.WithLabelValues(job.check.Reporter()).Observe(time.Since(start).Seconds())
 				for _, problem := range problems {
 					results <- reporter.Report{
-						ReportedPath:  job.entry.ReportedPath,
-						SourcePath:    job.entry.SourcePath,
+						Path: discovery.Path{
+							Name:          job.entry.Path.Name,
+							SymlinkTarget: job.entry.Path.SymlinkTarget,
+						},
 						ModifiedLines: job.entry.ModifiedLines,
 						Rule:          job.entry.Rule,
 						Problem:       problem,

cmd/pint/tests/0159_ci_teamcity.txt

@@ -10,17 +10,15 @@ env GIT_COMMITTER_EMAIL=pint@example.com
 exec git add .
 exec git commit -am 'import rules and config'
 
-exec git checkout -b v1
+exec git checkout -b pr
 cp ../src/a.yml a.yml
 exec git add a.yml
 exec git commit -am 'v1'
 
-exec git checkout -b v2
 cp ../src/b.yml b.yml
 exec git add b.yml
 exec git commit -am 'v2'
 
-exec git checkout -b v3
 exec git rm a.yml
 exec git commit -am 'v3'
 

cmd/pint/tests/0171_rule_duplicate_rename.txt

@@ -0,0 +1,64 @@
+http response prometheus /api/v1/status/flags 200 {"status":"success","data":{"storage.tsdb.retention.time": "1d"}}
+http response prometheus /api/v1/metadata 200 {"status":"success","data":{}}
+http response prometheus /api/v1/status/config 200 {"status":"success","data":{"yaml":"global:\n  scrape_interval: 1m\n"}}
+http response prometheus /api/v1/query_range 200 {"status":"success","data":{"resultType":"matrix","result":[]}}
+http response prometheus /api/v1/query 200 {"status":"success","data":{"resultType":"vector","result":[]}}
+http start prometheus 127.0.0.1:7171
+
+mkdir testrepo
+cd testrepo
+exec git init --initial-branch=main .
+
+cp ../src/.pint.hcl .
+cp ../src/v1.yaml rules.yaml
+env GIT_AUTHOR_NAME=pint
+env GIT_AUTHOR_EMAIL=pint@example.com
+env GIT_COMMITTER_NAME=pint
+env GIT_COMMITTER_EMAIL=pint@example.com
+exec git add .
+exec git commit -am 'import rules and config'
+
+exec git checkout -b pr
+
+cp ../src/v2.yaml rules.yaml
+exec git commit -am 'v2'
+
+exec git mv rules.yaml renamed.yaml
+exec git commit -am 'v3'
+
+pint.error --no-color ci
+! stdout .
+cmp stderr ../stderr.txt
+
+-- stderr.txt --
+level=INFO msg="Loading configuration file" path=.pint.hcl
+level=INFO msg="Finding all rules to check on current git branch" base=main
+level=INFO msg="Configured new Prometheus server" name=prom uris=1 uptime=up tags=[] include=["^.*.yaml$"] exclude=[]
+level=WARN msg="No results for Prometheus uptime metric, you might have set uptime config option to a missing metric, please check your config" name=prom metric=up
+level=WARN msg="Using dummy Prometheus uptime metric results with no gaps" name=prom metric=up
+level=INFO msg="Problems found" Bug=1
+renamed.yaml:2 Bug: `prom` Prometheus server at http://127.0.0.1:7171 didn't have any series for `foo` metric in the last 1w. (promql/series)
+ 2 |   expr: sum(foo)
+
+level=ERROR msg="Fatal error" err="problems found"
+-- src/v1.yaml --
+- record: rule1
+  # pint disable promql/series
+  expr: sum(foo)
+-- src/v2.yaml --
+- record: rule1
+  expr: sum(foo)
+-- src/.pint.hcl --
+ci {
+  baseBranch = "main"
+}
+parser {
+  relaxed = [".*"]
+}
+prometheus "prom" {
+  uri      = "http://127.0.0.1:7171"
+  failover = []
+  timeout  = "5s"
+  required = true
+  include  = [".*.yaml"]
+}

cmd/pint/tests/0172_rule_dependency_symlink_delete.txt

@@ -0,0 +1,56 @@
+mkdir testrepo
+cd testrepo
+exec git init --initial-branch=main .
+
+mkdir rules1 rules2
+cp ../src/alert.yml rules1/alert.yml
+cp ../src/record.yml rules1/record.yml
+exec ln -s ../rules1/alert.yml rules2/alert.yml
+exec ln -s ../rules1/record.yml rules2/record.yml
+cp ../src/.pint.hcl .
+env GIT_AUTHOR_NAME=pint
+env GIT_AUTHOR_EMAIL=pint@example.com
+env GIT_COMMITTER_NAME=pint
+env GIT_COMMITTER_EMAIL=pint@example.com
+exec git add .
+exec git commit -am 'import rules and config'
+
+exec git checkout -b v2
+exec git rm -fr rules2
+exec git commit -am 'v2'
+
+pint.ok -l error --offline --no-color ci
+! stdout .
+cmp stderr ../stderr.txt
+
+-- stderr.txt --
+-- src/alert.yml --
+groups:
+- name: g1
+  rules:
+  - alert: Alert
+    expr: 'up:sum == 0'
+    annotations:
+      summary: 'Service is down'
+-- src/record.yml --
+groups:
+- name: g1
+  rules:
+  - record: up:sum
+    expr: sum(up)
+-- src/.pint.hcl --
+ci {
+  baseBranch = "main"
+}
+prometheus "prom1" {
+  uri      = "http://127.0.0.1:7172/1"
+  timeout  = "5s"
+  required = true
+  include  = ["rules1/.*"]
+}
+prometheus "prom2" {
+  uri      = "http://127.0.0.1:7172/2"
+  timeout  = "5s"
+  required = true
+  include  = ["rules2/.*"]
+}

cmd/pint/watch.go

@@ -326,7 +326,7 @@ func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool
 	fileOwners := map[string]string{}
 	for _, entry := range entries {
 		if entry.Owner != "" {
-			fileOwners[entry.ReportedPath] = entry.Owner
+			fileOwners[entry.Path.SymlinkTarget] = entry.Owner
 		}
 	}
 	c.fileOwners = fileOwners
@@ -377,7 +377,7 @@ func (c *problemCollector) Collect(ch chan<- prometheus.Metric) {
 			c.problem,
 			prometheus.GaugeValue,
 			1,
-			report.SourcePath,
+			report.Path.Name,
 			kind,
 			name,
 			strings.ToLower(report.Problem.Severity.String()),

docs/changelog.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## v0.58.0
+
+### Fixed
+
+- Fixed false positives from [rule/duplicate](checks/rule/duplicate.md) check
+  when running `pint ci` on files that are both edited and renamed in the same PR.
+- [rule/dependency](checks/rule/dependency.md) will no longer report removed symlinks.
+
 ## v0.57.3
 
 ### Fixed

internal/checks/alerts_annotation.go

@@ -61,7 +61,7 @@ func (c AnnotationCheck) Reporter() string {
 	return AnnotationCheckName
 }
 
-func (c AnnotationCheck) Check(_ context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c AnnotationCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	if rule.AlertingRule == nil {
 		return nil
 	}

internal/checks/alerts_comparison.go

@@ -45,7 +45,7 @@ func (c ComparisonCheck) Reporter() string {
 	return ComparisonCheckName
 }
 
-func (c ComparisonCheck) Check(_ context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c ComparisonCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	if rule.AlertingRule == nil {
 		return problems
 	}

internal/checks/alerts_count.go

@@ -61,7 +61,7 @@ func (c AlertsCheck) Reporter() string {
 	return AlertsCheckName
 }
 
-func (c AlertsCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c AlertsCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	if rule.AlertingRule == nil {
 		return problems
 	}

internal/checks/alerts_external_labels.go

@@ -43,7 +43,7 @@ func (c AlertsExternalLabelsCheck) Reporter() string {
 	return AlertsExternalLabelsCheckName
 }
 
-func (c AlertsExternalLabelsCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c AlertsExternalLabelsCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	if rule.AlertingRule == nil {
 		return problems
 	}

internal/checks/alerts_for.go

@@ -41,7 +41,7 @@ func (c AlertsForChecksFor) Reporter() string {
 	return AlertForCheckName
 }
 
-func (c AlertsForChecksFor) Check(_ context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c AlertsForChecksFor) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	if rule.AlertingRule == nil {
 		return problems
 	}

internal/checks/alerts_template.go

@@ -110,7 +110,7 @@ func (c TemplateCheck) Reporter() string {
 	return TemplateCheckName
 }
 
-func (c TemplateCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
+func (c TemplateCheck) Check(ctx context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) {
 	if rule.AlertingRule == nil {
 		return nil
 	}

internal/checks/base.go

@@ -123,7 +123,7 @@ type RuleChecker interface {
 	String() string
 	Reporter() string
 	Meta() CheckMeta
-	Check(_ context.Context, _ string, rule parser.Rule, _ []discovery.Entry) []Problem
+	Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) []Problem
 }
 
 type exprProblem struct {

internal/checks/base_test.go

@@ -143,7 +143,7 @@ func runTests(t *testing.T, testCases []checkTest) {
 				if tc.ctx != nil {
 					ctx = tc.ctx()
 				}
-				problems := tc.checker(prom).Check(ctx, entry.SourcePath, entry.Rule, tc.entries)
+				problems := tc.checker(prom).Check(ctx, entry.Path, entry.Rule, tc.entries)
 				require.Equal(t, tc.problems(uri), problems)
 			}
 
@@ -166,7 +166,7 @@ func runTests(t *testing.T, testCases []checkTest) {
 		require.NoError(t, err, "cannot parse rule content")
 		t.Run(tc.description+" (bogus rules)", func(_ *testing.T) {
 			for _, entry := range entries {
-				_ = tc.checker(newSimpleProm("prom")).Check(context.Background(), entry.SourcePath, entry.Rule, tc.entries)
+				_ = tc.checker(newSimpleProm("prom")).Check(context.Background(), entry.Path, entry.Rule, tc.entries)
 			}
 		})
 	}
@@ -181,8 +181,10 @@ func parseContent(content string) (entries []discovery.Entry, err error) {
 
 	for _, rule := range rules {
 		entries = append(entries, discovery.Entry{
-			SourcePath:    "fake.yml",
-			ReportedPath:  "fake.yml",
+			Path: discovery.Path{
+				Name:          "fake.yml",
+				SymlinkTarget: "fake.yml",
+			},
 			ModifiedLines: rule.Lines.Expand(),
 			Rule:          rule,
 		})