Merge pull request #93 from ahrefs/yasu/upgrade-status-rules

Upgrade status rules syntax and differentiate status state tracking b/w pipelines

Author: yasunariw

README.md

@@ -1,31 +1,35 @@
-# Notabot
+# Monorobot
 
-Notifications bot server to receive notifications from webhooks and post them to slack
+A Slackbot for GitHub monorepos. Configure how repo notifications should be routed to specified Slack channels based on file prefixes, issue/PR labels, and CI build statuses.
 
 ## Setting Up
 
-Install dependencies, if needed:
+Install dependencies via OPAM.
 
 ```sh
 opam install --deps-only .
 ```
 
-Build with
+Then, build with Dune.
 
 ```sh
 make
 ```
 
-and use resulting `_build/default/src/notabot.exe` binary.
-
 ## Running
 
-At startup time, secrets are read from the local `secrets.json` file. The main configuration is read remotely from a `notabot.json` file in the default branch, and its schema is defined in `lib/notabot.atd`.
+Run the `_build/default/src/notabot.exe` binary. The following commands are supported.
+
+- `run`: Launch the HTTP server
+- `check_gh <GH_PAYLOAD>`: read a Github notification from a file and display the actions that will be taken (used for testing)
+- `check_slack <SLACK_PAYLOAD> <SLACK_WEBHOOK>`: read a Slack notification from a file and send it to a webhook (used for testing)
 
 ### Documentation
 
-* [config](./documentation/config_docs.md)
-* [secret](./documentation/secret_docs.md)
+The bot expects two configuration files to be present.
+
+* [Repository configuration](./documentation/config_docs.md)
+* [Secrets](./documentation/secret_docs.md)
 
 ## Testing (development)
 

documentation/config_docs.md

@@ -8,67 +8,30 @@ To update the configuration, simply edit the configuration file and push your ch
 
 Refer [here](https://docs.github.com/en/free-pro-team@latest/developers/webhooks-and-events/webhook-events-and-payloads) for more information on GitHub event payload structure.
 
-# Configuration values
+## Options
 
-**example**
+**Example**
 ```json
 {
     "main_branch_name": "develop",
-    "status_rules": {
-        ...
-    },
     "prefix_rules": {
         ...
     },
     "label_rules": {
         ...
+    },
+    "status_rules": {
+        ...
     }
 }
 ```
 
 | value | description | optional | default |
 |-|-|-|-|
 | `main_branch_name` | main branch used for the repo; filtering notifications about merges of main into other branches | Yes | - |
-| `status_rules` | status rules config object | No | - |
 | `label_rules` | label rules config object | No | - |
 | `prefix_rules` | prefix rules config object | No | - |
-
-## Status Config
-
-**example**
-```json
-"status_rules": {
-    "allowed_pipelines": [
-        "default",
-        "buildkite/pipeline2"
-    ],
-    "status": {
-        "pending": false,
-        "success": "once",
-        "failure": true,
-        "error": true,
-        "cancelled": "^\\(Build #[0-9]+ canceled by .+\\|Failed (exit status 255)\\)$"
-    }
-},
-```
-
-| value | description | optional | default |
-|-|-|-|-|
-| `title` | if defines a whitelist of values for the github payload. If not specified, all is permitted. | Yes | - |
-| `status` | a `status_state` config object | No | - |
-
-### Status State
-
-A json object with fields of bools for each status type.
-
-| value | description | optional | default |
-|-|-|-|-|
-| `pending` | `true` to notify; `false` to ignore | No | - |
-| `success` | `true` to notify; `false` to notify all; `"once"` to notify the first and ignore subsequent consecutive successes| No | - |
-| `failure` | `true` to notify; `false` to ignore | No | - |
-| `error` | `true` to notify; `false` to ignore | No | - |
-| `cancelled` | provide regex to ignore `failure` notifications with a description that matches it | Yes | - |
-
+| `status_rules` | status rules config object | No | - |
 
 ## Label Options
 
@@ -163,3 +126,101 @@ A **prefix rule** specifies whether or not a Slack channel should be notified, b
 | `allow` | if commit files match any prefix in this list, they should be routed to the channel | Yes | all prefixes allowed if no list provided |
 | `ignore` | if commit files match any prefix in this list, they shouldn't be routed to the channel (even if they match any allow prefixes) | Yes | - |
 | `channel` | channel to use as webhook if the rule is matched | No | - |
+
+## Status Options
+
+Monorobot supports additional behavior for GitHub status notifications, which are typically triggered by CI builds. A payload of this type contains:
+
+- A `context` field, whose value is the name of the CI pipeline the notificiation is about
+- A `state` field, whose value is either `success`, `failure`, `pending`, or `error`
+
+The following takes place when a status notification is received.
+
+1. Check whether a status notification should be *allowed* for further processing or *ignored*, according to a list of **status rules**. The bot will check the list *in order*, and use the policy defined by the first rule that the notification satisfies. If no rule matches, the default behavior of the status state is used:
+   - `pending`: `ignore`
+   - `failure`: `allow`
+   - `error`: `allow`
+   - `success`: `allow_once`
+1. For those payloads allowed by step 1, if it isn't a main branch build notification, route to the default channel to reduce spam in topic channels. Otherwise, check the notification commit's files according to the prefix rules.
+
+Internally, the bot keeps track of the status of the last allowed payload, for a given pipeline and branch. This information is used to evaluate the status rules (see below).
+
+**Example**
+
+```json
+"status_rules": {
+    "allowed_pipelines": [
+        "default",
+        "buildkite/pipeline2"
+    ],
+    "rules": [
+        {
+            "on": ["failure"],
+            "when": {
+                "match": {
+                    "field": "description",
+                    "re": "^\\(Build #[0-9]+ canceled by .+\\|Failed (exit status 255)\\)$"
+                }
+            },
+            "policy": "ignore"
+        },
+        { "on": ["pending"], "policy": "ignore"},
+        { "on": ["failure", "error"], "policy": "allow"},
+        { "on": ["success"], "policy": "allow_once"}
+    ]
+}
+```
+
+| value | description | optional | default |
+|-|-|-|-|
+| `allowed_pipelines` | a list of pipeline names; if specified, payloads whose pipeline name is not in the list will be ignored immediately, without checking the **status rules**; otherwise, all pipelines will be included in the status rule check | Yes | - |
+| `rules` | a list of **status rules** to determine whether to *allow* or *ignore* a payload for further processing | No | - |
+
+### Status Rules
+
+A **status rule** specifies whether a GitHub status notification should generate a Slack notification, given the notification's status and the last allowed build status. There are three policy options for handling payloads:
+
+- `allow`: Notify every time.
+- `ignore`: Suppress every time.
+- `allow_once`: Only notify if the last allowed status notification's build status for the given pipeline and branch differs from the current one. Useful for suppressing consecutive build success notifications.
+
+For example, the rule:
+```
+{ "on": A, "when": B, "policy": C }
+```
+is interpreted as:
+
+> "on a notification with a build state in `A`, when condition `B` is met, adopt the policy `C`".
+
+| value | description | optional | default |
+|-|-|-|-|
+| `on` | a list of build states that can trigger this rule | No | - |
+| `when` | a **status condition** object which, if specified, must be true for the rule to match | Yes | - |
+| `policy` | a policy option (one of `allow`, `ignore`, `allow_once`) | No | - |
+
+### Status Conditions
+
+You can optionally provide a **status condition** to specify additional requirements that a notification payload should meet, in order for a status rule's policy to apply.
+
+- `all_of`: Matches if every sub-condition in a list is true. Value should be the list of sub-conditions.
+- `one_of`: Matches if at least one sub-condition in a list is true. Value should be the list of sub-conditions.
+```json
+{
+    "all_of/one_of": [ condition1, condition2, ...]
+}
+```
+- `not`: Matches if a sub-condition is false. Value should be the sub-condition.
+```json
+{
+    "not": condition
+}
+```
+- `match`: Matches a text field in the payload against a regular expression. Value should be an object of the form:
+```json
+{
+    "match": {
+        "field": "context" | "description" | "target_url",
+        "re": string // a regular expression
+    }
+}
+```

lib/action.ml

@@ -2,7 +2,6 @@ open Devkit
 open Base
 open Slack
 open Config_t
-open Config
 open Common
 open Github_j
 
@@ -13,17 +12,7 @@ let action_error msg = raise (Action_error msg)
 let log = Log.from "action"
 
 module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
-  (* this should move to context.ml once rule.ml is refactored to not depend on it *)
-  let print_config ctx =
-    let cfg = Context.get_config_exn ctx in
-    let secrets = Context.get_secrets_exn ctx in
-    log#info "using prefix routing:";
-    Rule.Prefix.print_prefix_routing cfg.prefix_rules.rules;
-    log#info "using label routing:";
-    Rule.Label.print_label_routing cfg.label_rules.rules;
-    log#info "signature checking %s" (if Option.is_some secrets.gh_hook_token then "enabled" else "disabled")
-
-  let partition_push cfg n =
+  let partition_push (cfg : Config_t.config) n =
     let default = Option.to_list cfg.prefix_rules.default_channel in
     let rules = cfg.prefix_rules.rules in
     n.commits
@@ -45,7 +34,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
     |> Map.map ~f:(fun commits -> { n with commits })
     |> Map.to_alist
 
-  let partition_label (cfg : Config.t) (labels : label list) =
+  let partition_label (cfg : Config_t.config) (labels : label list) =
     let default = cfg.label_rules.default_channel in
     let rules = cfg.label_rules.rules in
     labels |> List.concat_map ~f:(Rule.Label.match_rules ~rules) |> List.dedup_and_sort ~compare:String.compare
@@ -87,7 +76,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
     | Submitted, _, _ -> partition_label cfg n.pull_request.labels
     | _ -> []
 
-  let partition_commit (cfg : Config.t) files =
+  let partition_commit (cfg : Config_t.config) files =
     let default = Option.to_list cfg.prefix_rules.default_channel in
     let rules = cfg.prefix_rules.rules in
     let matched_channel_names =
@@ -101,18 +90,18 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
     let cfg = Context.get_config_exn ctx in
     let pipeline = n.context in
     let current_status = n.state in
-    let updated_at = n.updated_at in
-    let get_commit_info () =
+    let rules = cfg.status_rules.rules in
+    let action_on_match (branches : branch list) =
       let default = Option.to_list cfg.prefix_rules.default_channel in
-      let () = Context.refresh_pipeline_status ~pipeline ~branches:n.branches ~status:current_status ~updated_at ctx in
-      match List.is_empty n.branches with
+      let () = Context.refresh_pipeline_status ~pipeline ~branches ~status:current_status ctx in
+      match List.is_empty branches with
       | true -> Lwt.return []
       | false ->
       match cfg.main_branch_name with
       | None -> Lwt.return default
       | Some main_branch_name ->
       (* non-main branch build notifications go to default channel to reduce spam in topic channels *)
-      match List.exists n.branches ~f:(fun { name } -> String.equal name main_branch_name) with
+      match List.exists branches ~f:(fun { name } -> String.equal name main_branch_name) with
       | false -> Lwt.return default
       | true ->
         let sha = n.commit.sha in
@@ -122,27 +111,23 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
         | Ok commit -> Lwt.return @@ partition_commit cfg commit.files
         )
     in
-    let res =
-      match
-        List.exists cfg.status_rules.status ~f:(fun x ->
-          match x with
-          | State s -> Poly.equal s n.state
-          | HideConsecutiveSuccess -> Poly.equal Success n.state
-          | _ -> false)
-      with
-      | false -> Lwt.return []
-      | true ->
-      match List.exists ~f:id [ Rule.Status.hide_cancelled n cfg; Rule.Status.hide_success n ctx ] with
-      | true -> Lwt.return []
-      | false ->
-      match cfg.status_rules.title with
-      | None -> get_commit_info ()
-      | Some status_filter ->
-      match List.exists status_filter ~f:(String.equal n.context) with
-      | false -> Lwt.return []
-      | true -> get_commit_info ()
-    in
-    res
+    if Context.is_pipeline_allowed ctx ~pipeline then begin
+      match Rule.Status.match_rules ~rules n with
+      | Some Ignore | None -> Lwt.return []
+      | Some Allow -> action_on_match n.branches
+      | Some Allow_once ->
+      match Map.find ctx.state.pipeline_statuses pipeline with
+      | Some branch_statuses ->
+        let has_same_status_state_as_prev (branch : branch) =
+          match Map.find branch_statuses branch.name with
+          | None -> false
+          | Some state -> Poly.equal state current_status
+        in
+        let branches = List.filter n.branches ~f:(Fn.non @@ has_same_status_state_as_prev) in
+        action_on_match branches
+      | None -> action_on_match n.branches
+    end
+    else Lwt.return []
 
   let partition_commit_comment (ctx : Context.t) n =
     let cfg = Context.get_config_exn ctx in
@@ -213,9 +198,8 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
       let repo = Github.repo_of_notification notification in
       match%lwt Github_api.get_config ~ctx ~repo with
       | Ok config ->
-        print_config ctx;
-        (* can remove this wrapper once status_rules doesn't depend on Config.t *)
-        ctx.config <- Some (Config.make config);
+        Context.print_config ctx;
+        ctx.config <- Some config;
         Lwt.return @@ Ok ()
       | Error e -> action_error e
     in
@@ -243,7 +227,11 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
           let%lwt () = send_notifications ctx notifications in
           ( match ctx.state_filepath with
           | None -> Lwt.return_unit
-          | Some path -> Lwt.return @@ State.save path ctx.state
+          | Some path ->
+            ( match%lwt State.save ctx.state path with
+            | Ok () -> Lwt.return_unit
+            | Error e -> action_error e
+            )
           )
         )
     with

lib/common.ml

@@ -22,18 +22,6 @@ let first_line s =
   | x :: _ -> x
   | [] -> s
 
-module Tristate : Atdgen_runtime.Json_adapter.S = struct
-  let normalize = function
-    | `Bool true -> `String "true"
-    | `Bool false -> `String "false"
-    | x -> x
-
-  let restore = function
-    | `String "true" -> `Bool true
-    | `String "false" -> `Bool false
-    | x -> x
-end
-
 let decode_string_pad s =
   String.rstrip ~drop:(List.mem [ '='; ' '; '\n'; '\r'; '\t' ] ~equal:Char.equal) s |> Base64.decode_string
 

lib/config.atd

@@ -1,11 +1,11 @@
-type status_state <ocaml from="Rule"> = abstract
+type status_rule <ocaml from="Rule"> = abstract
 type prefix_rule <ocaml from="Rule"> = abstract
 type label_rule <ocaml from="Rule"> = abstract
 
 (* This type of rule is used for CI build notifications. *)
 type status_rules = {
   ?allowed_pipelines : string list nullable; (* keep only status events with a title matching this list *)
-  rules: status_state;
+  rules: status_rule list;
 }
 
 (* This type of rule is used for events that must be routed based on the