From 37a97b613478bc5276b5fbdd6cbfacc7e3a6a456 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Mon, 27 Nov 2023 18:18:44 +0000 Subject: [PATCH] Refactor comment parsing --- cmd/pint/tests/0111_snooze.txt | 2 +- cmd/pint/tests/0116_file_snooze.txt | 4 +- docs/changelog.md | 2 + docs/index.md | 25 ++ internal/checks/promql_series.go | 46 +-- internal/comments/comments.go | 311 ++++++++++++++++++++ internal/comments/comments_test.go | 422 ++++++++++++++++++++++++++++ internal/config/rule.go | 63 ++--- internal/discovery/discovery.go | 66 +++-- internal/discovery/git_branch.go | 2 +- internal/parser/models.go | 223 +++++++-------- internal/parser/models_test.go | 180 ++++++++++++ internal/parser/parser.go | 25 +- internal/parser/parser_test.go | 221 +++++++++++++-- internal/parser/read.go | 135 +++------ internal/parser/read_test.go | 160 +++-------- 16 files changed, 1428 insertions(+), 459 deletions(-) create mode 100644 internal/comments/comments.go create mode 100644 internal/comments/comments_test.go create mode 100644 internal/parser/models_test.go diff --git a/cmd/pint/tests/0111_snooze.txt b/cmd/pint/tests/0111_snooze.txt index 17af2040..7d540945 100644 --- a/cmd/pint/tests/0111_snooze.txt +++ b/cmd/pint/tests/0111_snooze.txt @@ -8,7 +8,7 @@ level=INFO msg="Finding all rules to check" paths=["rules"] level=DEBUG msg="File parsed" path=rules/0001.yml rules=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) comment="snooze 2099-11-28T10:24:18Z promql/aggregate" until="2099-11-28T10:24:18Z" snooze=promql/aggregate +level=DEBUG msg="Check snoozed by comment" check=promql/aggregate(job:true) match=promql/aggregate until="2099-11-28T10:24:18Z" 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=sum-job -- rules/0001.yml -- # pint snooze 2099-11-28T10:24:18Z promql/aggregate diff --git a/cmd/pint/tests/0116_file_snooze.txt b/cmd/pint/tests/0116_file_snooze.txt index b643b322..99d7fd5f 100644 --- a/cmd/pint/tests/0116_file_snooze.txt +++ b/cmd/pint/tests/0116_file_snooze.txt @@ -5,8 +5,8 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check" paths=["rules"] -level=DEBUG msg="Check snoozed by comment" check=promql/aggregate(job:true) comment="file/snooze 2099-11-28T10:24:18Z promql/aggregate(job:true)" until="2099-11-28T10:24:18Z" snooze=promql/aggregate(job:true) -level=DEBUG msg="Check snoozed by comment" check=alerts/for comment="file/snooze 2099-11-28T10:24:18Z alerts/for" until="2099-11-28T10:24:18Z" snooze=alerts/for +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="Generated all Prometheus servers" count=0 level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=4-5 diff --git a/docs/changelog.md b/docs/changelog.md index fa526acd..86e8faa6 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,6 +10,8 @@ rules. - pint will now try to create fewer BitBucket comments by merging multiple problem reports into a single comment. +- Control comment handling code was refactored, there are some additional rules + that comment must follow. See `Control comments` section in [pint docs](./index.md). ## v0.49.2 diff --git a/docs/index.md b/docs/index.md index e6111b2d..7abd87ef 100644 --- a/docs/index.md +++ b/docs/index.md @@ -209,6 +209,31 @@ Here's an example alert you can use for problems detected by pint: {% endraw %} +## Control comments + +There is a number of comments you can add to your rule files in order to change +pint behaviour, some of them allow you to exclude selected files or line, see +[docs here](./ignoring.md) for details. + +There are a few requirement for any comment to be recognized by pint: + +- All comments must have a `pint` prefix. +- All comments must have at least one space between `#` symbol and `pint` prefix. + +Good comment examples: + +```yaml +# pint file/owner bob +# pint file/owner bob +``` + +Bad comment examples: + +```yaml +# file/owner bob +#pint file/owner bob +``` + ## Release Notes See [changelog](changelog.md) for history of changes. diff --git a/internal/checks/promql_series.go b/internal/checks/promql_series.go index 98a2c00b..8e1e5d61 100644 --- a/internal/checks/promql_series.go +++ b/internal/checks/promql_series.go @@ -10,6 +10,7 @@ import ( "golang.org/x/exp/slices" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/output" "github.com/cloudflare/pint/internal/parser" @@ -615,18 +616,22 @@ func (c SeriesCheck) instantSeriesCount(ctx context.Context, query string) (int, func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelector) (minAge time.Duration, problems []Problem) { minAge = time.Hour * 2 - bareSelector := stripLabels(selector) - for _, s := range [][]string{ - {"rule/set", c.Reporter(), "min-age"}, - {"rule/set", fmt.Sprintf("%s(%s)", c.Reporter(), bareSelector.String()), "min-age"}, - {"rule/set", fmt.Sprintf("%s(%s)", c.Reporter(), selector.String()), "min-age"}, - } { - if cmt, ok := rule.GetComment(s...); ok { - dur, err := model.ParseDuration(cmt.Value) + prefixes := []string{ + fmt.Sprintf("%s min-age ", c.Reporter()), + fmt.Sprintf("%s(%s) min-age ", c.Reporter(), bareSelector.String()), + fmt.Sprintf("%s(%s) min-age ", c.Reporter(), selector.String()), + } + for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) { + for _, prefix := range prefixes { + if !strings.HasPrefix(ruleSet.Value, prefix) { + continue + } + val := strings.TrimPrefix(ruleSet.Value, prefix) + dur, err := model.ParseDuration(val) if err != nil { problems = append(problems, Problem{ - Fragment: cmt.String(), + Fragment: fmt.Sprintf("%s %s", comments.RuleSetComment, ruleSet.Value), Lines: rule.LineRange(), Reporter: c.Reporter(), Text: fmt.Sprintf("Failed to parse pint comment as duration: %s", err), @@ -644,13 +649,16 @@ func (c SeriesCheck) getMinAge(rule parser.Rule, selector promParser.VectorSelec func (c SeriesCheck) isLabelValueIgnored(rule parser.Rule, selector promParser.VectorSelector, labelName string) bool { bareSelector := stripLabels(selector) - for _, s := range []string{ - fmt.Sprintf("rule/set %s ignore/label-value %s", c.Reporter(), labelName), - fmt.Sprintf("rule/set %s(%s) ignore/label-value %s", c.Reporter(), bareSelector.String(), labelName), - fmt.Sprintf("rule/set %s(%s) ignore/label-value %s", c.Reporter(), selector.String(), labelName), - } { - if rule.HasComment(s) { - return true + values := []string{ + fmt.Sprintf("%s ignore/label-value %s", c.Reporter(), labelName), + fmt.Sprintf("%s(%s) ignore/label-value %s", c.Reporter(), bareSelector.String(), labelName), + fmt.Sprintf("%s(%s) ignore/label-value %s", c.Reporter(), selector.String(), labelName), + } + for _, ruleSet := range comments.Only[comments.RuleSet](rule.Comments, comments.RuleSetType) { + for _, val := range values { + if ruleSet.Value == val { + return true + } } } return false @@ -704,9 +712,9 @@ func stripLabels(selector promParser.VectorSelector) promParser.VectorSelector { } func isDisabled(rule parser.Rule, selector promParser.VectorSelector) bool { - for _, c := range rule.GetComments("disable") { - if strings.HasPrefix(c.Value, SeriesCheckName+"(") && strings.HasSuffix(c.Value, ")") { - cs := strings.TrimSuffix(strings.TrimPrefix(c.Value, SeriesCheckName+"("), ")") + for _, disable := range comments.Only[comments.Disable](rule.Comments, comments.DisableType) { + if strings.HasPrefix(disable.Match, SeriesCheckName+"(") && strings.HasSuffix(disable.Match, ")") { + cs := strings.TrimSuffix(strings.TrimPrefix(disable.Match, SeriesCheckName+"("), ")") // try full string or name match first if cs == selector.String() || cs == selector.Name { return true diff --git a/internal/comments/comments.go b/internal/comments/comments.go new file mode 100644 index 00000000..a7b9319c --- /dev/null +++ b/internal/comments/comments.go @@ -0,0 +1,311 @@ +package comments + +import ( + "bufio" + "fmt" + "strings" + "time" + "unicode" +) + +type Type uint8 + +const ( + UnknownType Type = iota + InvalidComment + IgnoreFileType // ignore/file + IgnoreLineType // ignore/line + IgnoreBeginType // ignore/begin + IgnoreEndType // ignore/end + IgnoreNextLineType // ignore/next-line + FileOwnerType // file/owner + RuleOwnerType // rule/owner + FileDisableType // file/disable + DisableType // disable + FileSnoozeType // file/snooze + SnoozeType // snooze + RuleSetType // rule/set +) + +var ( + Prefix = "pint" + + IgnoreFileComment = "ignore/file" + IgnoreLineComment = "ignore/line" + IgnoreBeginComment = "ignore/begin" + IgnoreEndComment = "ignore/end" + IgnoreNextLineComment = "ignore/next-line" + FileOwnerComment = "file/owner" + RuleOwnerComment = "rule/owner" + FileDisableComment = "file/disable" + DisableComment = "disable" + FileSnoozeComment = "file/snooze" + SnoozeComment = "snooze" + RuleSetComment = "rule/set" + + EmptyComment Comment +) + +type CommentValue interface { + String() string +} + +type Comment struct { + Value CommentValue + Type Type +} + +func parseType(s string) Type { + switch s { + case IgnoreFileComment: + return IgnoreFileType + case IgnoreLineComment: + return IgnoreLineType + case IgnoreBeginComment: + return IgnoreBeginType + case IgnoreEndComment: + return IgnoreEndType + case IgnoreNextLineComment: + return IgnoreNextLineType + case FileOwnerComment: + return FileOwnerType + case RuleOwnerComment: + return RuleOwnerType + case FileDisableComment: + return FileDisableType + case DisableComment: + return DisableType + case FileSnoozeComment: + return FileSnoozeType + case SnoozeComment: + return SnoozeType + case RuleSetComment: + return RuleSetType + default: + return UnknownType + } +} + +type Invalid struct { + Err error +} + +func (i Invalid) String() string { + return i.Err.Error() +} + +type Owner struct { + Name string +} + +func (o Owner) String() string { + return o.Name +} + +type Disable struct { + Match string +} + +func (d Disable) String() string { + return d.Match +} + +type Snooze struct { + Until time.Time + Match string +} + +func (s Snooze) String() string { + return fmt.Sprintf("%s %s", s.Until.Format(time.RFC3339), s.Match) +} + +type RuleSet struct { + Value string +} + +func (r RuleSet) String() string { + return r.Value +} + +func parseSnooze(s string) (snz Snooze, err error) { + parts := strings.SplitN(s, " ", 2) + if len(parts) != 2 { + return Snooze{}, fmt.Errorf("invalid snooze comment, expected '$TIME $MATCH' got %q", s) + } + + snz = Snooze{Match: parts[1]} + snz.Until, err = time.Parse(time.RFC3339, parts[0]) + if err != nil { + snz.Until, err = time.Parse("2006-01-02", parts[0]) + } + if err != nil { + return snz, fmt.Errorf("invalid snooze timestamp: %w", err) + } + return snz, nil +} + +func parseValue(typ Type, s string) (CommentValue, error) { + switch typ { + case IgnoreFileType, IgnoreLineType, IgnoreBeginType, IgnoreEndType, IgnoreNextLineType: + if s != "" { + return nil, fmt.Errorf("unexpected comment suffix: %q", s) + } + case FileOwnerType: + if s == "" { + return nil, fmt.Errorf("missing %s value", FileOwnerComment) + } + return Owner{Name: s}, nil + case RuleOwnerType: + if s == "" { + return nil, fmt.Errorf("missing %s value", RuleOwnerComment) + } + return Owner{Name: s}, nil + case FileDisableType: + if s == "" { + return nil, fmt.Errorf("missing %s value", FileDisableComment) + } + return Disable{Match: s}, nil + case DisableType: + if s == "" { + return nil, fmt.Errorf("missing %s value", DisableComment) + } + return Disable{Match: s}, nil + case FileSnoozeType: + if s == "" { + return nil, fmt.Errorf("missing %s value", FileSnoozeComment) + } + return parseSnooze(s) + case SnoozeType: + if s == "" { + return nil, fmt.Errorf("missing %s value", SnoozeComment) + } + return parseSnooze(s) + case RuleSetType: + if s == "" { + return nil, fmt.Errorf("missing %s value", RuleSetComment) + } + return RuleSet{Value: s}, nil + case UnknownType, InvalidComment: + // pass + } + return nil, nil +} + +const ( + needsPrefix int = iota + readsPrefix + needsType + readsType + needsValue + readsValue +) + +func parseComment(s string) (c Comment, err error) { + var buf strings.Builder + + state := needsPrefix + for _, r := range s + "\n" { + READRUNE: + switch state { + case needsPrefix: + if unicode.IsSpace(r) { + goto NEXT + } + state = readsPrefix + goto READRUNE + case readsPrefix: + if unicode.IsLetter(r) { + _, _ = buf.WriteRune(r) + goto NEXT + } + if unicode.IsSpace(r) { + // Invalid comment prefix, ignore this comment. + if buf.String() != Prefix { + return EmptyComment, nil + } + buf.Reset() + state = needsType + goto NEXT + } + // Invalid character in the prefix, ignore this comment. + return EmptyComment, nil + case needsType: + if unicode.IsSpace(r) { + goto NEXT + } + state = readsType + goto READRUNE + case readsType: + if unicode.IsLetter(r) || r == '/' || r == '-' { + _, _ = buf.WriteRune(r) + goto NEXT + } + if unicode.IsSpace(r) || r == '\n' { + c.Type = parseType(buf.String()) + buf.Reset() + state = needsValue + goto NEXT + } + // Invalid character in the type, ignore this comment. + return EmptyComment, nil + case needsValue: + if unicode.IsSpace(r) { + goto NEXT + } + state = readsValue + goto READRUNE + case readsValue: + if r == '\n' { + goto NEXT + } + _, _ = buf.WriteRune(r) + } + NEXT: + } + + c.Value, err = parseValue(c.Type, strings.TrimSpace(buf.String())) + return c, err +} + +func Parse(text string) (comments []Comment) { + sc := bufio.NewScanner(strings.NewReader(text)) + for sc.Scan() { + elems := strings.SplitN(sc.Text(), "# ", 2) + if len(elems) != 2 { + continue + } + c, err := parseComment(elems[1]) + switch { + case err != nil: + comments = append(comments, Comment{ + Type: InvalidComment, + Value: Invalid{Err: err}, + }) + case c == EmptyComment: + // pass + default: + comments = append(comments, c) + } + } + return comments +} + +func Only[T any](src []Comment, typ Type) []T { + dst := make([]T, 0, len(src)) + for _, c := range src { + if c.Type != typ { + continue + } + dst = append(dst, c.Value.(T)) + } + return dst +} + +func IsRuleComment(typ Type) bool { + // nolint:exhaustive + switch typ { + case RuleOwnerType, DisableType, SnoozeType, RuleSetType: + return true + } + return false +} diff --git a/internal/comments/comments_test.go b/internal/comments/comments_test.go new file mode 100644 index 00000000..b3ae07e8 --- /dev/null +++ b/internal/comments/comments_test.go @@ -0,0 +1,422 @@ +package comments_test + +import ( + "errors" + "fmt" + "testing" + "time" + + "github.com/cloudflare/pint/internal/comments" + + "github.com/stretchr/testify/require" +) + +func TestParse(t *testing.T) { + type testCaseT struct { + input string + output []comments.Comment + } + + parseUntil := func(s string) time.Time { + until, err := time.Parse(time.RFC3339, s) + require.NoError(t, err) + return until + } + + errUntil := func(s string) error { + _, err := time.Parse("2006-01-02", s) + require.Error(t, err) + return err + } + + testCases := []testCaseT{ + { + input: "code\n", + }, + { + input: "code # bob\n", + }, + { + input: "code # bob\ncode # alice\n", + }, + { + input: "# pint bamboozle me this", + }, + { + input: "# pint/xxx bamboozle me this", + }, + { + input: "# pint bambo[]ozle me this", + }, + { + input: "# pint ignore/file \t this file", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`unexpected comment suffix: "this file"`)}, + }, + }, + }, + { + input: "# pint ignore/file", + output: []comments.Comment{ + {Type: comments.IgnoreFileType}, + }, + }, + { + input: "# pint ignore/line this line", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`unexpected comment suffix: "this line"`)}, + }, + }, + }, + { + input: "# pint ignore/line", + output: []comments.Comment{ + {Type: comments.IgnoreLineType}, + }, + }, + { + input: "# pint ignore/begin here", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`unexpected comment suffix: "here"`)}, + }, + }, + }, + { + input: "# pint ignore/begin", + output: []comments.Comment{ + {Type: comments.IgnoreBeginType}, + }, + }, + { + input: "# pint ignore/end here", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`unexpected comment suffix: "here"`)}, + }, + }, + }, + { + input: "# pint ignore/end", + output: []comments.Comment{ + {Type: comments.IgnoreEndType}, + }, + }, + { + input: "# pint ignore/next-line\there", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`unexpected comment suffix: "here"`)}, + }, + }, + }, + { + input: "# pint ignore/next-line", + output: []comments.Comment{ + {Type: comments.IgnoreNextLineType}, + }, + }, + { + input: "# pint file/owner", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing file/owner value")}, + }, + }, + }, + { + input: "# pint file/owner bob and alice", + output: []comments.Comment{ + { + Type: comments.FileOwnerType, + Value: comments.Owner{Name: "bob and alice"}, + }, + }, + }, + { + input: "# pint rule/owner", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing rule/owner value")}, + }, + }, + }, + { + input: "# pint rule/owner bob and alice", + output: []comments.Comment{ + { + Type: comments.RuleOwnerType, + Value: comments.Owner{Name: "bob and alice"}, + }, + }, + }, + { + input: "# pint file/disable", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing file/disable value")}, + }, + }, + }, + { + input: `# pint file/disable promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.FileDisableType, + Value: comments.Disable{Match: `promql/series(http_errors_total{label="this has spaces"})`}, + }, + }, + }, + { + input: "# pint disable", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing disable value")}, + }, + }, + }, + { + input: `# pint disable promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: `promql/series(http_errors_total{label="this has spaces"})`}, + }, + }, + }, + { + input: `# pint disable promql/series(http_errors_total{label="this has spaces and a # symbol"})`, + output: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: `promql/series(http_errors_total{label="this has spaces and a # symbol"})`}, + }, + }, + }, + { + input: "# pint file/snooze", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing file/snooze value")}, + }, + }, + }, + { + input: "# pint file/snooze 2023-12-31", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`invalid snooze comment, expected '$TIME $MATCH' got "2023-12-31"`)}, + }, + }, + }, + { + input: "# pint file/snooze abc", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`invalid snooze comment, expected '$TIME $MATCH' got "abc"`)}, + }, + }, + }, + { + input: `# pint file/snooze 2023-1231 promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("invalid snooze timestamp: %w", errUntil("2023-1231"))}, + }, + }, + }, + { + input: `# pint file/snooze 2023-12-31 promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.FileSnoozeType, + Value: comments.Snooze{ + Until: parseUntil("2023-12-31T00:00:00Z"), + Match: `promql/series(http_errors_total{label="this has spaces"})`, + }, + }, + }, + }, + { + input: "# pint snooze", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing snooze value")}, + }, + }, + }, + { + input: "# pint snooze 2023-12-31", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`invalid snooze comment, expected '$TIME $MATCH' got "2023-12-31"`)}, + }, + }, + }, + { + input: "# pint snooze abc", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf(`invalid snooze comment, expected '$TIME $MATCH' got "abc"`)}, + }, + }, + }, + { + input: `# pint snooze 2023-1231 promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("invalid snooze timestamp: %w", errUntil("2023-1231"))}, + }, + }, + }, + { + input: `# pint snooze 2023-12-31 promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.SnoozeType, + Value: comments.Snooze{ + Until: parseUntil("2023-12-31T00:00:00Z"), + Match: `promql/series(http_errors_total{label="this has spaces"})`, + }, + }, + }, + }, + { + input: `# pint snooze 2023-12-31 promql/series(http_errors_total{label="this has spaces"})`, + output: []comments.Comment{ + { + Type: comments.SnoozeType, + Value: comments.Snooze{ + Until: parseUntil("2023-12-31T00:00:00Z"), + Match: `promql/series(http_errors_total{label="this has spaces"})`, + }, + }, + }, + }, + { + input: "# pint rule/set", + output: []comments.Comment{ + { + Type: comments.InvalidComment, + Value: comments.Invalid{Err: fmt.Errorf("missing rule/set value")}, + }, + }, + }, + { + input: "# pint rule/set bob and alice", + output: []comments.Comment{ + { + Type: comments.RuleSetType, + Value: comments.RuleSet{Value: "bob and alice"}, + }, + }, + }, + { + input: "code # pint disable xxx \ncode # alice\n", + output: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "xxx"}, + }, + }, + }, + { + input: "code # pint disable xxx yyy \n # pint\tfile/owner bob", + output: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "xxx yyy"}, + }, + { + Type: comments.FileOwnerType, + Value: comments.Owner{Name: "bob"}, + }, + }, + }, + { + input: "# pint rule/set promql/series(found) min-age foo", + output: []comments.Comment{ + { + Type: comments.RuleSetType, + Value: comments.RuleSet{Value: "promql/series(found) min-age foo"}, + }, + }, + }, + { + input: "{#- comment #} # pint ignore/line", + output: []comments.Comment{ + { + Type: comments.IgnoreLineType, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.input, func(t *testing.T) { + output := comments.Parse(tc.input) + require.Equal(t, tc.output, output) + }) + } +} + +func TestCommentValueString(t *testing.T) { + type testCaseT struct { + comment comments.CommentValue + expected string + } + + parseUntil := func(s string) time.Time { + until, err := time.Parse(time.RFC3339, s) + require.NoError(t, err) + return until + } + + testCases := []testCaseT{ + { + comment: comments.Invalid{Err: errors.New("foo bar")}, + expected: "foo bar", + }, + { + comment: comments.Owner{Name: "bob & alice"}, + expected: "bob & alice", + }, + { + comment: comments.Disable{Match: `promql/series({code="500"})`}, + expected: `promql/series({code="500"})`, + }, + { + comment: comments.RuleSet{Value: "bob & alice"}, + expected: "bob & alice", + }, + { + comment: comments.Snooze{Match: `promql/series({code="500"})`, Until: parseUntil("2023-11-28T00:00:00Z")}, + expected: `2023-11-28T00:00:00Z promql/series({code="500"})`, + }, + } + + for _, tc := range testCases { + t.Run(tc.expected, func(t *testing.T) { + require.Equal(t, tc.expected, tc.comment.String()) + }) + } +} diff --git a/internal/config/rule.go b/internal/config/rule.go index 3caf93ae..8e631d7a 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -7,9 +7,8 @@ import ( "regexp" "time" - "golang.org/x/exp/slices" - "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/promapi" ) @@ -271,51 +270,45 @@ func (rule Rule) resolveChecks(ctx context.Context, path string, r parser.Rule, } func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool { - instance := check.String() - - comments := []string{ - fmt.Sprintf("disable %s", name), - fmt.Sprintf("disable %s", instance), + matches := []string{ + name, + check.String(), } for _, tag := range promTags { - comments = append(comments, fmt.Sprintf("disable %s(+%s)", name, tag)) + matches = append(matches, fmt.Sprintf("%s(+%s)", name, tag)) } - for _, comment := range comments { - if rule.HasComment(comment) { - slog.Debug( - "Check disabled by comment", - slog.String("check", instance), - slog.String("comment", comment), - ) - return false + for _, disable := range comments.Only[comments.Disable](rule.Comments, comments.DisableType) { + for _, match := range matches { + if match == disable.Match { + slog.Debug( + "Check disabled by comment", + slog.String("check", check.String()), + slog.String("match", match), + ) + return false + } } } - - disabled := []string{name, instance} - for _, tag := range promTags { - disabled = append(disabled, fmt.Sprintf("%s(+%s)", name, tag)) - } - for _, comment := range rule.GetComments("snooze") { - s := parser.ParseSnooze(comment.Value) - if s == nil { + for _, snooze := range comments.Only[comments.Snooze](rule.Comments, comments.SnoozeType) { + if !snooze.Until.After(time.Now()) { continue } - if !slices.Contains(disabled, s.Text) { - continue + for _, match := range matches { + if match == snooze.Match { + slog.Debug( + "Check snoozed by comment", + slog.String("check", check.String()), + slog.String("match", snooze.Match), + slog.Time("until", snooze.Until), + ) + return false + } } - slog.Debug( - "Check snoozed by comment", - slog.String("check", instance), - slog.String("comment", comment.String()), - slog.Time("until", s.Until), - slog.String("snooze", s.Text), - ) - return false } for _, c := range disabledChecks { - if c == name || c == instance { + if c == name || c == check.String() { return false } for _, tag := range promTags { diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index 1661e320..9bc8768a 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -7,10 +7,12 @@ import ( "log/slog" "regexp" "strings" + "time" "github.com/prometheus/prometheus/model/rulefmt" "golang.org/x/exp/slices" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/output" "github.com/cloudflare/pint/internal/parser" ) @@ -94,7 +96,7 @@ type Entry struct { func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (entries []Entry, err error) { p := parser.NewParser() - content, err := parser.ReadContent(r) + content, fileComments, err := parser.ReadContent(r) if err != nil { return nil, err } @@ -104,30 +106,34 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent contentLines = append(contentLines, i) } - body := string(content.Body) - fileOwner, _ := parser.GetLastComment(body, FileOwnerComment) - + var fileOwner string var disabledChecks []string - for _, comment := range parser.GetComments(body, FileDisabledCheckComment) { - if !slices.Contains(disabledChecks, comment.Value) { - disabledChecks = append(disabledChecks, comment.Value) - } - } - for _, comment := range parser.GetComments(body, FileSnoozeCheckComment) { - s := parser.ParseSnooze(comment.Value) - if s == nil { - continue - } - if !slices.Contains(disabledChecks, s.Text) { - disabledChecks = append(disabledChecks, s.Text) + for _, comment := range fileComments { + // nolint:exhaustive + switch comment.Type { + case comments.FileOwnerType: + owner := comment.Value.(comments.Owner) + fileOwner = owner.Name + case comments.FileDisableType: + disable := comment.Value.(comments.Disable) + if !slices.Contains(disabledChecks, disable.Match) { + disabledChecks = append(disabledChecks, disable.Match) + } + case comments.FileSnoozeType: + snooze := comment.Value.(comments.Snooze) + if !snooze.Until.After(time.Now()) { + continue + } + if !slices.Contains(disabledChecks, snooze.Match) { + disabledChecks = append(disabledChecks, snooze.Match) + } + slog.Debug( + "Check snoozed by comment", + slog.String("check", snooze.Match), + slog.String("match", snooze.Match), + slog.Time("until", snooze.Until), + ) } - slog.Debug( - "Check snoozed by comment", - slog.String("check", s.Text), - slog.String("comment", comment.String()), - slog.Time("until", s.Until), - slog.String("snooze", s.Text), - ) } if content.Ignored { @@ -135,7 +141,7 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent ReportedPath: reportedPath, SourcePath: sourcePath, PathError: ErrFileIsIgnored, - Owner: fileOwner.Value, + Owner: fileOwner, ModifiedLines: contentLines, }) return entries, nil @@ -156,7 +162,7 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent ReportedPath: reportedPath, SourcePath: sourcePath, PathError: err, - Owner: fileOwner.Value, + Owner: fileOwner, ModifiedLines: contentLines, }) } @@ -178,23 +184,23 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent ReportedPath: reportedPath, SourcePath: sourcePath, PathError: err, - Owner: fileOwner.Value, + Owner: fileOwner, ModifiedLines: contentLines, }) return entries, nil } for _, rule := range rules { - owner, ok := rule.GetComment(RuleOwnerComment) - if !ok { - owner = fileOwner + ruleOwner := fileOwner + for _, owner := range comments.Only[comments.Owner](rule.Comments, comments.RuleOwnerType) { + ruleOwner = owner.Name } entries = append(entries, Entry{ ReportedPath: reportedPath, SourcePath: sourcePath, Rule: rule, ModifiedLines: rule.Lines(), - Owner: owner.Value, + Owner: ruleOwner, DisabledChecks: disabledChecks, }) } diff --git a/internal/discovery/git_branch.go b/internal/discovery/git_branch.go index d45f4c09..3ea8ecf8 100644 --- a/internal/discovery/git_branch.go +++ b/internal/discovery/git_branch.go @@ -250,7 +250,7 @@ func matchEntries(before, after []Entry) (ml []matchedEntry) { for _, b := range before { b := b - if !matched && a.Rule.Name() != "" && a.Rule.ToYAML() == b.Rule.ToYAML() { + if !matched && a.Rule.Name() != "" && a.Rule.IsIdentical(b.Rule) { m.before = &b m.isIdentical = true m.wasMoved = a.SourcePath != b.SourcePath diff --git a/internal/parser/models.go b/internal/parser/models.go index 9440e197..0cd4e8e7 100644 --- a/internal/parser/models.go +++ b/internal/parser/models.go @@ -1,7 +1,6 @@ package parser import ( - "bufio" "fmt" "strings" @@ -9,6 +8,8 @@ import ( "gopkg.in/yaml.v3" promparser "github.com/prometheus/prometheus/promql/parser" + + "github.com/cloudflare/pint/internal/comments" ) func appendLine(lines []int, newLines ...int) []int { @@ -119,6 +120,19 @@ func (ykv YamlKeyValue) Lines() (lines []int) { return lines } +func (ykv *YamlKeyValue) IsIdentical(b *YamlKeyValue) bool { + if (ykv == nil) != (b == nil) { + return false + } + if ykv == nil { + return true + } + if ykv.Value.Value != b.Value.Value { + return false + } + return true +} + type YamlMap struct { Key *YamlNode Items []*YamlKeyValue @@ -132,6 +146,28 @@ func (ym YamlMap) Lines() (lines []int) { return lines } +func (ym *YamlMap) IsIdentical(b *YamlMap) bool { + var al, bl []string + + if ym != nil && ym.Items != nil { + al = make([]string, 0, len(ym.Items)) + for _, kv := range ym.Items { + al = append(al, fmt.Sprintf("%s: %s", kv.Key.Value, kv.Value.Value)) + } + slices.Sort(al) + } + + if b != nil && b.Items != nil { + bl = make([]string, 0, len(b.Items)) + for _, kv := range b.Items { + bl = append(bl, fmt.Sprintf("%s: %s", kv.Key.Value, kv.Value.Value)) + } + slices.Sort(bl) + } + + return slices.Equal(al, bl) +} + func (ym YamlMap) GetValue(key string) *YamlNode { for _, child := range ym.Items { if child.Key.Value == key { @@ -199,6 +235,10 @@ func (pqle PromQLExpr) Lines() (lines []int) { return lines } +func (pqle PromQLExpr) IsIdentical(b PromQLExpr) bool { + return pqle.Value.Value == b.Value.Value +} + func newPromQLExpr(key, val *yaml.Node, offset int) *PromQLExpr { expr := PromQLExpr{ Key: newYamlNode(key, offset), @@ -243,6 +283,34 @@ func (ar AlertingRule) Lines() (lines []int) { return lines } +func (ar *AlertingRule) IsIdentical(b *AlertingRule) bool { + if (ar == nil) != (b == nil) { + return false + } + if ar == nil { + return true + } + if !ar.Alert.IsIdentical(&b.Alert) { + return false + } + if !ar.Expr.IsIdentical(b.Expr) { + return false + } + if !ar.For.IsIdentical(b.For) { + return false + } + if !ar.KeepFiringFor.IsIdentical(b.KeepFiringFor) { + return false + } + if !ar.Labels.IsIdentical(b.Labels) { + return false + } + if !ar.Annotations.IsIdentical(b.Annotations) { + return false + } + return true +} + type RecordingRule struct { Record YamlKeyValue Expr PromQLExpr @@ -259,6 +327,25 @@ func (rr RecordingRule) Lines() (lines []int) { return lines } +func (rr *RecordingRule) IsIdentical(b *RecordingRule) bool { + if (rr == nil) != (b == nil) { + return false + } + if rr == nil { + return true + } + if !rr.Record.IsIdentical(&b.Record) { + return false + } + if !rr.Expr.IsIdentical(b.Expr) { + return false + } + if !rr.Labels.IsIdentical(b.Labels) { + return false + } + return true +} + type ParseError struct { Fragment string Err error @@ -268,103 +355,34 @@ type ParseError struct { type Rule struct { AlertingRule *AlertingRule RecordingRule *RecordingRule - Comments []string + Comments []comments.Comment Error ParseError } -func (r Rule) ToYAML() string { - if r.Error.Err != nil { - return fmt.Sprintf("line=%d fragment=%s err=%s", r.Error.Line, r.Error.Fragment, r.Error.Err) +func (r Rule) IsIdentical(b Rule) bool { + if r.Type() != b.Type() { + return false } - - if r.AlertingRule == nil && r.RecordingRule == nil { - return "" + if !r.AlertingRule.IsIdentical(b.AlertingRule) { + return false + } + if !r.RecordingRule.IsIdentical(b.RecordingRule) { + return false } - var b strings.Builder - + ac := make([]string, 0, len(r.Comments)) for _, c := range r.Comments { - b.WriteString(c) - b.WriteRune('\n') + ac = append(ac, c.Value.String()) } + slices.Sort(ac) - if r.AlertingRule != nil { - b.WriteString("- ") - b.WriteString(r.AlertingRule.Alert.Key.Value) - b.WriteString(": ") - b.WriteString(r.AlertingRule.Alert.Value.Value) - b.WriteRune('\n') - - b.WriteString(" ") - b.WriteString(r.AlertingRule.Expr.Key.Value) - b.WriteString(": ") - b.WriteString(r.AlertingRule.Expr.Value.Value) - b.WriteRune('\n') - - if r.AlertingRule.For != nil { - b.WriteString(" ") - b.WriteString(r.AlertingRule.For.Key.Value) - b.WriteString(": ") - b.WriteString(r.AlertingRule.For.Value.Value) - b.WriteRune('\n') - } - if r.AlertingRule.KeepFiringFor != nil { - b.WriteString(" ") - b.WriteString(r.AlertingRule.KeepFiringFor.Key.Value) - b.WriteString(": ") - b.WriteString(r.AlertingRule.KeepFiringFor.Value.Value) - b.WriteRune('\n') - } - - if r.AlertingRule.Annotations != nil { - b.WriteString(" annotations:\n") - for _, a := range r.AlertingRule.Annotations.Items { - b.WriteString(" ") - b.WriteString(a.Key.Value) - b.WriteString(": ") - b.WriteString(a.Value.Value) - b.WriteRune('\n') - } - } - - if r.AlertingRule.Labels != nil { - b.WriteString(" labels:\n") - for _, l := range r.AlertingRule.Labels.Items { - b.WriteString(" ") - b.WriteString(l.Key.Value) - b.WriteString(": ") - b.WriteString(l.Value.Value) - b.WriteRune('\n') - } - } - - return b.String() - } - - b.WriteString("- ") - b.WriteString(r.RecordingRule.Record.Key.Value) - b.WriteString(": ") - b.WriteString(r.RecordingRule.Record.Value.Value) - b.WriteRune('\n') - - b.WriteString(" ") - b.WriteString(r.RecordingRule.Expr.Key.Value) - b.WriteString(": ") - b.WriteString(r.RecordingRule.Expr.Value.Value) - b.WriteRune('\n') - - if r.RecordingRule.Labels != nil { - b.WriteString(" labels:\n") - for _, l := range r.RecordingRule.Labels.Items { - b.WriteString(" ") - b.WriteString(l.Key.Value) - b.WriteString(": ") - b.WriteString(l.Value.Value) - b.WriteRune('\n') - } + bc := make([]string, 0, len(r.Comments)) + for _, c := range b.Comments { + bc = append(bc, c.Value.String()) } + slices.Sort(bc) - return b.String() + return slices.Equal(ac, bc) } func (r Rule) IsSame(nr Rule) bool { @@ -436,37 +454,6 @@ func (r Rule) LineRange() []int { return lines } -func (r Rule) HasComment(comment string) bool { - for _, c := range r.Comments { - if hasComment(c, comment) { - return true - } - } - return false -} - -func (r Rule) GetComment(comment ...string) (s Comment, ok bool) { - for _, c := range r.Comments { - var val Comment - if val, ok = GetLastComment(c, comment...); ok { - return val, ok - } - } - return s, ok -} - -func (r Rule) GetComments(key string) (cs []Comment) { - for _, c := range r.Comments { - sc := bufio.NewScanner(strings.NewReader(c)) - for sc.Scan() { - if val, ok := GetLastComment(sc.Text(), key); ok { - cs = append(cs, val) - } - } - } - return cs -} - type RuleType string const ( diff --git a/internal/parser/models_test.go b/internal/parser/models_test.go new file mode 100644 index 00000000..4f11defc --- /dev/null +++ b/internal/parser/models_test.go @@ -0,0 +1,180 @@ +package parser_test + +import ( + "strconv" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/cloudflare/pint/internal/parser" +) + +func newMustRule(content string) parser.Rule { + p := parser.NewParser() + rules, err := p.Parse([]byte(content)) + if err != nil { + panic(err) + } + for _, rule := range rules { + return rule + } + return parser.Rule{} +} + +func TestRuleIsIdentical(t *testing.T) { + type testCaseT struct { + a parser.Rule + b parser.Rule + equal bool + } + + testCases := []testCaseT{ + { + a: parser.Rule{}, + b: parser.Rule{}, + equal: true, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- record: foo\n expr: bob\n"), + equal: true, + }, + { + a: newMustRule("- record: bob\n expr: bob\n"), + b: newMustRule("- record: foo\n expr: bob\n"), + equal: false, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- record: foo\n expr: bob\n labels: {}\n"), + equal: true, + }, + { + a: newMustRule("- record: foo\n expr: bob\n labels: {}\n"), + b: newMustRule("- record: foo\n expr: bob\n"), + equal: true, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- expr: bob\n record: foo\n"), + equal: true, + }, + { + a: newMustRule("- record: foo\n expr: bob\n labels:\n foo: bar\n"), + b: newMustRule("- record: foo\n expr: bob\n"), + equal: false, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- record: foo\n expr: bob\n labels:\n foo: bar\n"), + equal: false, + }, + { + a: newMustRule("- record: foo\n expr: bob\n labels:\n bar: bar\n"), + b: newMustRule("- record: foo\n expr: bob\n labels:\n foo: bar\n"), + equal: false, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob\n"), + equal: false, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- record: foo\n xxx: bob\n"), + equal: false, + }, + { + a: newMustRule("- record: foo\n expr: bob\n"), + b: newMustRule("- record: foo\n expr: bob == 0\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob\n for: 4m\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- record: foo\n expr: bob\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob\n"), + equal: true, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob(\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo1\n expr: bob\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob1\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n for: 4m\n"), + b: newMustRule("- alert: foo\n expr: bob\n for: 5m\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n for: 4m\n"), + b: newMustRule("- alert: foo\n expr: bob\n keep_firing_for: 4m\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n keep_firing_for: 4m\n"), + b: newMustRule("- alert: foo\n expr: bob\n for: 4m\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n keep_firing_for: 3m\n"), + b: newMustRule("- alert: foo\n expr: bob\n keep_firing_for: 4m\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob\n labels:\n foo: bar\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n labels:\n bar: bar\n"), + b: newMustRule("- alert: foo\n expr: bob\n labels:\n foo: bar\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob\n annotations:\n foo: bar\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n annotations:\n bar: bar\n"), + b: newMustRule("- alert: foo\n expr: bob\n annotations:\n foo: bar\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n # pint disable promql/series\n expr: bob\n"), + b: newMustRule("- alert: foo\n expr: bob\n"), + equal: false, + }, + { + a: newMustRule("- alert: foo\n expr: bob\n"), + b: newMustRule("- alert: foo\n # pint disable promql/series\n expr: bob\n"), + equal: false, + }, + } + + for i, tc := range testCases { + t.Run(strconv.Itoa(i+1), func(t *testing.T) { + equal := tc.a.IsIdentical(tc.b) + require.Equal(t, tc.equal, equal) + }) + } +} diff --git a/internal/parser/parser.go b/internal/parser/parser.go index e591a692..bef6f693 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -1,10 +1,13 @@ package parser import ( + "errors" "fmt" "strings" "gopkg.in/yaml.v3" + + "github.com/cloudflare/pint/internal/comments" ) const ( @@ -17,6 +20,8 @@ const ( annotationsKey = "annotations" ) +var ErrRuleCommentOnFile = errors.New("this comment is only valid when attached to a rule") + func NewParser() Parser { return Parser{} } @@ -40,7 +45,8 @@ func (p Parser) Parse(content []byte) (rules []Rule, err error) { return nil, err } - return parseNode(content, &node, 0) + rules, err = parseNode(content, &node, 0) + return rules, err } func parseNode(content []byte, node *yaml.Node, offset int) (rules []Rule, err error) { @@ -117,16 +123,25 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool, var key *yaml.Node unknownKeys := []*yaml.Node{} - var comments []string + var ruleComments []comments.Comment for i, part := range unpackNodes(node) { if i == 0 && node.HeadComment != "" && part.HeadComment == "" { part.HeadComment = node.HeadComment } + if i == 0 && node.LineComment != "" && part.LineComment == "" { + part.LineComment = node.LineComment + } if i == len(node.Content)-1 && node.FootComment != "" && part.HeadComment == "" { part.FootComment = node.FootComment } - comments = append(comments, mergeComments(part)...) + for _, s := range mergeComments(part) { + for _, c := range comments.Parse(s) { + if comments.IsRuleComment(c.Type) { + ruleComments = append(ruleComments, c) + } + } + } if i%2 == 0 { key = part @@ -239,7 +254,7 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool, Expr: *exprPart, Labels: labelsPart, }, - Comments: comments, + Comments: ruleComments, } return rule, false, err } @@ -254,7 +269,7 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool, Labels: labelsPart, Annotations: annotationsPart, }, - Comments: comments, + Comments: ruleComments, } return rule, false, err } diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 94cf0033..b381b3df 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -5,6 +5,7 @@ import ( "strconv" "testing" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/parser" "github.com/google/go-cmp/cmp" @@ -239,26 +240,48 @@ func TestParse(t *testing.T) { }, { content: []byte(` -# head comment -- record: foo # record comment - expr: foo offset 10m # expr comment - # pre-labels comment +# pint disable head comment +- record: foo # pint disable record comment + expr: foo offset 10m # pint disable expr comment + # pint disable pre-labels comment labels: - # pre-foo comment + # pint disable pre-foo comment foo: bar - # post-foo comment + # pint disable post-foo comment bob: alice -# foot comment + # pint disable foot comment `), output: []parser.Rule{ { - Comments: []string{ - "# head comment", - "# record comment", - "# expr comment", - "# pre-labels comment", - "# pre-foo comment", - "# post-foo comment", + Comments: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "head comment"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "record comment"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "expr comment"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "pre-labels comment"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "foot comment"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "pre-foo comment"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "post-foo comment"}, + }, }, RecordingRule: &parser.RecordingRule{ Record: parser.YamlKeyValue{ @@ -1012,33 +1035,57 @@ data: content: []byte(`groups: - name: certmanager rules: - - &recordAnchor - record: name1 - expr: expr1 + # pint disable before recordAnchor + - &recordAnchor # pint disable recordAnchor + record: name1 # pint disable name1 + expr: expr1 # pint disable expr1 + # pint disable after expr1 - <<: *recordAnchor expr: expr2 - <<: *recordAnchor `), output: []parser.Rule{ { + Comments: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "before recordAnchor"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "recordAnchor"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "name1"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "after expr1"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "expr1"}, + }, + }, RecordingRule: &parser.RecordingRule{ Record: parser.YamlKeyValue{ Key: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{5}}, + Position: parser.FilePosition{Lines: []int{6}}, Value: "record", }, Value: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{5}}, + Position: parser.FilePosition{Lines: []int{6}}, Value: "name1", }, }, Expr: parser.PromQLExpr{ Key: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{6}}, + Position: parser.FilePosition{Lines: []int{7}}, Value: "expr", }, Value: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{6}}, + Position: parser.FilePosition{Lines: []int{7}}, Value: "expr1", }, Query: &parser.PromQLNode{Expr: "expr1"}, @@ -1046,24 +1093,38 @@ data: }, }, { + Comments: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "before recordAnchor"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "recordAnchor"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "name1"}, + }, + }, RecordingRule: &parser.RecordingRule{ Record: parser.YamlKeyValue{ Key: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{5}}, + Position: parser.FilePosition{Lines: []int{6}}, Value: "record", }, Value: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{5}}, + Position: parser.FilePosition{Lines: []int{6}}, Value: "name1", }, }, Expr: parser.PromQLExpr{ Key: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{8}}, + Position: parser.FilePosition{Lines: []int{10}}, Value: "expr", }, Value: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{8}}, + Position: parser.FilePosition{Lines: []int{10}}, Value: "expr2", }, Query: &parser.PromQLNode{Expr: "expr2"}, @@ -1071,24 +1132,46 @@ data: }, }, { + Comments: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "before recordAnchor"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "recordAnchor"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "name1"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "after expr1"}, + }, + { + Type: comments.DisableType, + Value: comments.Disable{Match: "expr1"}, + }, + }, RecordingRule: &parser.RecordingRule{ Record: parser.YamlKeyValue{ Key: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{5}}, + Position: parser.FilePosition{Lines: []int{6}}, Value: "record", }, Value: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{5}}, + Position: parser.FilePosition{Lines: []int{6}}, Value: "name1", }, }, Expr: parser.PromQLExpr{ Key: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{6}}, + Position: parser.FilePosition{Lines: []int{7}}, Value: "expr", }, Value: &parser.YamlNode{ - Position: parser.FilePosition{Lines: []int{6}}, + Position: parser.FilePosition{Lines: []int{7}}, Value: "expr1", }, Query: &parser.PromQLNode{Expr: "expr1"}, @@ -1109,7 +1192,7 @@ data: - record: name2 expr: expr2 labels: *labelsAnchor - # foot comment + # pint disable foot comment `), output: []parser.Rule{ { @@ -1166,7 +1249,12 @@ data: }, }, { - Comments: []string{"# foot comment"}, + Comments: []comments.Comment{ + { + Type: comments.DisableType, + Value: comments.Disable{Match: "foot comment"}, + }, + }, RecordingRule: &parser.RecordingRule{ Record: parser.YamlKeyValue{ Key: &parser.YamlNode{ @@ -1257,6 +1345,77 @@ data: {Error: parser.ParseError{Err: fmt.Errorf("missing expr key"), Line: 1}}, }, }, + { + content: []byte(string(` +# pint file/owner bob +# pint ignore/begin +# pint ignore/end +# pint disable up + +- record: foo + expr: up + +# pint file/owner alice + +- record: foo + expr: up + +# pint ignore/next-line +`)), + output: []parser.Rule{ + { + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlKeyValue{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{7}}, + Value: "record", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{7}}, + Value: "foo", + }, + }, + Expr: parser.PromQLExpr{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{8}}, + Value: "expr", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{8}}, + Value: "up", + }, + Query: &parser.PromQLNode{Expr: "up"}, + }, + }, + }, + { + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlKeyValue{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{12}}, + Value: "record", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{12}}, + Value: "foo", + }, + }, + Expr: parser.PromQLExpr{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{13}}, + Value: "expr", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{13}}, + Value: "up", + }, + Query: &parser.PromQLNode{Expr: "up"}, + }, + }, + }, + }, + shouldError: false, + }, } alwaysEqual := cmp.Comparer(func(_, _ interface{}) bool { return true }) diff --git a/internal/parser/read.go b/internal/parser/read.go index 321df267..33728076 100644 --- a/internal/parser/read.go +++ b/internal/parser/read.go @@ -5,9 +5,11 @@ import ( "errors" "io" "strings" + + "github.com/cloudflare/pint/internal/comments" ) -type skipMode int +type skipMode uint8 const ( skipNone skipMode = iota @@ -18,10 +20,6 @@ const ( skipFile ) -func removeRedundantSpaces(line string) string { - return strings.Join(strings.Fields(line), " ") -} - func emptyLine(line string) (emptied string) { preComment := strings.TrimSuffix(line, "\n") var comment string @@ -39,45 +37,12 @@ func emptyLine(line string) (emptied string) { return emptied } -func hasComment(line, comment string) bool { - sc := bufio.NewScanner(strings.NewReader(line)) - for sc.Scan() { - elems := strings.Split(sc.Text(), "#") - lastComment := elems[len(elems)-1] - parts := strings.SplitN(removeRedundantSpaces(lastComment), " ", 2) - if len(parts) < 2 { - continue - } - if parts[0] == "pint" && parts[1] == comment { - return true - } - } - return false -} - -func parseSkipComment(line string) (skipMode, bool) { - switch { - case hasComment(line, "ignore/file"): - return skipFile, true - case hasComment(line, "ignore/line"): - return skipCurrentLine, true - case hasComment(line, "ignore/next-line"): - return skipNextLine, true - case hasComment(line, "ignore/begin"): - return skipBegin, true - case hasComment(line, "ignore/end"): - return skipEnd, true - default: - return skipNone, false - } -} - type Content struct { Body []byte Ignored bool } -func ReadContent(r io.Reader) (out Content, err error) { +func ReadContent(r io.Reader) (out Content, fileComments []comments.Comment, err error) { reader := bufio.NewReader(r) var line string var found bool @@ -96,7 +61,44 @@ func ReadContent(r io.Reader) (out Content, err error) { if skipAll { out.Body = append(out.Body, []byte(emptyLine(line))...) } else { - skip, found = parseSkipComment(line) + skip = skipNone + found = false + for _, comment := range comments.Parse(line) { + // nolint:exhaustive + switch comment.Type { + case comments.IgnoreFileType: + skip = skipFile + found = true + case comments.IgnoreLineType: + skip = skipCurrentLine + found = true + case comments.IgnoreBeginType: + skip = skipBegin + found = true + case comments.IgnoreEndType: + skip = skipEnd + found = true + case comments.IgnoreNextLineType: + skip = skipNextLine + found = true + case comments.FileOwnerType: + fileComments = append(fileComments, comment) + case comments.RuleOwnerType: + // pass + case comments.FileDisableType: + fileComments = append(fileComments, comment) + case comments.DisableType: + // pass + case comments.FileSnoozeType: + fileComments = append(fileComments, comment) + case comments.SnoozeType: + // pass + case comments.RuleSetType: + // pass + case comments.InvalidComment: + fileComments = append(fileComments, comment) + } + } switch { case found: switch skip { @@ -145,57 +147,8 @@ func ReadContent(r io.Reader) (out Content, err error) { } if !errors.Is(err, io.EOF) { - return out, err + return out, fileComments, err } - return out, nil -} - -type Comment struct { - Key string - Value string -} - -func (c Comment) String() string { - return c.Key + " " + c.Value -} - -func GetComments(text string, comment ...string) (comments []Comment) { - sc := bufio.NewScanner(strings.NewReader(text)) - for sc.Scan() { - elems := strings.Split(sc.Text(), "#") - lastComment := elems[len(elems)-1] - parts := strings.Split(removeRedundantSpaces(lastComment), " ") - if len(parts) < 2 { - continue - } - keys := make([]string, 0, len(parts)) - values := make([]string, 0, len(parts)) - if parts[0] == "pint" && len(parts) >= len(comment)+1 { - for i, c := range comment { - if parts[i+1] != c { - goto NEXT - } - keys = append(keys, parts[i+1]) - } - for i := len(comment) + 1; i < len(parts); i++ { - values = append(values, parts[i]) - } - comments = append(comments, Comment{ - Key: strings.Join(keys, " "), - Value: strings.Join(values, " "), - }) - } - NEXT: - } - - return comments -} - -func GetLastComment(text string, comment ...string) (Comment, bool) { - comments := GetComments(text, comment...) - if len(comments) == 0 { - return Comment{}, false - } - return comments[len(comments)-1], true + return out, fileComments, nil } diff --git a/internal/parser/read_test.go b/internal/parser/read_test.go index 9457e18a..1fd22ae0 100644 --- a/internal/parser/read_test.go +++ b/internal/parser/read_test.go @@ -2,13 +2,14 @@ package parser_test import ( "bytes" - "fmt" "strconv" "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/parser" ) @@ -16,6 +17,7 @@ func TestReadContent(t *testing.T) { type testCaseT struct { input []byte output []byte + comments []comments.Comment ignored bool shouldError bool } @@ -78,12 +80,12 @@ func TestReadContent(t *testing.T) { output: []byte("# pint ignore/next-line \n \n"), }, { - input: []byte("#pint ignore/next-line \nfoo\n"), - output: []byte("#pint ignore/next-line \n \n"), + input: []byte("# pint ignore/next-line \nfoo\n"), + output: []byte("# pint ignore/next-line \n \n"), }, { - input: []byte("#pintignore/next-line\nfoo\n"), - output: []byte("#pintignore/next-line\nfoo\n"), + input: []byte("# pintignore/next-line\nfoo\n"), + output: []byte("# pintignore/next-line\nfoo\n"), }, { input: []byte("# pint ignore/next-linex\nfoo\n"), @@ -135,12 +137,33 @@ func TestReadContent(t *testing.T) { input: []byte(" {% raw %} # pint ignore/line\n"), output: []byte(" # pint ignore/line\n"), }, + { + input: []byte("# pint file/owner bob\n# pint rule/set xxx\n# pint bamboozle xxx\n"), + output: []byte("# pint file/owner bob\n# pint rule/set xxx\n# pint bamboozle xxx\n"), + comments: []comments.Comment{ + { + Type: comments.FileOwnerType, + Value: comments.Owner{Name: "bob"}, + }, + }, + }, } + cmpErrorText := cmp.Comparer(func(x, y interface{}) bool { + xe := x.(error) + ye := y.(error) + return xe.Error() == ye.Error() + }) + sameErrorText := cmp.FilterValues(func(x, y interface{}) bool { + _, xe := x.(error) + _, ye := y.(error) + return xe && ye + }, cmpErrorText) + for i, tc := range testCases { t.Run(strconv.Itoa(i+1), func(t *testing.T) { r := bytes.NewReader(tc.input) - output, err := parser.ReadContent(r) + output, comments, err := parser.ReadContent(r) hadError := err != nil if hadError != tc.shouldError { @@ -155,128 +178,13 @@ func TestReadContent(t *testing.T) { return } + if diff := cmp.Diff(tc.comments, comments, sameErrorText); diff != "" { + t.Errorf("ReadContent() returned wrong comments (-want +got):\n%s", diff) + return + } + require.Equal(t, string(tc.output), string(output.Body), "ReadContent() returned wrong output") require.Equal(t, tc.ignored, output.Ignored, "ReadContent() returned wrong Ignored value") }) } } - -func TestGetLastComment(t *testing.T) { - type testCaseT struct { - input string - comment []string - output parser.Comment - ok bool - } - - testCases := []testCaseT{ - { - input: "", - comment: []string{"rule/owner"}, - }, - { - input: "\n", - comment: []string{"rule/owner"}, - }, - { - input: "\n \n", - comment: []string{"rule/owner"}, - }, - { - input: "foo bar", - comment: []string{"rule/owner"}, - }, - { - input: "foo bar\n", - comment: []string{"rule/owner"}, - }, - { - input: "line1\nline2", - comment: []string{"rule/owner"}, - }, - { - input: "line1\nline2\n", - comment: []string{"rule/owner"}, - }, - { - input: "line1\n\nline2\n\n", - comment: []string{"rule/owner"}, - }, - { - input: "# pint rule/owner", - comment: []string{"rule/owner"}, - ok: true, - output: parser.Comment{Key: "rule/owner"}, - }, - { - input: "# pint rule/owner foo", - comment: []string{"rule/owner"}, - ok: true, - output: parser.Comment{Key: "rule/owner", Value: "foo"}, - }, - { - input: "# pint rule/owner foo bar bob/alice", - comment: []string{"rule/owner"}, - ok: true, - output: parser.Comment{Key: "rule/owner", Value: "foo bar bob/alice"}, - }, - { - input: "line1\n # pint rule/owner foo bar bob/alice\n line2\n\n", - comment: []string{"rule/owner"}, - ok: true, - output: parser.Comment{Key: "rule/owner", Value: "foo bar bob/alice"}, - }, - { - input: "line1\n #### pint rule/owner foo bar bob/alice\n line2\n\n", - comment: []string{"rule/owner"}, - ok: true, - output: parser.Comment{Key: "rule/owner", Value: "foo bar bob/alice"}, - }, - { - input: "# pint set promql/series min-age 1w", - comment: []string{"set promql/series min-age"}, - }, - { - input: "# pint set promql/series min-age 1w", - comment: []string{"set", "promql/series", "min-age"}, - ok: true, - output: parser.Comment{Key: "set promql/series min-age", Value: "1w"}, - }, - { - input: "# pint set promql/series min-age 1d\n# pint set promql/series min-age 1w\n", - comment: []string{"set", "promql/series", "min-age"}, - ok: true, - output: parser.Comment{Key: "set promql/series min-age", Value: "1w"}, - }, - { - input: "# pint set promql/series min-age 2d\n# pint set promql/series min-age 1w\n# pint set promql/series min-age 1s\n", - comment: []string{"set", "promql/series", "min-age"}, - ok: true, - output: parser.Comment{Key: "set promql/series min-age", Value: "1s"}, - }, - { - input: "# pint set promql/series min-age 1w ", - comment: []string{"set", "promql/series", "min-age"}, - ok: true, - output: parser.Comment{Key: "set promql/series min-age", Value: "1w"}, - }, - { - input: "# pint set", - comment: []string{"set", "promql/series", "min-age"}, - }, - { - input: "# pint rule/set promql/series ignore/label-value error", - comment: []string{"rule/set", "promql/series", "ignore/label-value"}, - ok: true, - output: parser.Comment{Key: "rule/set promql/series ignore/label-value", Value: "error"}, - }, - } - - for i, tc := range testCases { - t.Run(fmt.Sprintf("%d/%s", i, tc.input), func(t *testing.T) { - output, ok := parser.GetLastComment(tc.input, tc.comment...) - require.Equal(t, tc.ok, ok) - require.Equal(t, tc.output, output) - }) - } -}