From a12eda3f1a4780bc1ab1721d9d821f0ea6e74a51 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Thu, 22 Aug 2024 11:58:44 +0100 Subject: [PATCH 1/3] Allow matching on rule state --- cmd/pint/ci.go | 11 + cmd/pint/scan.go | 2 +- cmd/pint/tests/0083_github_action.txt | 1 + cmd/pint/tests/0184_ci_file_ignore.txt | 1 + cmd/pint/tests/0185_state_empty.txt | 45 + cmd/pint/tests/0186_ci_state_any.txt | 54 + docs/changelog.md | 1 + docs/configuration.md | 64 +- .../config/__snapshots__/config_test.snap | 1506 +++++++++-------- internal/config/config.go | 109 +- internal/config/config_test.go | 110 +- internal/config/match.go | 59 +- internal/config/rule.go | 23 + internal/config/rule_test.go | 158 ++ internal/discovery/git_branch.go | 4 - 15 files changed, 1391 insertions(+), 757 deletions(-) create mode 100644 cmd/pint/tests/0185_state_empty.txt create mode 100644 cmd/pint/tests/0186_ci_state_any.txt diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go index dfa7819f..9da26c43 100644 --- a/cmd/pint/ci.go +++ b/cmd/pint/ci.go @@ -96,6 +96,7 @@ func actionCI(c *cli.Context) error { if err != nil { return err } + entries = filterNoopEntries(entries) ctx := context.WithValue(context.Background(), config.CommandKey, config.CICommand) @@ -342,3 +343,13 @@ func detectGithubActions(gh *config.GitHub) *config.GitHub { } return gh } + +func filterNoopEntries(src []discovery.Entry) (dst []discovery.Entry) { + for _, e := range src { + if e.State == discovery.Noop && e.PathError != nil { + continue + } + dst = append(dst, e) + } + return dst +} diff --git a/cmd/pint/scan.go b/cmd/pint/scan.go index 031157ea..466b436e 100644 --- a/cmd/pint/scan.go +++ b/cmd/pint/scan.go @@ -103,7 +103,7 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr } checkedEntriesCount.Inc() - checkList := cfg.GetChecksForRule(ctx, gen, entry, entry.DisabledChecks) + checkList := cfg.GetChecksForRule(ctx, gen, entry) for _, check := range checkList { checkIterationChecks.Inc() if check.Meta().IsOnline { diff --git a/cmd/pint/tests/0083_github_action.txt b/cmd/pint/tests/0083_github_action.txt index a239a956..a55d9a09 100644 --- a/cmd/pint/tests/0083_github_action.txt +++ b/cmd/pint/tests/0083_github_action.txt @@ -61,3 +61,4 @@ groups: -- src/.pint.hcl -- repository {} + diff --git a/cmd/pint/tests/0184_ci_file_ignore.txt b/cmd/pint/tests/0184_ci_file_ignore.txt index 93068a35..01c4dbc2 100644 --- a/cmd/pint/tests/0184_ci_file_ignore.txt +++ b/cmd/pint/tests/0184_ci_file_ignore.txt @@ -23,6 +23,7 @@ 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="No rules found, skipping Prometheus discovery" -- src/rules.yml -- # pint ignore/file - record: rule1 diff --git a/cmd/pint/tests/0185_state_empty.txt b/cmd/pint/tests/0185_state_empty.txt new file mode 100644 index 00000000..03d40fdc --- /dev/null +++ b/cmd/pint/tests/0185_state_empty.txt @@ -0,0 +1,45 @@ +mkdir testrepo +cd testrepo +exec git init --initial-branch=main . + +cp ../src/rules.yml rules.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 touch .keep +exec git add .keep +exec git commit -am 'v2' + +pint.ok --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 +-- src/rules.yml -- +- record: rule1 + expr: sum(foo) by(job) +- record: rule2 + expr: sum(foo) + +-- src/.pint.hcl -- +ci { + baseBranch = "main" +} +parser { + relaxed = [".*"] + include = ["rules.yml"] +} +rule { + aggregate ".+" { + keep = [ "job" ] + severity = "bug" + } +} diff --git a/cmd/pint/tests/0186_ci_state_any.txt b/cmd/pint/tests/0186_ci_state_any.txt new file mode 100644 index 00000000..bd334e34 --- /dev/null +++ b/cmd/pint/tests/0186_ci_state_any.txt @@ -0,0 +1,54 @@ +mkdir testrepo +cd testrepo +exec git init --initial-branch=main . + +cp ../src/rules.yml rules.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 touch .keep +exec git add .keep +exec git commit -am 'v2' + +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="Problems found" Bug=1 +rules.yml:4 Bug: `job` label is required and should be preserved when aggregating `^.+$` rules, use `by(job, ...)`. (promql/aggregate) + 4 | expr: sum(foo) + +level=ERROR msg="Fatal error" err="problems found" +-- src/rules.yml -- +- record: rule1 + expr: sum(foo) by(job) +- record: rule2 + expr: sum(foo) + +-- src/.pint.hcl -- +ci { + baseBranch = "main" +} +parser { + relaxed = [".*"] + include = ["rules.yml"] +} +rule { + match { + kind = "recording" + state = ["any"] + } + aggregate ".+" { + keep = [ "job" ] + severity = "bug" + } +} diff --git a/docs/changelog.md b/docs/changelog.md index 572a64bf..5f33d296 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -9,6 +9,7 @@ - [promql/range_query](checks/promql/range_query.md) now allows to configure a custom maximum duration for range queries - #1064. - Added `--enabled` flag to the pint command. Passing this flag will only run selected check(s). +- Added `state` option to the rule `match` block. See [configuration](configuration.md) docs for details. ### Fixed diff --git a/docs/configuration.md b/docs/configuration.md index 2e9cb580..7ccbd8b7 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -80,10 +80,10 @@ parser { ```yaml groups: - - name: example - rules: - - record: ... - expr: ... + - name: example + rules: + - record: ... + expr: ... ``` If you're using pint to lint rules that are embedded inside a different structure @@ -519,9 +519,10 @@ Syntax: ```js rule { match { - path = "(.+)" - name = "(.+)" - kind = "alerting|recording" + path = "(.+)" + state = [ "any|added|modified|renamed|unmodified", ... ] + name = "(.+)" + kind = "alerting|recording" command = "ci|lint|watch" annotation "(.*)" { value = "(.*)" @@ -534,9 +535,10 @@ rule { match { ... } match { ... } ignore { - path = "(.+)" - name = "(.+)" - kind = "alerting|recording" + path = "(.+)" + state = [ "any|added|modified|renamed|unmodified", ... ] + name = "(.+)" + kind = "alerting|recording" command = "ci|lint|watch" annotation "(.*)" { value = "(.*)" @@ -555,10 +557,19 @@ rule { } ``` -- `match:path` - only files matching this pattern will be checked by this rule +- `match:path` - only files matching this pattern will be checked by this rule. +- `match:state` - only match rules based on their state. Default value for `state` depends on the + pint command that is being run, for `pint ci` the default value is `["added", "modified", "renamed"]`, + for any other command the default value is `["any"]`. + Possible values: + - `any` - match rule in any state + - `added` - a rule is being added in a git commit, a rule can only be in this state when running `pint ci` on a pull request. + - `modified` - a rule is being modified in a git commit, a rule can only be in this state when running `pint ci` on a pull request. + - `renamed` - a rule is being modified in a git commit, a rule can only be in this state when running `pint ci` on a pull request. + - `unmodified` - a rule is not being modified in a git commit when running `pint ci` or other pint command was run that doesn't try to detect which rules were modified. - `match:name` - only rules with names (`record` for recording rules and `alert` for alerting - rules) matching this pattern will be checked rule -- `match:kind` - optional rule type filter, only rule of this type will be checked + rules) matching this pattern will be checked rule. +- `match:kind` - optional rule type filter, only rule of this type will be checked. - `match:command` - optional command type filter, this allows to include or ignore rules based on the command pint is run with `pint ci`, `pint lint` or `pint watch`. - `match:annotation` - optional annotation filter, only alert rules with at least one @@ -581,6 +592,8 @@ be matched / ignored. Examples: +Check applied only to severity="critical" and severity="warning" alerts in "ci" or "lint" command is run: + ```js rule { match { @@ -593,10 +606,12 @@ rule { ignore { command = "watch" } - [ check applied only to severity="critical" and severity="warning" alerts in "ci" or "lint" command is run ] + check { ... } } ``` +Check applied unless "watch" or "lint" command is run: + ```js rule { ignore { @@ -605,24 +620,39 @@ rule { ignore { command = "lint" } - [ check applied unless "watch" or "lint" command is run ] + check { ... } } ``` +Check applied only to alerting rules with "for" field value that is >= 5m: + ```js rule { match { for = ">= 5m" } - [ check applied only to alerting rules with "for" field value that is >= 5m ] + check { ... } } ``` +Check applied only to alerting rules with "keep_firing_for" field value that is > 15m: + ```js rule { match { keep_firing_for = "> 15m" } - [ check applied only to alerting rules with "keep_firing_for" field value that is > 15m ] + check { ... } +} +``` + +Check that is run on all rules, including unmodified files, when running `pint ci`: + +```js +rule { + match { + state = ["any"] + } + check { ... } } ``` diff --git a/internal/config/__snapshots__/config_test.snap b/internal/config/__snapshots__/config_test.snap index b27d96ec..2b5f3404 100755 --- a/internal/config/__snapshots__/config_test.snap +++ b/internal/config/__snapshots__/config_test.snap @@ -1,5 +1,5 @@ -[TestGetChecksForRule/two_checks_enabled_via_config - 1] +[TestGetChecksForRule/defaults - 1] { "ci": { "baseBranch": "master", @@ -8,38 +8,38 @@ "parser": {}, "checks": { "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", "promql/syntax", - "alerts/count" + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" ] }, - "owners": {}, - "prometheus": [ - { - "name": "prom1", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - } - ], - "rules": [ - { - "alerts": { - "range": "1h", - "step": "1m", - "resolve": "5m" - } - } - ] + "owners": {} } --- -[TestGetChecksForRule/rule_with_ignore_block_/_mismatch - 1] +[TestGetChecksForRule/single_prometheus_server - 1] { "ci": { "baseBranch": "master", @@ -48,43 +48,49 @@ "parser": {}, "checks": { "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", "promql/syntax", - "alerts/count" + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" ] }, "owners": {}, "prometheus": [ { - "name": "prom1", + "name": "prom", "uri": "http://localhost", "timeout": "1s", "uptime": "up", - "include": [ - "rules.yml" - ], "concurrency": 16, "rateLimit": 100, "required": false } - ], - "rules": [ - { - "ignore": [ - { - "path": "foo.xml" - } - ], - "alerts": { - "range": "1h", - "step": "1m", - "resolve": "5m" - } - } ] } --- -[TestGetChecksForRule/rule_with_ignore_block_/_match - 1] +[TestGetChecksForRule/multiple_URIs - 1] { "ci": { "baseBranch": "master", @@ -93,43 +99,53 @@ "parser": {}, "checks": { "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", "promql/syntax", - "alerts/count" + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" ] }, "owners": {}, "prometheus": [ { - "name": "prom1", + "name": "prom", "uri": "http://localhost", "timeout": "1s", "uptime": "up", - "include": [ - "rules.yml" + "failover": [ + "http://localhost/1", + "http://localhost/2" ], "concurrency": 16, "rateLimit": 100, "required": false } - ], - "rules": [ - { - "ignore": [ - { - "path": "rules.yml" - } - ], - "alerts": { - "range": "1h", - "step": "1m", - "resolve": "5m" - } - } ] } --- -[TestGetChecksForRule/defaults - 1] +[TestGetChecksForRule/two_prometheus_servers_/_disable_all_checks_via_comment - 1] { "ci": { "baseBranch": "master", @@ -163,9 +179,33 @@ "rule/label", "rule/link", "rule/reject" + ], + "disabled": [ + "alerts/template", + "alerts/external_labels" ] }, - "owners": {} + "owners": {}, + "prometheus": [ + { + "name": "prom1", + "uri": "http://localhost/1", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "prom2", + "uri": "http://localhost/2", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ] } --- @@ -334,7 +374,7 @@ } --- -[TestGetChecksForRule/single_empty_rule - 1] +[TestGetChecksForRule/single_prometheus_server_/_path_match - 1] { "ci": { "baseBranch": "master", @@ -371,13 +411,24 @@ ] }, "owners": {}, - "rules": [ - {} + "prometheus": [ + { + "name": "prom", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + } ] } --- -[TestGetChecksForRule/rule_with_aggregate_checks - 1] +[TestGetChecksForRule/multiple_prometheus_servers - 1] { "ci": { "baseBranch": "master", @@ -414,31 +465,36 @@ ] }, "owners": {}, - "rules": [ + "prometheus": [ { - "aggregate": [ - { - "name": ".+", - "severity": "bug", - "keep": [ - "job" - ] - }, - { - "name": ".+", - "severity": "bug", - "strip": [ - "instance", - "rack" - ] - } - ] + "name": "prom", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "ignore", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "foo.+" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false } ] } --- -[TestGetChecksForRule/multiple_checks_and_disable_comment - 1] +[TestGetChecksForRule/single_empty_rule - 1] { "ci": { "baseBranch": "master", @@ -476,26 +532,130 @@ }, "owners": {}, "rules": [ - { - "aggregate": [ - { - "name": ".+", - "severity": "bug", - "keep": [ - "job" - ] - }, - { - "name": ".+", - "comment": "this is rule comment", - "severity": "bug", - "strip": [ - "instance", - "rack" - ] - } - ] - } + {} + ] +} +--- + +[TestGetChecksForRule/rule_with_aggregate_checks - 1] +{ + "ci": { + "baseBranch": "master", + "maxCommits": 20 + }, + "parser": {}, + "checks": { + "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", + "promql/syntax", + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" + ] + }, + "owners": {}, + "rules": [ + { + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "keep": [ + "job" + ] + }, + { + "name": ".+", + "severity": "bug", + "strip": [ + "instance", + "rack" + ] + } + ] + } + ] +} +--- + +[TestGetChecksForRule/multiple_checks_and_disable_comment - 1] +{ + "ci": { + "baseBranch": "master", + "maxCommits": 20 + }, + "parser": {}, + "checks": { + "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", + "promql/syntax", + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" + ] + }, + "owners": {}, + "rules": [ + { + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "keep": [ + "job" + ] + }, + { + "name": ".+", + "comment": "this is rule comment", + "severity": "bug", + "strip": [ + "instance", + "rack" + ] + } + ] + } ] } --- @@ -549,6 +709,77 @@ } --- +[TestGetChecksForRule/prometheus_check_with_prometheus_servers_and_disable_comment - 1] +{ + "ci": { + "baseBranch": "master", + "maxCommits": 20 + }, + "parser": {}, + "checks": { + "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", + "promql/syntax", + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" + ] + }, + "owners": {}, + "prometheus": [ + { + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "prom2", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], + "rules": [ + { + "cost": {} + } + ] +} +--- + [TestGetChecksForRule/duplicated_rules - 1] { "ci": { @@ -647,7 +878,7 @@ } --- -[TestGetChecksForRule/reject_rules - 1] +[TestGetChecksForRule/multiple_cost_checks - 1] { "ci": { "baseBranch": "master", @@ -684,32 +915,56 @@ ] }, "owners": {}, + "prometheus": [ + { + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "prom2", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], "rules": [ { - "reject": [ - { - "key": "http://.+", - "label_keys": true, - "label_values": true - }, - { - "key": ".* +.*", - "comment": "this is rule comment", - "label_keys": true, - "annotation_keys": true - }, - { - "comment": "this is rule comment", - "severity": "bug", - "annotation_values": true - } - ] + "cost": { + "comment": "this is rule comment", + "severity": "info" + } + }, + { + "cost": { + "severity": "warning", + "maxSeries": 10000 + } + }, + { + "cost": { + "severity": "bug", + "maxSeries": 20000 + } } ] } --- -[TestGetChecksForRule/rule_with_label_match_/_type_mismatch - 1] +[TestGetChecksForRule/reject_rules - 1] { "ci": { "baseBranch": "master", @@ -748,21 +1003,22 @@ "owners": {}, "rules": [ { - "match": [ + "reject": [ { - "label": { - "key": "cluster", - "value": "prod" - }, - "kind": "alerting" - } - ], - "label": [ + "key": "http://.+", + "label_keys": true, + "label_values": true + }, { - "key": "priority", - "value": "(1|2|3|4|5)", + "key": ".* +.*", + "comment": "this is rule comment", + "label_keys": true, + "annotation_keys": true + }, + { + "comment": "this is rule comment", "severity": "bug", - "required": true + "annotation_values": true } ] } @@ -770,7 +1026,7 @@ } --- -[TestGetChecksForRule/rule_with_label_match_/_no_label - 1] +[TestGetChecksForRule/rule_with_label_match_/_type_mismatch - 1] { "ci": { "baseBranch": "master", @@ -831,7 +1087,7 @@ } --- -[TestGetChecksForRule/rule_with_label_match_/_label_mismatch - 1] +[TestGetChecksForRule/rule_with_label_match_/_no_label - 1] { "ci": { "baseBranch": "master", @@ -892,7 +1148,7 @@ } --- -[TestGetChecksForRule/rule_with_annotation_match_/_no_annotation - 1] +[TestGetChecksForRule/rule_with_label_match_/_label_mismatch - 1] { "ci": { "baseBranch": "master", @@ -933,7 +1189,7 @@ { "match": [ { - "annotation": { + "label": { "key": "cluster", "value": "prod" }, @@ -953,7 +1209,7 @@ } --- -[TestGetChecksForRule/rule_with_annotation_match_/_annotation_mismatch - 1] +[TestGetChecksForRule/rule_with_label_match_/_label_match - 1] { "ci": { "baseBranch": "master", @@ -994,7 +1250,7 @@ { "match": [ { - "annotation": { + "label": { "key": "cluster", "value": "prod" }, @@ -1014,7 +1270,7 @@ } --- -[TestGetChecksForRule/for_match_/_passing - 1] +[TestGetChecksForRule/rule_with_annotation_match_/_no_annotation - 1] { "ci": { "baseBranch": "master", @@ -1055,12 +1311,18 @@ { "match": [ { - "for": "\u003e 15m" + "annotation": { + "key": "cluster", + "value": "prod" + }, + "kind": "alerting" } ], - "annotation": [ + "label": [ { - "key": "summary", + "key": "priority", + "value": "(1|2|3|4|5)", + "severity": "bug", "required": true } ] @@ -1069,7 +1331,7 @@ } --- -[TestGetChecksForRule/for_match_/_not_passing - 1] +[TestGetChecksForRule/rule_with_annotation_match_/_annotation_mismatch - 1] { "ci": { "baseBranch": "master", @@ -1110,13 +1372,18 @@ { "match": [ { - "for": "\u003e 15m" + "annotation": { + "key": "cluster", + "value": "prod" + }, + "kind": "alerting" } ], - "annotation": [ + "label": [ { - "key": "summary", - "comment": "this is rule comment", + "key": "priority", + "value": "(1|2|3|4|5)", + "severity": "bug", "required": true } ] @@ -1125,7 +1392,7 @@ } --- -[TestGetChecksForRule/for_match_/_passing#01 - 1] +[TestGetChecksForRule/rule_with_annotation_match_/_annotation_match - 1] { "ci": { "baseBranch": "master", @@ -1166,12 +1433,20 @@ { "match": [ { - "keep_firing_for": "\u003e 15m" + "annotation": { + "key": "cluster", + "value": "prod" + }, + "kind": "alerting" } ], - "annotation": [ + "label": [ { - "key": "summary", + "key": "priority", + "token": "\\w+", + "value": "(1|2|3|4|5)", + "comment": "this is rule comment", + "severity": "bug", "required": true } ] @@ -1180,7 +1455,7 @@ } --- -[TestGetChecksForRule/for_match_/_passing#02 - 1] +[TestGetChecksForRule/checks_disabled_via_config - 1] { "ci": { "baseBranch": "master", @@ -1214,83 +1489,80 @@ "rule/label", "rule/link", "rule/reject" + ], + "disabled": [ + "promql/counter", + "promql/rate", + "promql/vector_matching", + "promql/range_query", + "rule/duplicate", + "labels/conflict", + "alerts/absent" ] }, "owners": {}, - "rules": [ + "prometheus": [ { - "match": [ - { - "keep_firing_for": "\u003e 15m" - } + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" ], - "annotation": [ - { - "key": "summary", - "required": true - } - ] + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], + "rules": [ + { + "alerts": { + "range": "1h", + "step": "1m", + "resolve": "5m" + } } ] } --- -[TestGetChecksForRule/for_match_/_passing#03 - 1] +[TestGetChecksForRule/single_check_enabled_via_config - 1] { "ci": { "baseBranch": "master", "maxCommits": 20 }, "parser": {}, - "checks": { - "enabled": [ - "alerts/absent", - "alerts/annotation", - "alerts/count", - "alerts/external_labels", - "alerts/for", - "alerts/template", - "labels/conflict", - "promql/aggregate", - "alerts/comparison", - "promql/fragile", - "promql/range_query", - "promql/rate", - "promql/regexp", - "promql/syntax", - "promql/vector_matching", - "query/cost", - "promql/counter", - "promql/series", - "rule/dependency", - "rule/duplicate", - "rule/for", - "rule/name", - "rule/label", - "rule/link", - "rule/reject" - ] - }, + "checks": {}, "owners": {}, - "rules": [ + "prometheus": [ { - "match": [ - { - "keep_firing_for": "\u003e 15m" - } + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" ], - "annotation": [ - { - "key": "summary", - "required": true - } - ] + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], + "rules": [ + { + "alerts": { + "range": "1h", + "step": "1m", + "resolve": "5m" + } } ] } --- -[TestGetChecksForRule/for_match_/_recording_rules_/_not_passing - 1] +[TestGetChecksForRule/two_checks_enabled_via_config - 1] { "ci": { "baseBranch": "master", @@ -1299,53 +1571,38 @@ "parser": {}, "checks": { "enabled": [ - "alerts/absent", - "alerts/annotation", - "alerts/count", - "alerts/external_labels", - "alerts/for", - "alerts/template", - "labels/conflict", - "promql/aggregate", - "alerts/comparison", - "promql/fragile", - "promql/range_query", - "promql/rate", - "promql/regexp", "promql/syntax", - "promql/vector_matching", - "query/cost", - "promql/counter", - "promql/series", - "rule/dependency", - "rule/duplicate", - "rule/for", - "rule/name", - "rule/label", - "rule/link", - "rule/reject" + "alerts/count" ] }, "owners": {}, - "rules": [ + "prometheus": [ { - "match": [ - { - "for": "!= 15m" - } + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" ], - "annotation": [ - { - "key": "summary", - "required": true - } - ] + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], + "rules": [ + { + "alerts": { + "range": "1h", + "step": "1m", + "resolve": "5m" + } } ] } --- -[TestGetChecksForRule/for_ignore_/_passing - 1] +[TestGetChecksForRule/rule_with_ignore_block_/_mismatch - 1] { "ci": { "baseBranch": "master", @@ -1354,53 +1611,43 @@ "parser": {}, "checks": { "enabled": [ - "alerts/absent", - "alerts/annotation", - "alerts/count", - "alerts/external_labels", - "alerts/for", - "alerts/template", - "labels/conflict", - "promql/aggregate", - "alerts/comparison", - "promql/fragile", - "promql/range_query", - "promql/rate", - "promql/regexp", "promql/syntax", - "promql/vector_matching", - "query/cost", - "promql/counter", - "promql/series", - "rule/dependency", - "rule/duplicate", - "rule/for", - "rule/name", - "rule/label", - "rule/link", - "rule/reject" + "alerts/count" ] }, "owners": {}, + "prometheus": [ + { + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], "rules": [ { "ignore": [ { - "for": "\u003c 15m" + "path": "foo.xml" } ], - "annotation": [ - { - "key": "summary", - "required": true - } - ] + "alerts": { + "range": "1h", + "step": "1m", + "resolve": "5m" + } } ] } --- -[TestGetChecksForRule/for_ignore_/_not_passing - 1] +[TestGetChecksForRule/rule_with_ignore_block_/_match - 1] { "ci": { "baseBranch": "master", @@ -1409,54 +1656,43 @@ "parser": {}, "checks": { "enabled": [ - "alerts/absent", - "alerts/annotation", - "alerts/count", - "alerts/external_labels", - "alerts/for", - "alerts/template", - "labels/conflict", - "promql/aggregate", - "alerts/comparison", - "promql/fragile", - "promql/range_query", - "promql/rate", - "promql/regexp", "promql/syntax", - "promql/vector_matching", - "query/cost", - "promql/counter", - "promql/series", - "rule/dependency", - "rule/duplicate", - "rule/for", - "rule/name", - "rule/label", - "rule/link", - "rule/reject" + "alerts/count" ] }, "owners": {}, + "prometheus": [ + { + "name": "prom1", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "include": [ + "rules.yml" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], "rules": [ { "ignore": [ { - "for": "\u003c 15m" + "path": "rules.yml" } ], - "annotation": [ - { - "key": "summary", - "comment": "this is rule comment", - "required": true - } - ] + "alerts": { + "range": "1h", + "step": "1m", + "resolve": "5m" + } } ] } --- -[TestGetChecksForRule/for_ignore_/_recording_rules_/_passing - 1] +[TestGetChecksForRule/for_match_/_passing - 1] { "ci": { "baseBranch": "master", @@ -1495,9 +1731,9 @@ "owners": {}, "rules": [ { - "ignore": [ + "match": [ { - "for": "\u003e 0" + "for": "\u003e 15m" } ], "annotation": [ @@ -1511,7 +1747,7 @@ } --- -[TestGetChecksForRule/link - 1] +[TestGetChecksForRule/for_match_/_not_passing - 1] { "ci": { "baseBranch": "master", @@ -1550,16 +1786,16 @@ "owners": {}, "rules": [ { - "link": [ + "match": [ { - "key": "https?://(.+)", - "uri": "http://localhost/$1", - "timeout": "10s", - "headers": { - "X-Auth": "xxx" - }, + "for": "\u003e 15m" + } + ], + "annotation": [ + { + "key": "summary", "comment": "this is rule comment", - "severity": "bug" + "required": true } ] } @@ -1567,7 +1803,7 @@ } --- -[TestGetChecksForRule/rule_with_label_match_/_label_match - 1] +[TestGetChecksForRule/for_match_/_passing#01 - 1] { "ci": { "baseBranch": "master", @@ -1608,18 +1844,12 @@ { "match": [ { - "label": { - "key": "cluster", - "value": "prod" - }, - "kind": "alerting" + "keep_firing_for": "\u003e 15m" } ], - "label": [ + "annotation": [ { - "key": "priority", - "value": "(1|2|3|4|5)", - "severity": "bug", + "key": "summary", "required": true } ] @@ -1628,7 +1858,7 @@ } --- -[TestGetChecksForRule/rule_with_annotation_match_/_annotation_match - 1] +[TestGetChecksForRule/for_match_/_passing#02 - 1] { "ci": { "baseBranch": "master", @@ -1669,20 +1899,12 @@ { "match": [ { - "annotation": { - "key": "cluster", - "value": "prod" - }, - "kind": "alerting" + "keep_firing_for": "\u003e 15m" } ], - "label": [ + "annotation": [ { - "key": "priority", - "token": "\\w+", - "value": "(1|2|3|4|5)", - "comment": "this is rule comment", - "severity": "bug", + "key": "summary", "required": true } ] @@ -1691,7 +1913,7 @@ } --- -[TestGetChecksForRule/single_prometheus_server - 1] +[TestGetChecksForRule/for_match_/_passing#03 - 1] { "ci": { "baseBranch": "master", @@ -1728,21 +1950,25 @@ ] }, "owners": {}, - "prometheus": [ + "rules": [ { - "name": "prom", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false + "match": [ + { + "keep_firing_for": "\u003e 15m" + } + ], + "annotation": [ + { + "key": "summary", + "required": true + } + ] } ] } --- -[TestGetChecksForRule/multiple_URIs - 1] +[TestGetChecksForRule/for_match_/_recording_rules_/_not_passing - 1] { "ci": { "baseBranch": "master", @@ -1779,25 +2005,25 @@ ] }, "owners": {}, - "prometheus": [ + "rules": [ { - "name": "prom", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "failover": [ - "http://localhost/1", - "http://localhost/2" + "match": [ + { + "for": "!= 15m" + } ], - "concurrency": 16, - "rateLimit": 100, - "required": false + "annotation": [ + { + "key": "summary", + "required": true + } + ] } ] } --- -[TestGetChecksForRule/single_prometheus_server_/_path_match - 1] +[TestGetChecksForRule/for_ignore_/_passing - 1] { "ci": { "baseBranch": "master", @@ -1834,24 +2060,25 @@ ] }, "owners": {}, - "prometheus": [ + "rules": [ { - "name": "prom", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false + "ignore": [ + { + "for": "\u003c 15m" + } + ], + "annotation": [ + { + "key": "summary", + "required": true + } + ] } ] } --- -[TestGetChecksForRule/multiple_prometheus_servers - 1] +[TestGetChecksForRule/for_ignore_/_not_passing - 1] { "ci": { "baseBranch": "master", @@ -1888,36 +2115,26 @@ ] }, "owners": {}, - "prometheus": [ - { - "name": "prom", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - }, + "rules": [ { - "name": "ignore", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "foo.+" + "ignore": [ + { + "for": "\u003c 15m" + } ], - "concurrency": 16, - "rateLimit": 100, - "required": false + "annotation": [ + { + "key": "summary", + "comment": "this is rule comment", + "required": true + } + ] } ] } --- -[TestGetChecksForRule/prometheus_check_with_prometheus_servers_and_disable_comment - 1] +[TestGetChecksForRule/for_ignore_/_recording_rules_/_passing - 1] { "ci": { "baseBranch": "master", @@ -1954,41 +2171,25 @@ ] }, "owners": {}, - "prometheus": [ - { - "name": "prom1", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - }, - { - "name": "prom2", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - } - ], "rules": [ { - "cost": {} + "ignore": [ + { + "for": "\u003e 0" + } + ], + "annotation": [ + { + "key": "summary", + "required": true + } + ] } ] } --- -[TestGetChecksForRule/checks_disabled_via_config - 1] +[TestGetChecksForRule/link - 1] { "ci": { "baseBranch": "master", @@ -2022,80 +2223,29 @@ "rule/label", "rule/link", "rule/reject" - ], - "disabled": [ - "promql/counter", - "promql/rate", - "promql/vector_matching", - "promql/range_query", - "rule/duplicate", - "labels/conflict", - "alerts/absent" ] }, "owners": {}, - "prometheus": [ - { - "name": "prom1", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - } - ], - "rules": [ - { - "alerts": { - "range": "1h", - "step": "1m", - "resolve": "5m" - } - } - ] -} ---- - -[TestGetChecksForRule/single_check_enabled_via_config - 1] -{ - "ci": { - "baseBranch": "master", - "maxCommits": 20 - }, - "parser": {}, - "checks": {}, - "owners": {}, - "prometheus": [ - { - "name": "prom1", - "uri": "http://localhost", - "timeout": "1s", - "uptime": "up", - "include": [ - "rules.yml" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - } - ], "rules": [ { - "alerts": { - "range": "1h", - "step": "1m", - "resolve": "5m" - } + "link": [ + { + "key": "https?://(.+)", + "uri": "http://localhost/$1", + "timeout": "10s", + "headers": { + "X-Auth": "xxx" + }, + "comment": "this is rule comment", + "severity": "bug" + } + ] } ] } --- -[TestGetChecksForRule/two_prometheus_servers_/_expired_snooze - 1] +[TestGetChecksForRule/name - 1] { "ci": { "baseBranch": "master", @@ -2129,37 +2279,22 @@ "rule/label", "rule/link", "rule/reject" - ], - "disabled": [ - "alerts/template", - "promql/regexp" ] }, "owners": {}, - "prometheus": [ - { - "name": "prom1", - "uri": "http://localhost/1", - "timeout": "1s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false - }, + "rules": [ { - "name": "prom2", - "uri": "http://localhost/2", - "timeout": "1s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false + "name": [ + { + "key": "total:.+" + } + ] } ] } --- -[TestGetChecksForRule/tag_disables_all_prometheus_checks - 1] +[TestGetChecksForRule/two_prometheus_servers_/_disable_checks_via_file/disable_comment - 1] { "ci": { "baseBranch": "master", @@ -2193,6 +2328,11 @@ "rule/label", "rule/link", "rule/reject" + ], + "disabled": [ + "alerts/template", + "alerts/external_labels", + "alerts/absent" ] }, "owners": {}, @@ -2200,13 +2340,8 @@ { "name": "prom1", "uri": "http://localhost/1", - "timeout": "2m0s", + "timeout": "1s", "uptime": "up", - "tags": [ - "foo", - "disable", - "bar" - ], "concurrency": 16, "rateLimit": 100, "required": false @@ -2214,20 +2349,8 @@ { "name": "prom2", "uri": "http://localhost/2", - "timeout": "2m0s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false - }, - { - "name": "prom3", - "uri": "http://localhost/3", - "timeout": "2m0s", + "timeout": "1s", "uptime": "up", - "tags": [ - "foo" - ], "concurrency": 16, "rateLimit": 100, "required": false @@ -2236,7 +2359,7 @@ } --- -[TestGetChecksForRule/alerts/count_defaults - 1] +[TestGetChecksForRule/two_prometheus_servers_/_snoozed_checks_via_comment - 1] { "ci": { "baseBranch": "master", @@ -2270,33 +2393,37 @@ "rule/label", "rule/link", "rule/reject" + ], + "disabled": [ + "alerts/template", + "promql/regexp" ] }, "owners": {}, "prometheus": [ { - "name": "prom", - "uri": "http://localhost", + "name": "prom1", + "uri": "http://localhost/1", "timeout": "1s", "uptime": "up", "concurrency": 16, "rateLimit": 100, "required": false - } - ], - "rules": [ + }, { - "alerts": { - "range": "1d", - "step": "1m", - "resolve": "5m" - } + "name": "prom2", + "uri": "http://localhost/2", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false } ] } --- -[TestGetChecksForRule/alerts/count_full - 1] +[TestGetChecksForRule/two_prometheus_servers_/_expired_snooze - 1] { "ci": { "baseBranch": "master", @@ -2330,36 +2457,37 @@ "rule/label", "rule/link", "rule/reject" + ], + "disabled": [ + "alerts/template", + "promql/regexp" ] }, "owners": {}, "prometheus": [ { - "name": "prom", - "uri": "http://localhost", + "name": "prom1", + "uri": "http://localhost/1", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "prom2", + "uri": "http://localhost/2", "timeout": "1s", "uptime": "up", "concurrency": 16, "rateLimit": 100, "required": false } - ], - "rules": [ - { - "alerts": { - "range": "1d", - "step": "1m", - "resolve": "5m", - "comment": "this is rule comment", - "severity": "bug", - "minCount": 100 - } - } ] } --- -[TestGetChecksForRule/two_prometheus_servers_/_disable_all_checks_via_comment - 1] +[TestGetChecksForRule/tag_disables_all_prometheus_checks - 1] { "ci": { "baseBranch": "master", @@ -2393,10 +2521,6 @@ "rule/label", "rule/link", "rule/reject" - ], - "disabled": [ - "alerts/template", - "alerts/external_labels" ] }, "owners": {}, @@ -2404,8 +2528,13 @@ { "name": "prom1", "uri": "http://localhost/1", - "timeout": "1s", + "timeout": "2m0s", "uptime": "up", + "tags": [ + "foo", + "disable", + "bar" + ], "concurrency": 16, "rateLimit": 100, "required": false @@ -2413,8 +2542,20 @@ { "name": "prom2", "uri": "http://localhost/2", - "timeout": "1s", + "timeout": "2m0s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "prom3", + "uri": "http://localhost/3", + "timeout": "2m0s", "uptime": "up", + "tags": [ + "foo" + ], "concurrency": 16, "rateLimit": 100, "required": false @@ -2423,7 +2564,7 @@ } --- -[TestGetChecksForRule/multiple_cost_checks - 1] +[TestGetChecksForRule/tag_snoozes_all_prometheus_checks - 1] { "ci": { "baseBranch": "master", @@ -2463,11 +2604,13 @@ "prometheus": [ { "name": "prom1", - "uri": "http://localhost", - "timeout": "1s", + "uri": "http://localhost/1", + "timeout": "2m0s", "uptime": "up", - "include": [ - "rules.yml" + "tags": [ + "foo", + "disable", + "bar" ], "concurrency": 16, "rateLimit": 100, @@ -2475,41 +2618,30 @@ }, { "name": "prom2", - "uri": "http://localhost", - "timeout": "1s", + "uri": "http://localhost/2", + "timeout": "2m0s", "uptime": "up", - "include": [ - "rules.yml" - ], "concurrency": 16, "rateLimit": 100, "required": false - } - ], - "rules": [ - { - "cost": { - "comment": "this is rule comment", - "severity": "info" - } }, { - "cost": { - "severity": "warning", - "maxSeries": 10000 - } - }, - { - "cost": { - "severity": "bug", - "maxSeries": 20000 - } + "name": "prom3", + "uri": "http://localhost/3", + "timeout": "2m0s", + "uptime": "up", + "tags": [ + "foo" + ], + "concurrency": 16, + "rateLimit": 100, + "required": false } ] } --- -[TestGetChecksForRule/two_prometheus_servers_/_disable_checks_via_file/disable_comment - 1] +[TestGetChecksForRule/alerts/count_defaults - 1] { "ci": { "baseBranch": "master", @@ -2543,38 +2675,33 @@ "rule/label", "rule/link", "rule/reject" - ], - "disabled": [ - "alerts/template", - "alerts/external_labels", - "alerts/absent" ] }, "owners": {}, "prometheus": [ { - "name": "prom1", - "uri": "http://localhost/1", + "name": "prom", + "uri": "http://localhost", "timeout": "1s", "uptime": "up", "concurrency": 16, "rateLimit": 100, "required": false - }, + } + ], + "rules": [ { - "name": "prom2", - "uri": "http://localhost/2", - "timeout": "1s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false + "alerts": { + "range": "1d", + "step": "1m", + "resolve": "5m" + } } ] } --- -[TestGetChecksForRule/two_prometheus_servers_/_snoozed_checks_via_comment - 1] +[TestGetChecksForRule/alerts/count_full - 1] { "ci": { "baseBranch": "master", @@ -2608,37 +2735,36 @@ "rule/label", "rule/link", "rule/reject" - ], - "disabled": [ - "alerts/template", - "promql/regexp" ] }, "owners": {}, "prometheus": [ { - "name": "prom1", - "uri": "http://localhost/1", + "name": "prom", + "uri": "http://localhost", "timeout": "1s", "uptime": "up", "concurrency": 16, "rateLimit": 100, "required": false - }, + } + ], + "rules": [ { - "name": "prom2", - "uri": "http://localhost/2", - "timeout": "1s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false + "alerts": { + "range": "1d", + "step": "1m", + "resolve": "5m", + "comment": "this is rule comment", + "severity": "bug", + "minCount": 100 + } } ] } --- -[TestGetChecksForRule/tag_snoozes_all_prometheus_checks - 1] +[TestGetChecksForRule/custom_range_query - 1] { "ci": { "baseBranch": "master", @@ -2675,47 +2801,18 @@ ] }, "owners": {}, - "prometheus": [ - { - "name": "prom1", - "uri": "http://localhost/1", - "timeout": "2m0s", - "uptime": "up", - "tags": [ - "foo", - "disable", - "bar" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false - }, - { - "name": "prom2", - "uri": "http://localhost/2", - "timeout": "2m0s", - "uptime": "up", - "concurrency": 16, - "rateLimit": 100, - "required": false - }, + "rules": [ { - "name": "prom3", - "uri": "http://localhost/3", - "timeout": "2m0s", - "uptime": "up", - "tags": [ - "foo" - ], - "concurrency": 16, - "rateLimit": 100, - "required": false + "range_query": { + "max": "1h", + "severity": "bug" + } } ] } --- -[TestGetChecksForRule/name - 1] +[TestGetChecksForRule/state_mismatch - 1] { "ci": { "baseBranch": "master", @@ -2754,9 +2851,39 @@ "owners": {}, "rules": [ { - "name": [ + "match": [ { - "key": "total:.+" + "state": [ + "renamed" + ] + } + ], + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "keep": [ + "job" + ] + } + ] + }, + { + "ignore": [ + { + "state": [ + "modified" + ] + } + ], + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "strip": [ + "instance", + "rack" + ] } ] } @@ -2764,7 +2891,7 @@ } --- -[TestGetChecksForRule/custom_range_query - 1] +[TestGetChecksForRule/state_match - 1] { "ci": { "baseBranch": "master", @@ -2803,10 +2930,41 @@ "owners": {}, "rules": [ { - "range_query": { - "max": "1h", - "severity": "bug" - } + "match": [ + { + "state": [ + "renamed" + ] + } + ], + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "keep": [ + "job" + ] + } + ] + }, + { + "ignore": [ + { + "state": [ + "modified" + ] + } + ], + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "strip": [ + "instance", + "rack" + ] + } + ] } ] } diff --git a/internal/config/config.go b/internal/config/config.go index 57764bb8..77c62c23 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,7 @@ import ( "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/promapi" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsimple" @@ -80,9 +81,68 @@ func (cfg Config) String() string { return string(content) } -func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry, disabledChecks []string) []checks.RuleChecker { +func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry) []checks.RuleChecker { enabled := []checks.RuleChecker{} + proms := gen.ServersForPath(entry.Path.Name) + allChecks := getBaseChecks(ctx, proms, entry) + for _, rule := range cfg.Rules { + allChecks = append(allChecks, rule.resolveChecks(ctx, entry, proms)...) + } + + for _, cm := range allChecks { + // Entry state is not what the check is for. + if !slices.Contains(cm.check.Meta().States, entry.State) { + continue + } + + // check if check is disabled for specific rule + if !isEnabled(cfg.Checks.Enabled, entry.DisabledChecks, entry.Rule, cm.name, cm.check, cm.tags) { + continue + } + + // check if rule was disabled + if !isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, entry.Rule, cm.name, cm.check, cm.tags) { + continue + } + // check if rule was already enabled + var v bool + for _, er := range enabled { + if er.String() == cm.check.String() { + v = true + break + } + } + if !v { + enabled = append(enabled, cm.check) + } + } + + el := []string{} + for _, e := range enabled { + el = append(el, fmt.Sprintf("%v", e)) + } + slog.Debug("Configured checks for rule", + slog.Any("enabled", el), + slog.String("path", entry.Path.Name), + slog.String("rule", entry.Rule.Name()), + ) + + return enabled +} + +func getBaseChecks(ctx context.Context, proms []*promapi.FailoverGroup, entry discovery.Entry) []checkMeta { + cmd := ctx.Value(CommandKey).(ContextCommandVal) + if cmd == CICommand { + switch entry.State { + case discovery.Noop, discovery.Excluded: + // Don't include base checks for non-modified rules when running ci. + return []checkMeta{} + case discovery.Unknown, discovery.Added, discovery.Modified, discovery.Removed, discovery.Moved: + // Include all checks. + } + } + allChecks := []checkMeta{ { name: checks.SyntaxCheckName, @@ -114,8 +174,6 @@ func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerato }, } - proms := gen.ServersForPath(entry.Path.Name) - for _, p := range proms { allChecks = append(allChecks, checkMeta{ name: checks.RateCheckName, @@ -163,50 +221,7 @@ func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerato tags: p.Tags(), }) } - - for _, rule := range cfg.Rules { - allChecks = append(allChecks, rule.resolveChecks(ctx, entry, proms)...) - } - - for _, cm := range allChecks { - // Entry state is not what the check is for. - if !slices.Contains(cm.check.Meta().States, entry.State) { - continue - } - - // check if check is disabled for specific rule - if !isEnabled(cfg.Checks.Enabled, disabledChecks, entry.Rule, cm.name, cm.check, cm.tags) { - continue - } - - // check if rule was disabled - if !isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, entry.Rule, cm.name, cm.check, cm.tags) { - continue - } - // check if rule was already enabled - var v bool - for _, er := range enabled { - if er.String() == cm.check.String() { - v = true - break - } - } - if !v { - enabled = append(enabled, cm.check) - } - } - - el := []string{} - for _, e := range enabled { - el = append(el, fmt.Sprintf("%v", e)) - } - slog.Debug("Configured checks for rule", - slog.Any("enabled", el), - slog.String("path", entry.Path.Name), - slog.String("rule", entry.Rule.Name()), - ) - - return enabled + return allChecks } func getContext() *hcl.EvalContext { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 221d4235..46786c61 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -147,11 +147,10 @@ func newRule(t *testing.T, content string) parser.Rule { func TestGetChecksForRule(t *testing.T) { type testCaseT struct { - title string - config string - checks []string - disabledChecks []string - entry discovery.Entry + title string + config string + checks []string + entry discovery.Entry } testCases := []testCaseT{ @@ -1603,6 +1602,7 @@ checks { - record: foo expr: sum(foo) `), + DisabledChecks: []string{"promql/rate", "promql/vector_matching", "rule/duplicate", "labels/conflict", "promql/counter"}, }, checks: []string{ checks.SyntaxCheckName, @@ -1611,7 +1611,6 @@ checks { checks.FragileCheckName, checks.RegexpCheckName, }, - disabledChecks: []string{"promql/rate", "promql/vector_matching", "rule/duplicate", "labels/conflict", "promql/counter"}, }, { title: "two prometheus servers / snoozed checks via comment", @@ -1647,6 +1646,7 @@ checks { expr: sum(foo) # pint file/disable promql/vector_matching `), + DisabledChecks: []string{"promql/rate"}, }, checks: []string{ checks.SyntaxCheckName, @@ -1659,7 +1659,6 @@ checks { checks.LabelsConflictCheckName + "(prom2)", checks.AlertsExternalLabelsCheckName + "(prom2)", }, - disabledChecks: []string{"promql/rate"}, }, { title: "two prometheus servers / expired snooze", @@ -1691,6 +1690,7 @@ checks { expr: sum(foo) # pint file/disable promql/vector_matching `), + DisabledChecks: []string{"promql/rate"}, }, checks: []string{ checks.SyntaxCheckName, @@ -1714,7 +1714,6 @@ checks { checks.CounterCheckName + "(prom2)", checks.AlertsAbsentCheckName + "(prom2)", }, - disabledChecks: []string{"promql/rate"}, }, { title: "tag disables all prometheus checks", @@ -1958,6 +1957,91 @@ rule { checks.RangeQueryCheckName + "(1h)", }, }, + { + title: "state mismatch", + config: ` +rule { + match { + state = ["renamed"] + } + aggregate ".+" { + severity = "bug" + keep = ["job"] + } +} +rule { + ignore { + state = ["modified"] + } + aggregate ".+" { + severity = "bug" + strip = ["instance", "rack"] + } +}`, + entry: discovery.Entry{ + State: discovery.Modified, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, ` +- record: foo + expr: sum(foo) +`), + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + }, + }, + { + title: "state match", + config: ` +rule { + match { + state = ["renamed"] + } + aggregate ".+" { + severity = "bug" + keep = ["job"] + } +} +rule { + ignore { + state = ["modified"] + } + aggregate ".+" { + severity = "bug" + strip = ["instance", "rack"] + } +}`, + entry: discovery.Entry{ + State: discovery.Moved, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, ` +- record: foo + expr: sum(foo) +`), + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + checks.AggregationCheckName + "(job:true)", + checks.AggregationCheckName + "(instance:false)", + checks.AggregationCheckName + "(rack:false)", + }, + }, } dir := t.TempDir() @@ -1977,7 +2061,7 @@ rule { defer gen.Stop() require.NoError(t, gen.GenerateStatic()) - checks := cfg.GetChecksForRule(ctx, gen, tc.entry, tc.disabledChecks) + checks := cfg.GetChecksForRule(ctx, gen, tc.entry) checkNames := make([]string, 0, len(checks)) for _, c := range checks { checkNames = append(checkNames, c.String()) @@ -2314,6 +2398,14 @@ func TestConfigErrors(t *testing.T) { }`, err: `not a valid duration string: "abc"`, }, + { + config: `rule { + match { + state = ["added", "foo"] + } +}`, + err: "unknown rule state: foo", + }, } dir := t.TempDir() diff --git a/internal/config/match.go b/internal/config/match.go index 1b7edb6c..d50980e9 100644 --- a/internal/config/match.go +++ b/internal/config/match.go @@ -16,11 +16,12 @@ const ( AlertingRuleType = "alerting" RecordingRuleType = "recording" InvalidRuleType = "invalid" -) -type ( - ContextCommandKey string - ContextCommandVal string + StateAny = "any" + StateAdded = "added" + StateModified = "modified" + StateRenamed = "renamed" + StateUnmodified = "unmodified" ) var ( @@ -28,6 +29,14 @@ var ( CICommand ContextCommandVal = "ci" LintCommand ContextCommandVal = "lint" WatchCommand ContextCommandVal = "watch" + + CIStates = []string{StateAdded, StateModified, StateRenamed} + AnyStates = []string{StateAny} +) + +type ( + ContextCommandKey string + ContextCommandVal string ) type Match struct { @@ -39,6 +48,7 @@ type Match struct { Kind string `hcl:"kind,optional" json:"kind,omitempty"` For string `hcl:"for,optional" json:"for,omitempty"` KeepFiringFor string `hcl:"keep_firing_for,optional" json:"keep_firing_for,omitempty"` + State []string `hcl:"state,optional" json:"state,omitempty"` } func (m Match) validate(allowEmpty bool) error { @@ -77,7 +87,16 @@ func (m Match) validate(allowEmpty bool) error { } } - if !allowEmpty && m.Path == "" && m.Name == "" && m.Kind == "" && m.Label == nil && m.Annotation == nil && m.Command == nil && m.For == "" { + for _, s := range m.State { + switch s { + case StateAny, StateAdded, StateModified, StateRenamed, StateUnmodified: + // valid values + default: + return fmt.Errorf("unknown rule state: %s", s) + } + } + + if !allowEmpty && m.Path == "" && m.Name == "" && m.Kind == "" && m.Label == nil && m.Annotation == nil && m.Command == nil && m.For == "" && m.State == nil { return errors.New("ignore block must have at least one condition") } @@ -93,6 +112,10 @@ func (m Match) IsMatch(ctx context.Context, path string, e discovery.Entry) bool } } + if len(m.State) != 0 && !stateMatches(m.State, e.State) { + return false + } + if m.Kind != "" { if e.Rule.AlertingRule != nil && m.Kind != AlertingRuleType { return false @@ -299,3 +322,29 @@ func (dm durationMatch) isMatch(dur time.Duration) bool { } return false } + +func stateMatches(states []string, state discovery.ChangeType) bool { + for _, s := range states { + switch s { + case StateAny: + return true + case StateAdded: + if state == discovery.Added { + return true + } + case StateModified: + if state == discovery.Modified { + return true + } + case StateRenamed: + if state == discovery.Moved { + return true + } + case StateUnmodified: + if state == discovery.Noop { + return true + } + } + } + return false +} diff --git a/internal/config/rule.go b/internal/config/rule.go index 18da02c6..81ca3093 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -113,6 +113,8 @@ func (rule Rule) validate() (err error) { } func (rule Rule) resolveChecks(ctx context.Context, e discovery.Entry, prometheusServers []*promapi.FailoverGroup) []checkMeta { + cmd := ctx.Value(CommandKey).(ContextCommandVal) + enabled := []checkMeta{} for _, ignore := range rule.Ignore { @@ -124,6 +126,14 @@ func (rule Rule) resolveChecks(ctx context.Context, e discovery.Entry, prometheu if len(rule.Match) > 0 { var found bool for _, match := range rule.Match { + switch { + case len(match.State) != 0: + // use value from config + case cmd == CICommand: + match.State = CIStates + default: + match.State = AnyStates + } if match.IsMatch(ctx, e.Path.Name, e) { found = true break @@ -132,6 +142,19 @@ func (rule Rule) resolveChecks(ctx context.Context, e discovery.Entry, prometheu if !found { return enabled } + } else { + // If there are no match blocks then we would allow rule in any state to pass through. + // But we need to exclude some states depending on commands we run. + var match Match + switch cmd { + case CICommand: + match.State = CIStates + default: + match.State = AnyStates + } + if !match.IsMatch(ctx, e.Path.Name, e) { + return enabled + } } if len(rule.Aggregate) > 0 { diff --git a/internal/config/rule_test.go b/internal/config/rule_test.go index ac419b6f..7870bf5f 100644 --- a/internal/config/rule_test.go +++ b/internal/config/rule_test.go @@ -507,6 +507,164 @@ func TestMatch(t *testing.T) { }, isMatch: false, }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{ + Command: &config.CICommand, + State: config.CIStates, + }, + isMatch: false, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Added, + }, + match: config.Match{ + Command: &config.CICommand, + }, + isMatch: true, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{ + Command: &config.CICommand, + State: []string{config.StateAny}, + }, + isMatch: true, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{ + Command: &config.CICommand, + State: []string{config.StateAdded}, + }, + isMatch: false, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{ + Command: &config.CICommand, + State: []string{config.StateUnmodified}, + }, + isMatch: true, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Moved, + }, + match: config.Match{ + Command: &config.CICommand, + State: []string{config.StateUnmodified}, + }, + isMatch: false, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Modified, + }, + match: config.Match{ + Command: &config.CICommand, + State: []string{config.StateModified}, + }, + isMatch: true, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Removed, + }, + match: config.Match{ + State: []string{config.StateModified}, + }, + isMatch: false, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Moved, + }, + match: config.Match{ + State: []string{config.StateRenamed}, + }, + isMatch: true, + }, + { + cmd: config.CICommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Unknown, + }, + match: config.Match{ + State: []string{config.StateRenamed}, + }, + isMatch: false, + }, + { + cmd: config.LintCommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{}, + isMatch: true, + }, + { + cmd: config.WatchCommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{}, + isMatch: true, + }, + { + cmd: config.WatchCommand, + path: "foo.yaml", + entry: discovery.Entry{ + Rule: parser.Rule{}, + State: discovery.Noop, + }, + match: config.Match{ + State: []string{}, + }, + isMatch: true, + }, } for i, tc := range testCases { diff --git a/internal/discovery/git_branch.go b/internal/discovery/git_branch.go index 979d5aa9..eba76368 100644 --- a/internal/discovery/git_branch.go +++ b/internal/discovery/git_branch.go @@ -36,10 +36,6 @@ type GitBranchFinder struct { } func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) { - for i := range allEntries { - allEntries[i].State = Excluded - } - changes, err := git.Changes(f.gitCmd, f.baseBranch, f.filter) if err != nil { return nil, err From dd52e56015d89523eceb03dd7b5dabd5c1d8a0ab Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 23 Aug 2024 16:43:41 +0100 Subject: [PATCH 2/3] Refactor how rules are selected for checks --- cmd/pint/ci.go | 11 - internal/config/config.go | 135 +----------- internal/config/parsed_rule.go | 391 +++++++++++++++++++++++++++++++++ internal/config/rule.go | 222 ------------------- 4 files changed, 397 insertions(+), 362 deletions(-) create mode 100644 internal/config/parsed_rule.go diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go index 9da26c43..dfa7819f 100644 --- a/cmd/pint/ci.go +++ b/cmd/pint/ci.go @@ -96,7 +96,6 @@ func actionCI(c *cli.Context) error { if err != nil { return err } - entries = filterNoopEntries(entries) ctx := context.WithValue(context.Background(), config.CommandKey, config.CICommand) @@ -343,13 +342,3 @@ func detectGithubActions(gh *config.GitHub) *config.GitHub { } return gh } - -func filterNoopEntries(src []discovery.Entry) (dst []discovery.Entry) { - for _, e := range src { - if e.State == discovery.Noop && e.PathError != nil { - continue - } - dst = append(dst, e) - } - return dst -} diff --git a/internal/config/config.go b/internal/config/config.go index 77c62c23..74c13f7c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,7 +15,6 @@ import ( "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" - "github.com/cloudflare/pint/internal/promapi" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclsimple" @@ -84,37 +83,14 @@ func (cfg Config) String() string { func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry) []checks.RuleChecker { enabled := []checks.RuleChecker{} + defaultMatch := []Match{{State: defaultMatchStates(ctx.Value(CommandKey).(ContextCommandVal))}} proms := gen.ServersForPath(entry.Path.Name) - allChecks := getBaseChecks(ctx, proms, entry) - for _, rule := range cfg.Rules { - allChecks = append(allChecks, rule.resolveChecks(ctx, entry, proms)...) + for _, pr := range baseRules(proms, defaultMatch) { + enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) } - - for _, cm := range allChecks { - // Entry state is not what the check is for. - if !slices.Contains(cm.check.Meta().States, entry.State) { - continue - } - - // check if check is disabled for specific rule - if !isEnabled(cfg.Checks.Enabled, entry.DisabledChecks, entry.Rule, cm.name, cm.check, cm.tags) { - continue - } - - // check if rule was disabled - if !isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, entry.Rule, cm.name, cm.check, cm.tags) { - continue - } - // check if rule was already enabled - var v bool - for _, er := range enabled { - if er.String() == cm.check.String() { - v = true - break - } - } - if !v { - enabled = append(enabled, cm.check) + for _, rule := range cfg.Rules { + for _, pr := range parseRule(rule, proms, defaultMatch) { + enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) } } @@ -131,99 +107,6 @@ func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerato return enabled } -func getBaseChecks(ctx context.Context, proms []*promapi.FailoverGroup, entry discovery.Entry) []checkMeta { - cmd := ctx.Value(CommandKey).(ContextCommandVal) - if cmd == CICommand { - switch entry.State { - case discovery.Noop, discovery.Excluded: - // Don't include base checks for non-modified rules when running ci. - return []checkMeta{} - case discovery.Unknown, discovery.Added, discovery.Modified, discovery.Removed, discovery.Moved: - // Include all checks. - } - } - - allChecks := []checkMeta{ - { - name: checks.SyntaxCheckName, - check: checks.NewSyntaxCheck(), - }, - { - name: checks.AlertForCheckName, - check: checks.NewAlertsForCheck(), - }, - { - name: checks.ComparisonCheckName, - check: checks.NewComparisonCheck(), - }, - { - name: checks.TemplateCheckName, - check: checks.NewTemplateCheck(), - }, - { - name: checks.FragileCheckName, - check: checks.NewFragileCheck(), - }, - { - name: checks.RegexpCheckName, - check: checks.NewRegexpCheck(), - }, - { - name: checks.RuleDependencyCheckName, - check: checks.NewRuleDependencyCheck(), - }, - } - - for _, p := range proms { - allChecks = append(allChecks, checkMeta{ - name: checks.RateCheckName, - check: checks.NewRateCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.SeriesCheckName, - check: checks.NewSeriesCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.VectorMatchingCheckName, - check: checks.NewVectorMatchingCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.RangeQueryCheckName, - check: checks.NewRangeQueryCheck(p, 0, "", checks.Warning), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.RuleDuplicateCheckName, - check: checks.NewRuleDuplicateCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.LabelsConflictCheckName, - check: checks.NewLabelsConflictCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.AlertsExternalLabelsCheckName, - check: checks.NewAlertsExternalLabelsCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.CounterCheckName, - check: checks.NewCounterCheck(p), - tags: p.Tags(), - }) - allChecks = append(allChecks, checkMeta{ - name: checks.AlertsAbsentCheckName, - check: checks.NewAlertsAbsentCheck(p), - tags: p.Tags(), - }) - } - return allChecks -} - func getContext() *hcl.EvalContext { vars := map[string]cty.Value{} for _, e := range os.Environ() { @@ -337,9 +220,3 @@ func parseDuration(d string) (time.Duration, error) { } return time.Duration(mdur), nil } - -type checkMeta struct { - name string - check checks.RuleChecker - tags []string -} diff --git a/internal/config/parsed_rule.go b/internal/config/parsed_rule.go new file mode 100644 index 00000000..f759939e --- /dev/null +++ b/internal/config/parsed_rule.go @@ -0,0 +1,391 @@ +package config + +import ( + "context" + "time" + + "golang.org/x/exp/slices" + + "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/promapi" +) + +type parsedRule struct { + match []Match + ignore []Match + name string + check checks.RuleChecker + tags []string +} + +func (rule parsedRule) entryChecks(ctx context.Context, enabled, disabled []string, checks []checks.RuleChecker, e discovery.Entry) []checks.RuleChecker { + for _, ignore := range rule.ignore { + if ignore.IsMatch(ctx, e.Path.Name, e) { + return checks + } + } + + if len(rule.match) > 0 { + var found bool + for _, match := range rule.match { + if match.IsMatch(ctx, e.Path.Name, e) { + found = true + break + } + } + if !found { + return checks + } + } + + // Entry state is not what the check is for. + if !slices.Contains(rule.check.Meta().States, e.State) { + return checks + } + + // Check if check is disabled for specific rule. + if !isEnabled(enabled, e.DisabledChecks, e.Rule, rule.name, rule.check, rule.tags) { + return checks + } + + // Check if rule was disabled globally. + if !isEnabled(enabled, disabled, e.Rule, rule.name, rule.check, rule.tags) { + return checks + } + // Check if rule was already enabled. + var v bool + for _, er := range checks { + if er.String() == rule.check.String() { + v = true + break + } + } + if !v { + checks = append(checks, rule.check) + } + + return checks +} + +func defaultMatchStates(cmd ContextCommandVal) []string { + switch cmd { + case CICommand: + return CIStates + default: + return AnyStates + } +} + +func baseRules(proms []*promapi.FailoverGroup, match []Match) (rules []parsedRule) { + rules = append(rules, + parsedRule{ + match: match, + name: checks.SyntaxCheckName, + check: checks.NewSyntaxCheck(), + }, + parsedRule{ + match: match, + name: checks.AlertForCheckName, + check: checks.NewAlertsForCheck(), + }, + parsedRule{ + match: match, + name: checks.ComparisonCheckName, + check: checks.NewComparisonCheck(), + }, + parsedRule{ + match: match, + name: checks.TemplateCheckName, + check: checks.NewTemplateCheck(), + }, + parsedRule{ + match: match, + name: checks.FragileCheckName, + check: checks.NewFragileCheck(), + }, + parsedRule{ + match: match, + name: checks.RegexpCheckName, + check: checks.NewRegexpCheck(), + }, + parsedRule{ + match: match, + name: checks.RuleDependencyCheckName, + check: checks.NewRuleDependencyCheck(), + }, + ) + + for _, p := range proms { + rules = append(rules, + parsedRule{ + match: match, + name: checks.RateCheckName, + check: checks.NewRateCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.SeriesCheckName, + check: checks.NewSeriesCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.VectorMatchingCheckName, + check: checks.NewVectorMatchingCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.RangeQueryCheckName, + check: checks.NewRangeQueryCheck(p, 0, "", checks.Warning), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.RuleDuplicateCheckName, + check: checks.NewRuleDuplicateCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.LabelsConflictCheckName, + check: checks.NewLabelsConflictCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.AlertsExternalLabelsCheckName, + check: checks.NewAlertsExternalLabelsCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.CounterCheckName, + check: checks.NewCounterCheck(p), + tags: p.Tags(), + }, + parsedRule{ + match: match, + name: checks.AlertsAbsentCheckName, + check: checks.NewAlertsAbsentCheck(p), + tags: p.Tags(), + }, + ) + } + + return rules +} + +func defaultRuleMatch(match, fallback []Match) []Match { + if len(match) == 0 { + return fallback + } + return match +} + +func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultMatch []Match) (rules []parsedRule) { + if len(rule.Aggregate) > 0 { + var nameRegex *checks.TemplatedRegexp + for _, aggr := range rule.Aggregate { + if aggr.Name != "" { + nameRegex = checks.MustTemplatedRegexp(aggr.Name) + } + severity := aggr.getSeverity(checks.Warning) + for _, label := range aggr.Keep { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.AggregationCheckName, + check: checks.NewAggregationCheck(nameRegex, label, true, aggr.Comment, severity), + }) + } + for _, label := range aggr.Strip { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.AggregationCheckName, + check: checks.NewAggregationCheck(nameRegex, label, false, aggr.Comment, severity), + }) + } + } + } + + if rule.Cost != nil { + severity := rule.Cost.getSeverity(checks.Bug) + evalDur, _ := parseDuration(rule.Cost.MaxEvaluationDuration) + for _, prom := range prometheusServers { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.CostCheckName, + check: checks.NewCostCheck(prom, rule.Cost.MaxSeries, rule.Cost.MaxTotalSamples, rule.Cost.MaxPeakSamples, evalDur, rule.Cost.Comment, severity), + tags: prom.Tags(), + }) + } + } + + if len(rule.Annotation) > 0 { + for _, ann := range rule.Annotation { + var tokenRegex, valueRegex *checks.TemplatedRegexp + if ann.Token != "" { + tokenRegex = checks.MustRawTemplatedRegexp(ann.Token) + } + if ann.Value != "" { + valueRegex = checks.MustTemplatedRegexp(ann.Value) + } + severity := ann.getSeverity(checks.Warning) + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.AnnotationCheckName, + check: checks.NewAnnotationCheck(checks.MustTemplatedRegexp(ann.Key), tokenRegex, valueRegex, ann.Values, ann.Required, ann.Comment, severity), + }) + } + } + + if len(rule.Label) > 0 { + for _, lab := range rule.Label { + var tokenRegex, valueRegex *checks.TemplatedRegexp + if lab.Token != "" { + tokenRegex = checks.MustRawTemplatedRegexp(lab.Token) + } + if lab.Value != "" { + valueRegex = checks.MustTemplatedRegexp(lab.Value) + } + severity := lab.getSeverity(checks.Warning) + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.LabelCheckName, + check: checks.NewLabelCheck(checks.MustTemplatedRegexp(lab.Key), tokenRegex, valueRegex, lab.Values, lab.Required, lab.Comment, severity), + }) + } + } + + if rule.Alerts != nil { + qRange := time.Hour * 24 + if rule.Alerts.Range != "" { + qRange, _ = parseDuration(rule.Alerts.Range) + } + qStep := time.Minute + if rule.Alerts.Step != "" { + qStep, _ = parseDuration(rule.Alerts.Step) + } + qResolve := time.Minute * 5 + if rule.Alerts.Resolve != "" { + qResolve, _ = parseDuration(rule.Alerts.Resolve) + } + severity := rule.Alerts.getSeverity(checks.Information) + for _, prom := range prometheusServers { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.AlertsCheckName, + check: checks.NewAlertsCheck(prom, qRange, qStep, qResolve, rule.Alerts.MinCount, rule.Alerts.Comment, severity), + tags: prom.Tags(), + }) + } + } + + if len(rule.Reject) > 0 { + for _, reject := range rule.Reject { + severity := reject.getSeverity(checks.Bug) + re := checks.MustTemplatedRegexp(reject.Regex) + if reject.LabelKeys { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RejectCheckName, + check: checks.NewRejectCheck(true, false, re, nil, severity), + }) + } + if reject.LabelValues { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RejectCheckName, + check: checks.NewRejectCheck(true, false, nil, re, severity), + }) + } + if reject.AnnotationKeys { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RejectCheckName, + check: checks.NewRejectCheck(false, true, re, nil, severity), + }) + } + if reject.AnnotationValues { + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RejectCheckName, + check: checks.NewRejectCheck(false, true, nil, re, severity), + }) + } + } + } + + for _, link := range rule.RuleLink { + severity := link.getSeverity(checks.Bug) + re := checks.MustTemplatedRegexp(link.Regex) + var timeout time.Duration + if link.Timeout != "" { + timeout, _ = parseDuration(link.Timeout) + } else { + timeout = time.Minute + } + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RuleLinkCheckName, + check: checks.NewRuleLinkCheck(re, link.URI, timeout, link.Headers, link.Comment, severity), + }) + } + + if rule.For != nil { + severity, minFor, maxFor := rule.For.resolve() + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RuleForCheckName, + check: checks.NewRuleForCheck(checks.RuleForFor, minFor, maxFor, rule.For.Comment, severity), + }) + } + + if rule.KeepFiringFor != nil { + severity, minFor, maxFor := rule.KeepFiringFor.resolve() + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RuleForCheckName, + check: checks.NewRuleForCheck(checks.RuleForKeepFiringFor, minFor, maxFor, rule.KeepFiringFor.Comment, severity), + }) + } + + for _, name := range rule.RuleName { + re := checks.MustTemplatedRegexp(name.Regex) + severity := name.getSeverity(checks.Information) + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.RuleNameCheckName, + check: checks.NewRuleNameCheck(re, name.Comment, severity), + }) + } + + if rule.RangeQuery != nil { + severity := rule.RangeQuery.getSeverity(checks.Warning) + limit, _ := parseDuration(rule.RangeQuery.Max) + rules = append(rules, parsedRule{ + match: defaultRuleMatch(rule.Match, defaultMatch), + ignore: rule.Ignore, + name: checks.CostCheckName, + check: checks.NewRangeQueryCheck(nil, limit, rule.RangeQuery.Comment, severity), + }) + } + + return rules +} diff --git a/internal/config/rule.go b/internal/config/rule.go index 81ca3093..1bcedf2f 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -1,7 +1,6 @@ package config import ( - "context" "fmt" "log/slog" "regexp" @@ -9,9 +8,7 @@ import ( "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/comments" - "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/parser" - "github.com/cloudflare/pint/internal/promapi" ) type Rule struct { @@ -112,225 +109,6 @@ func (rule Rule) validate() (err error) { return nil } -func (rule Rule) resolveChecks(ctx context.Context, e discovery.Entry, prometheusServers []*promapi.FailoverGroup) []checkMeta { - cmd := ctx.Value(CommandKey).(ContextCommandVal) - - enabled := []checkMeta{} - - for _, ignore := range rule.Ignore { - if ignore.IsMatch(ctx, e.Path.Name, e) { - return enabled - } - } - - if len(rule.Match) > 0 { - var found bool - for _, match := range rule.Match { - switch { - case len(match.State) != 0: - // use value from config - case cmd == CICommand: - match.State = CIStates - default: - match.State = AnyStates - } - if match.IsMatch(ctx, e.Path.Name, e) { - found = true - break - } - } - if !found { - return enabled - } - } else { - // If there are no match blocks then we would allow rule in any state to pass through. - // But we need to exclude some states depending on commands we run. - var match Match - switch cmd { - case CICommand: - match.State = CIStates - default: - match.State = AnyStates - } - if !match.IsMatch(ctx, e.Path.Name, e) { - return enabled - } - } - - if len(rule.Aggregate) > 0 { - var nameRegex *checks.TemplatedRegexp - for _, aggr := range rule.Aggregate { - if aggr.Name != "" { - nameRegex = checks.MustTemplatedRegexp(aggr.Name) - } - severity := aggr.getSeverity(checks.Warning) - for _, label := range aggr.Keep { - enabled = append(enabled, checkMeta{ - name: checks.AggregationCheckName, - check: checks.NewAggregationCheck(nameRegex, label, true, aggr.Comment, severity), - }) - } - for _, label := range aggr.Strip { - enabled = append(enabled, checkMeta{ - name: checks.AggregationCheckName, - check: checks.NewAggregationCheck(nameRegex, label, false, aggr.Comment, severity), - }) - } - } - } - - if rule.Cost != nil { - severity := rule.Cost.getSeverity(checks.Bug) - evalDur, _ := parseDuration(rule.Cost.MaxEvaluationDuration) - for _, prom := range prometheusServers { - enabled = append(enabled, checkMeta{ - name: checks.CostCheckName, - check: checks.NewCostCheck(prom, rule.Cost.MaxSeries, rule.Cost.MaxTotalSamples, rule.Cost.MaxPeakSamples, evalDur, rule.Cost.Comment, severity), - tags: prom.Tags(), - }) - } - } - - if len(rule.Annotation) > 0 { - for _, ann := range rule.Annotation { - var tokenRegex, valueRegex *checks.TemplatedRegexp - if ann.Token != "" { - tokenRegex = checks.MustRawTemplatedRegexp(ann.Token) - } - if ann.Value != "" { - valueRegex = checks.MustTemplatedRegexp(ann.Value) - } - severity := ann.getSeverity(checks.Warning) - enabled = append(enabled, checkMeta{ - name: checks.AnnotationCheckName, - check: checks.NewAnnotationCheck(checks.MustTemplatedRegexp(ann.Key), tokenRegex, valueRegex, ann.Values, ann.Required, ann.Comment, severity), - }) - } - } - - if len(rule.Label) > 0 { - for _, lab := range rule.Label { - var tokenRegex, valueRegex *checks.TemplatedRegexp - if lab.Token != "" { - tokenRegex = checks.MustRawTemplatedRegexp(lab.Token) - } - if lab.Value != "" { - valueRegex = checks.MustTemplatedRegexp(lab.Value) - } - severity := lab.getSeverity(checks.Warning) - enabled = append(enabled, checkMeta{ - name: checks.LabelCheckName, - check: checks.NewLabelCheck(checks.MustTemplatedRegexp(lab.Key), tokenRegex, valueRegex, lab.Values, lab.Required, lab.Comment, severity), - }) - } - } - - if rule.Alerts != nil { - qRange := time.Hour * 24 - if rule.Alerts.Range != "" { - qRange, _ = parseDuration(rule.Alerts.Range) - } - qStep := time.Minute - if rule.Alerts.Step != "" { - qStep, _ = parseDuration(rule.Alerts.Step) - } - qResolve := time.Minute * 5 - if rule.Alerts.Resolve != "" { - qResolve, _ = parseDuration(rule.Alerts.Resolve) - } - severity := rule.Alerts.getSeverity(checks.Information) - for _, prom := range prometheusServers { - enabled = append(enabled, checkMeta{ - name: checks.AlertsCheckName, - check: checks.NewAlertsCheck(prom, qRange, qStep, qResolve, rule.Alerts.MinCount, rule.Alerts.Comment, severity), - tags: prom.Tags(), - }) - } - } - - if len(rule.Reject) > 0 { - for _, reject := range rule.Reject { - severity := reject.getSeverity(checks.Bug) - re := checks.MustTemplatedRegexp(reject.Regex) - if reject.LabelKeys { - enabled = append(enabled, checkMeta{ - name: checks.RejectCheckName, - check: checks.NewRejectCheck(true, false, re, nil, severity), - }) - } - if reject.LabelValues { - enabled = append(enabled, checkMeta{ - name: checks.RejectCheckName, - check: checks.NewRejectCheck(true, false, nil, re, severity), - }) - } - if reject.AnnotationKeys { - enabled = append(enabled, checkMeta{ - name: checks.RejectCheckName, - check: checks.NewRejectCheck(false, true, re, nil, severity), - }) - } - if reject.AnnotationValues { - enabled = append(enabled, checkMeta{ - name: checks.RejectCheckName, - check: checks.NewRejectCheck(false, true, nil, re, severity), - }) - } - } - } - - for _, link := range rule.RuleLink { - severity := link.getSeverity(checks.Bug) - re := checks.MustTemplatedRegexp(link.Regex) - var timeout time.Duration - if link.Timeout != "" { - timeout, _ = parseDuration(link.Timeout) - } else { - timeout = time.Minute - } - enabled = append(enabled, checkMeta{ - name: checks.RuleLinkCheckName, - check: checks.NewRuleLinkCheck(re, link.URI, timeout, link.Headers, link.Comment, severity), - }) - } - - if rule.For != nil { - severity, minFor, maxFor := rule.For.resolve() - enabled = append(enabled, checkMeta{ - name: checks.RuleForCheckName, - check: checks.NewRuleForCheck(checks.RuleForFor, minFor, maxFor, rule.For.Comment, severity), - }) - } - - if rule.KeepFiringFor != nil { - severity, minFor, maxFor := rule.KeepFiringFor.resolve() - enabled = append(enabled, checkMeta{ - name: checks.RuleForCheckName, - check: checks.NewRuleForCheck(checks.RuleForKeepFiringFor, minFor, maxFor, rule.KeepFiringFor.Comment, severity), - }) - } - - for _, name := range rule.RuleName { - re := checks.MustTemplatedRegexp(name.Regex) - severity := name.getSeverity(checks.Information) - enabled = append(enabled, checkMeta{ - name: checks.RuleNameCheckName, - check: checks.NewRuleNameCheck(re, name.Comment, severity), - }) - } - - if rule.RangeQuery != nil { - severity := rule.RangeQuery.getSeverity(checks.Warning) - limit, _ := parseDuration(rule.RangeQuery.Max) - enabled = append(enabled, checkMeta{ - name: checks.CostCheckName, - check: checks.NewRangeQueryCheck(nil, limit, rule.RangeQuery.Comment, severity), - }) - } - - return enabled -} - func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool { matches := []string{ name, From 8c3a9ab251938b73a27c586dbcddab7a45bad42a Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 23 Aug 2024 17:49:14 +0100 Subject: [PATCH 3/3] Move error handing out of scan workers --- cmd/pint/scan.go | 150 ++++--------------------- cmd/pint/tests/0184_ci_file_ignore.txt | 1 - docs/configuration.md | 5 +- internal/checks/base.go | 5 +- internal/checks/error.go | 119 ++++++++++++++++++++ internal/config/config.go | 21 +++- internal/config/config_test.go | 2 +- internal/config/match.go | 9 +- internal/config/rule.go | 4 + 9 files changed, 176 insertions(+), 140 deletions(-) create mode 100644 internal/checks/error.go diff --git a/cmd/pint/scan.go b/cmd/pint/scan.go index 466b436e..e7fed9d7 100644 --- a/cmd/pint/scan.go +++ b/cmd/pint/scan.go @@ -2,8 +2,6 @@ package main import ( "context" - "errors" - "fmt" "log/slog" "sync" "time" @@ -11,23 +9,12 @@ import ( "go.uber.org/atomic" "github.com/cloudflare/pint/internal/checks" - "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/config" "github.com/cloudflare/pint/internal/discovery" - "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/promapi" "github.com/cloudflare/pint/internal/reporter" ) -const ( - yamlParseReporter = "yaml/parse" - ignoreFileReporter = "ignore/file" - pintCommentReporter = "pint/comment" - - yamlDetails = `This Prometheus rule is not valid. -This usually means that it's missing some required fields.` -) - func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, cfg config.Config, entries []discovery.Entry) (summary reporter.Summary, err error) { if isOffline { slog.Info("Offline mode, skipping Prometheus discovery") @@ -84,7 +71,7 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr continue case entry.Rule.Error.Err != nil && entry.State == discovery.Removed: continue - case entry.PathError == nil && entry.Rule.Error.Err == nil: + default: if entry.Rule.RecordingRule != nil { rulesParsedTotal.WithLabelValues(config.RecordingRuleType).Inc() slog.Debug("Found recording rule", @@ -101,9 +88,16 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr slog.String("lines", entry.Rule.Lines.String()), ) } + if entry.Rule.Error.Err != nil { + slog.Debug("Found invalid rule", + slog.String("path", entry.Path.Name), + slog.String("lines", entry.Rule.Lines.String()), + ) + rulesParsedTotal.WithLabelValues(config.InvalidRuleType).Inc() + } checkedEntriesCount.Inc() - checkList := cfg.GetChecksForRule(ctx, gen, entry) + checkList := cfg.GetChecksForEntry(ctx, gen, entry) for _, check := range checkList { checkIterationChecks.Inc() if check.Meta().IsOnline { @@ -113,15 +107,6 @@ func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.Pr } jobs <- scanJob{entry: entry, allEntries: entries, check: check} } - default: - if entry.Rule.Error.Err != nil { - slog.Debug("Found invalid rule", - slog.String("path", entry.Path.Name), - slog.String("lines", entry.Rule.Lines.String()), - ) - rulesParsedTotal.WithLabelValues(config.InvalidRuleType).Inc() - } - jobs <- scanJob{entry: entry, allEntries: entries, check: nil} } } defer close(jobs) @@ -154,72 +139,19 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte case <-ctx.Done(): return default: - var commentErr comments.CommentError - var ignoreErr discovery.FileIgnoreError - switch { - case errors.As(job.entry.PathError, &ignoreErr): - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: ignoreErr.Line, - Last: ignoreErr.Line, - }, - Reporter: ignoreFileReporter, - Text: ignoreErr.Error(), - Severity: checks.Information, - }, - Owner: job.entry.Owner, - } - case errors.As(job.entry.PathError, &commentErr): - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: commentErr.Line, - Last: commentErr.Line, - }, - Reporter: pintCommentReporter, - Text: "This comment is not a valid pint control comment: " + commentErr.Error(), - Severity: checks.Warning, - }, - Owner: job.entry.Owner, - } - case job.entry.PathError != nil: - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: 1, - Last: 1, - }, - Reporter: yamlParseReporter, - Text: fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", job.entry.PathError), - Details: `pint cannot read this file because YAML parser returned an error. -This usually means that you have an indention error or the file doesn't have the YAML structure required by Prometheus for [recording](https://prometheus.io/docs/prometheus/latest/configuration/recording_rules/) and [alerting](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/) rules. -If this file is a template that will be rendered into valid YAML then you can instruct pint to ignore some lines using comments, see [pint docs](https://cloudflare.github.io/pint/ignoring.html). -`, - Severity: checks.Fatal, - }, - Owner: job.entry.Owner, - } - case job.entry.Rule.Error.Err != nil: - details := yamlDetails - if job.entry.Rule.Error.Details != "" { - details = job.entry.Rule.Error.Details - } + if job.entry.State == discovery.Unknown { + slog.Warn( + "Bug: unknown rule state", + 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.Path, job.entry.Rule, job.allEntries) + checkDuration.WithLabelValues(job.check.Reporter()).Observe(time.Since(start).Seconds()) + for _, problem := range problems { results <- reporter.Report{ Path: discovery.Path{ Name: job.entry.Path.Name, @@ -227,42 +159,8 @@ If this file is a template that will be rendered into valid YAML then you can in }, ModifiedLines: job.entry.ModifiedLines, Rule: job.entry.Rule, - Problem: checks.Problem{ - Lines: parser.LineRange{ - First: job.entry.Rule.Error.Line, - Last: job.entry.Rule.Error.Line, - }, - Reporter: yamlParseReporter, - Text: fmt.Sprintf("This rule is not a valid Prometheus rule: `%s`.", job.entry.Rule.Error.Err.Error()), - Details: details, - Severity: checks.Fatal, - }, - Owner: job.entry.Owner, - } - default: - if job.entry.State == discovery.Unknown { - slog.Warn( - "Bug: unknown rule state", - 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.Path, job.entry.Rule, job.allEntries) - checkDuration.WithLabelValues(job.check.Reporter()).Observe(time.Since(start).Seconds()) - for _, problem := range problems { - results <- reporter.Report{ - Path: discovery.Path{ - Name: job.entry.Path.Name, - SymlinkTarget: job.entry.Path.SymlinkTarget, - }, - ModifiedLines: job.entry.ModifiedLines, - Rule: job.entry.Rule, - Problem: problem, - Owner: job.entry.Owner, - } + Problem: problem, + Owner: job.entry.Owner, } } } diff --git a/cmd/pint/tests/0184_ci_file_ignore.txt b/cmd/pint/tests/0184_ci_file_ignore.txt index 01c4dbc2..93068a35 100644 --- a/cmd/pint/tests/0184_ci_file_ignore.txt +++ b/cmd/pint/tests/0184_ci_file_ignore.txt @@ -23,7 +23,6 @@ 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="No rules found, skipping Prometheus discovery" -- src/rules.yml -- # pint ignore/file - record: rule1 diff --git a/docs/configuration.md b/docs/configuration.md index 7ccbd8b7..1b7fba3e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -520,7 +520,7 @@ Syntax: rule { match { path = "(.+)" - state = [ "any|added|modified|renamed|unmodified", ... ] + state = [ "any|added|modified|renamed|removed|unmodified", ... ] name = "(.+)" kind = "alerting|recording" command = "ci|lint|watch" @@ -536,7 +536,7 @@ rule { match { ... } ignore { path = "(.+)" - state = [ "any|added|modified|renamed|unmodified", ... ] + state = [ "any|added|modified|renamed|removed|unmodified", ... ] name = "(.+)" kind = "alerting|recording" command = "ci|lint|watch" @@ -566,6 +566,7 @@ rule { - `added` - a rule is being added in a git commit, a rule can only be in this state when running `pint ci` on a pull request. - `modified` - a rule is being modified in a git commit, a rule can only be in this state when running `pint ci` on a pull request. - `renamed` - a rule is being modified in a git commit, a rule can only be in this state when running `pint ci` on a pull request. + - `removed` - a rule is being removed in a git commit, a rule can only be in this state when running `pint ci` on a pull request. - `unmodified` - a rule is not being modified in a git commit when running `pint ci` or other pint command was run that doesn't try to detect which rules were modified. - `match:name` - only rules with names (`record` for recording rules and `alert` for alerting rules) matching this pattern will be checked rule. diff --git a/internal/checks/base.go b/internal/checks/base.go index b4ded357..070f3747 100644 --- a/internal/checks/base.go +++ b/internal/checks/base.go @@ -118,8 +118,9 @@ type Problem struct { } type CheckMeta struct { - States []discovery.ChangeType - IsOnline bool + States []discovery.ChangeType + IsOnline bool + AlwaysEnabled bool } type RuleChecker interface { diff --git a/internal/checks/error.go b/internal/checks/error.go new file mode 100644 index 00000000..fa54e7b2 --- /dev/null +++ b/internal/checks/error.go @@ -0,0 +1,119 @@ +package checks + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "github.com/cloudflare/pint/internal/comments" + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/parser" +) + +const ( + ErrorCheckName = "error" + + yamlParseReporter = "yaml/parse" + ignoreFileReporter = "ignore/file" + pintCommentReporter = "pint/comment" + + yamlDetails = `This Prometheus rule is not valid. +This usually means that it's missing some required fields.` +) + +func NewErrorCheck(err error) ErrorCheck { + return ErrorCheck{err: err} +} + +type ErrorCheck struct { + err error +} + +func (c ErrorCheck) Meta() CheckMeta { + return CheckMeta{ + States: []discovery.ChangeType{ + discovery.Noop, + discovery.Added, + discovery.Modified, + discovery.Moved, + discovery.Removed, + }, + IsOnline: false, + AlwaysEnabled: true, + } +} + +func (c ErrorCheck) String() string { + return ErrorCheckName +} + +func (c ErrorCheck) Reporter() string { + return ErrorCheckName +} + +func (c ErrorCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) { + var commentErr comments.CommentError + var ignoreErr discovery.FileIgnoreError + + switch { + case errors.As(c.err, &ignoreErr): + slog.Debug("ignore/file report", slog.Any("err", ignoreErr)) + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: ignoreErr.Line, + Last: ignoreErr.Line, + }, + Reporter: ignoreFileReporter, + Text: ignoreErr.Error(), + Severity: Information, + }) + + case errors.As(c.err, &commentErr): + slog.Debug("invalid comment report", slog.Any("err", commentErr)) + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: commentErr.Line, + Last: commentErr.Line, + }, + Reporter: pintCommentReporter, + Text: "This comment is not a valid pint control comment: " + commentErr.Error(), + Severity: Warning, + }) + + case c.err != nil: + slog.Debug("yaml syntax report", slog.Any("err", c.err)) + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: 1, + Last: 1, + }, + Reporter: yamlParseReporter, + Text: fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", c.err), + Details: `pint cannot read this file because YAML parser returned an error. +This usually means that you have an indention error or the file doesn't have the YAML structure required by Prometheus for [recording](https://prometheus.io/docs/prometheus/latest/configuration/recording_rules/) and [alerting](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/) rules. +If this file is a template that will be rendered into valid YAML then you can instruct pint to ignore some lines using comments, see [pint docs](https://cloudflare.github.io/pint/ignoring.html). +`, + Severity: Fatal, + }) + + case rule.Error.Err != nil: + slog.Debug("rule error report", slog.Any("err", rule.Error.Err)) + details := yamlDetails + if rule.Error.Details != "" { + details = rule.Error.Details + } + problems = append(problems, Problem{ + Lines: parser.LineRange{ + First: rule.Error.Line, + Last: rule.Error.Line, + }, + Reporter: yamlParseReporter, + Text: fmt.Sprintf("This rule is not a valid Prometheus rule: `%s`.", rule.Error.Err.Error()), + Details: details, + Severity: Fatal, + }) + } + + return problems +} diff --git a/internal/config/config.go b/internal/config/config.go index 74c13f7c..abe59a2a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -80,18 +80,27 @@ func (cfg Config) String() string { return string(content) } -func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry) []checks.RuleChecker { +func (cfg *Config) GetChecksForEntry(ctx context.Context, gen *PrometheusGenerator, entry discovery.Entry) []checks.RuleChecker { enabled := []checks.RuleChecker{} defaultMatch := []Match{{State: defaultMatchStates(ctx.Value(CommandKey).(ContextCommandVal))}} proms := gen.ServersForPath(entry.Path.Name) - for _, pr := range baseRules(proms, defaultMatch) { - enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) - } - for _, rule := range cfg.Rules { - for _, pr := range parseRule(rule, proms, defaultMatch) { + + if entry.PathError != nil || entry.Rule.Error.Err != nil { + enabled = parsedRule{ + match: defaultMatch, + name: checks.ErrorCheckName, + check: checks.NewErrorCheck(entry.PathError), + }.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) + } else { + for _, pr := range baseRules(proms, defaultMatch) { enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) } + for _, rule := range cfg.Rules { + for _, pr := range parseRule(rule, proms, defaultMatch) { + enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) + } + } } el := []string{} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 46786c61..5fc70ed7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2061,7 +2061,7 @@ rule { defer gen.Stop() require.NoError(t, gen.GenerateStatic()) - checks := cfg.GetChecksForRule(ctx, gen, tc.entry) + checks := cfg.GetChecksForEntry(ctx, gen, tc.entry) checkNames := make([]string, 0, len(checks)) for _, c := range checks { checkNames = append(checkNames, c.String()) diff --git a/internal/config/match.go b/internal/config/match.go index d50980e9..c7ac8b12 100644 --- a/internal/config/match.go +++ b/internal/config/match.go @@ -21,6 +21,7 @@ const ( StateAdded = "added" StateModified = "modified" StateRenamed = "renamed" + StateRemoved = "removed" StateUnmodified = "unmodified" ) @@ -30,7 +31,7 @@ var ( LintCommand ContextCommandVal = "lint" WatchCommand ContextCommandVal = "watch" - CIStates = []string{StateAdded, StateModified, StateRenamed} + CIStates = []string{StateAdded, StateModified, StateRenamed, StateRemoved} AnyStates = []string{StateAny} ) @@ -89,7 +90,7 @@ func (m Match) validate(allowEmpty bool) error { for _, s := range m.State { switch s { - case StateAny, StateAdded, StateModified, StateRenamed, StateUnmodified: + case StateAny, StateAdded, StateModified, StateRenamed, StateRemoved, StateUnmodified: // valid values default: return fmt.Errorf("unknown rule state: %s", s) @@ -340,6 +341,10 @@ func stateMatches(states []string, state discovery.ChangeType) bool { if state == discovery.Moved { return true } + case StateRemoved: + if state == discovery.Removed { + return true + } case StateUnmodified: if state == discovery.Noop { return true diff --git a/internal/config/rule.go b/internal/config/rule.go index 1bcedf2f..a8cb8f4d 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -110,6 +110,10 @@ func (rule Rule) validate() (err error) { } func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool { + if check.Meta().AlwaysEnabled { + return true + } + matches := []string{ name, check.String(),