Merge remote-tracking branch 'origin/master' into pr-reviews-in-threads

* origin/master:
  Revert "DM only when user breaks a step (#160)"

lib/action.ml

@@ -56,7 +56,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
     n.commits
     |> List.filter (fun c ->
            let skip = Github.is_merge_commit_to_ignore ~cfg ~branch c in
-           if skip then log#info "main branch merge, ignoring %s: %s" c.id (Util.first_line c.message);
+           if skip then log#info "main branch merge, ignoring %s: %s" c.id (first_line c.message);
            not skip)
     |> List.concat_map (fun commit ->
            let rules = List.filter (filter_by_branch ~distinct:commit.distinct) rules in
@@ -141,16 +141,12 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
     in
     if matched_channel_names = [] then default else matched_channel_names
 
-  let buildkite_is_failed_re = Re2.create_exn {|^Build #\d+ failed|}
-
   let partition_status (ctx : Context.t) (n : status_notification) =
     let repo = n.repository in
     let cfg = Context.find_repo_config_exn ctx repo.url in
     let pipeline = n.context in
     let current_status = n.state in
     let rules = cfg.status_rules.rules in
-    let repo_state = State.find_or_add_repo ctx.state n.repository.url in
-
     let action_on_match (branches : branch list) ~notify_channels ~notify_dm =
       let%lwt direct_message =
         if notify_dm then begin
@@ -176,10 +172,9 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
         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 (fun ({ name } : branch) -> String.equal name main_branch_name) branches with
-        | false ->
-          (* non-main branch build notifications go to default channel to reduce spam in topic channels *)
-          Lwt.return default
+        | false -> Lwt.return default
         | true ->
           let sha = n.commit.sha in
           (match%lwt Github_api.get_api_commit ~ctx ~repo ~sha with
@@ -188,36 +183,29 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
       in
       Lwt.return (direct_message @ chans)
     in
-
     let%lwt recipients =
       if Context.is_pipeline_allowed ctx repo.url ~pipeline then begin
+        let repo_state = State.find_or_add_repo ctx.state repo.url in
         match Rule.Status.match_rules ~rules n with
         | Some (Ignore, _, _) | None -> Lwt.return []
         | Some (Allow, notify_channels, notify_dm) -> action_on_match n.branches ~notify_channels ~notify_dm
         | Some (Allow_once, notify_channels, notify_dm) ->
-          let full_build_failed =
-            n.state = Failure && Re2.matches buildkite_is_failed_re (Option.default "" n.description)
+          let branches =
+            match StringMap.find_opt pipeline repo_state.pipeline_statuses with
+            | Some branch_statuses ->
+              let has_same_status_as_prev (branch : branch) =
+                match StringMap.find_opt branch.name branch_statuses with
+                | Some { status; _ } when status = current_status -> true
+                | _ -> false
+              in
+              let branches = List.filter (Fun.negate has_same_status_as_prev) n.branches in
+              branches
+            | None -> n.branches
+          in
+          let notify_dm =
+            notify_dm && not (State.mem_repo_pipeline_commits ctx.state repo.url ~pipeline ~commit:n.sha)
           in
-          (* for failing states, if we only allow one notification, it must be the final one, not an intermediate one *)
-          (match n.state <> Failure || full_build_failed with
-          | false -> Lwt.return []
-          | true ->
-            let branches =
-              match StringMap.find_opt pipeline repo_state.pipeline_statuses with
-              | Some branch_statuses ->
-                let has_same_status_as_prev (branch : branch) =
-                  match StringMap.find_opt branch.name branch_statuses with
-                  | Some { status; _ } when status = current_status -> true
-                  | _ -> false
-                in
-                let branches = List.filter (Fun.negate has_same_status_as_prev) n.branches in
-                branches
-              | None -> n.branches
-            in
-            let notify_dm =
-              notify_dm && not (State.mem_repo_pipeline_commits ctx.state repo.url ~pipeline ~commit:n.sha)
-            in
-            action_on_match branches ~notify_channels ~notify_dm)
+          action_on_match branches ~notify_channels ~notify_dm
       end
       else Lwt.return []
     in
@@ -299,7 +287,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
       Lwt.return notifs
     | Status n ->
       let%lwt channels = partition_status ctx n in
-      let notifs = List.map (generate_status_notification ctx cfg n) channels in
+      let notifs = List.map (generate_status_notification cfg n) channels in
       Lwt.return notifs
 
   let send_notifications (ctx : Context.t) notifications =

lib/api_remote.ml

@@ -1,7 +1,6 @@
 open Printf
 open Devkit
 open Common
-open Util
 
 module Github : Api.Github = struct
   let commits_url ~(repo : Github_t.repository) ~sha =

lib/common.ml

@@ -40,3 +40,32 @@ module Re2 = struct
   let wrap s = create_exn s
   let unwrap = Re2.to_string
 end
+
+open Devkit
+
+let fmt_error ?exn fmt =
+  Printf.ksprintf
+    (fun s ->
+      match exn with
+      | Some exn -> Error (s ^ " :  exn " ^ Exn.str exn)
+      | None -> Error s)
+    fmt
+
+let first_line s =
+  match String.split_on_char '\n' s with
+  | x :: _ -> x
+  | [] -> s
+
+let decode_string_pad s = Stre.rstrip ~chars:"= \n\r\t" s |> Base64.decode_exn ~pad:false
+
+let http_request ?headers ?body meth path =
+  let setup h =
+    Curl.set_followlocation h true;
+    Curl.set_maxredirs h 1
+  in
+  match%lwt Web.http_request_lwt ~setup ~ua:"monorobot" ~verbose:true ?headers ?body meth path with
+  | `Ok s -> Lwt.return @@ Ok s
+  | `Error e -> Lwt.return @@ Error e
+
+let sign_string_sha256 ~key ~basestring =
+  Cstruct.of_string basestring |> Nocrypto.Hash.SHA256.hmac ~key:(Cstruct.of_string key) |> Hex.of_cstruct |> Hex.show

lib/context.ml

@@ -79,11 +79,10 @@ let is_pipeline_allowed ctx repo_url ~pipeline =
   | None -> true
   | Some config ->
   match config.status_rules.allowed_pipelines with
-  | Some allowed_pipelines when not @@ List.mem pipeline allowed_pipelines -> false
+  | Some allowed_pipelines when not @@ List.exists (String.equal pipeline) allowed_pipelines -> false
   | _ -> true
 
 let refresh_secrets ctx =
-  let open Util in
   let path = ctx.secrets_filepath in
   match Config_j.secrets_of_string (Std.input_file path) with
   | exception exn -> fmt_error ~exn "failed to read secrets from file %s" path
@@ -105,7 +104,7 @@ let refresh_state ctx =
       log#info "loading saved state from file %s" path;
       (* todo: extract state related parts to state.ml *)
       match State_j.state_of_string (Std.input_file path) with
-      | exception exn -> Util.fmt_error ~exn "failed to read state from file %s" path
+      | exception exn -> fmt_error ~exn "failed to read state from file %s" path
       | state -> Ok { ctx with state = { State.state } }
     end
     else Ok ctx

lib/github.ml

@@ -49,7 +49,7 @@ let is_merge_commit_to_ignore ~(cfg : Config_t.config) ~branch commit =
       Merge remote-tracking branch 'origin/develop' into feature_branch
       Merge remote-tracking branch 'origin/develop' (the default message pattern generated by GitHub "Update with merge commit" button)
     *)
-    let title = Util.first_line commit.message in
+    let title = Common.first_line commit.message in
     begin
       match Re2.find_submatches_exn merge_commit_re title with
       | [| Some _; Some incoming_branch; receiving_branch |] ->

lib/slack.ml

@@ -1,6 +1,5 @@
 open Printf
 open Common
-open Util
 open Mrkdwn
 open Github_j
 open Slack_j
@@ -345,18 +344,15 @@ let generate_push_notification notification channel =
 
 let buildkite_description_re = Re2.create_exn {|^Build #(\d+)(.*)|}
 
-let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (notification : status_notification) channel
-    =
-  let { commit; state; description; target_url; context = pipeline; repository; _ } = notification in
+let generate_status_notification (cfg : Config_t.config) (notification : status_notification) channel =
+  let { commit; state; description; target_url; context; repository; _ } = notification in
   let ({ commit : inner_commit; sha; author; html_url; _ } : status_commit) = commit in
   let ({ message; _ } : inner_commit) = commit in
-  let repo_state = State.find_or_add_repo ctx.state repository.url in
-  let is_buildkite = String.starts_with pipeline ~prefix:"buildkite" in
-  let color_info = if state = Success then "good" else "danger" in
-  let new_failed_steps = Build.new_failed_steps notification repo_state pipeline in
-  let old_failed_steps =
-    Build.all_failed_steps notification repo_state pipeline
-    |> List.filter (fun (s, _) -> not @@ List.mem_assoc s new_failed_steps)
+  let is_buildkite = String.starts_with context ~prefix:"buildkite" in
+  let color_info =
+    match state with
+    | Success -> "good"
+    | _ -> "danger"
   in
   let description_info =
     match description with
@@ -370,25 +366,11 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
         (* Specific to buildkite *)
         match Re2.find_submatches_exn buildkite_description_re s with
         | [| Some _; Some build_nr; Some rest |] ->
-          let fail_info =
-            match old_failed_steps, new_failed_steps with
-            | [], _ ->
-              (* if we don't any have failed steps, or we only have new failed steps
-                 we just print the msg from the status notification. *)
-              ""
-            | _ :: _, [] ->
-              "\n\
-               These failing steps are from a previous commmit. Please merge the latest develop or contact the author."
-            | _ :: _, _ :: _ ->
-              "\nSome of the steps were broken in previous commit(s), but this commit also broke some step(s)."
-          in
-          (* We use a zero-width space \u{200B} to prevent slack from interpreting #XXXXXX as a color *)
-          sprintf "Build <%s|#\u{200B}%s>%s.%s" target_url build_nr rest fail_info
-        | _ | (exception _) ->
-          (* we either match on the first case or get an exception *)
-          s
+          (* We use a zero-with space \u{200B} to prevent slack from interpreting #XXXXXX as a color *)
+          sprintf "Build <%s|#\u{200B}%s>%s" target_url build_nr rest
+        | _ -> s
       in
-      Some (sprintf "*Description*: %s" text)
+      Some (sprintf "*Description*: %s." text)
   in
   let commit_info =
     [
@@ -413,17 +395,6 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
       in
       [ sprintf "*%s*: %s" (pluralize ~suf:"es" ~len:(List.length branches) "Branch") (String.concat ", " branches) ]
   in
-  let failed_steps_info =
-    let open Util.Slack in
-    let to_steps m ss =
-      match ss with
-      | [] -> []
-      | _ -> [ sprintf "*%s*: %s" m (String.concat ", " (List.map (slack_step_link notification.context) ss)) ]
-    in
-    let old_failed_steps' = to_steps "Failing steps" old_failed_steps in
-    let new_failed_steps' = to_steps "New steps broken" new_failed_steps in
-    old_failed_steps' @ new_failed_steps'
-  in
   let summary =
     let state_info =
       match state with
@@ -441,7 +412,7 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
     | Some target_url ->
       let default_summary =
         sprintf "<%s|[%s]> CI Build Status notification for <%s|%s>: %s" repository.url repository.full_name target_url
-          pipeline state_info
+          context state_info
       in
       if not is_buildkite then default_summary
       else (
@@ -454,9 +425,9 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
         in
         match pipeline_url with
         | None -> default_summary
-        | Some pipeline_url -> sprintf "<%s|[%s]>: Build %s for \"%s\"" pipeline_url pipeline state_info commit_message)
+        | Some pipeline_url -> sprintf "<%s|[%s]>: Build %s for \"%s\"" pipeline_url context state_info commit_message)
   in
-  let msg = String.concat "\n" @@ List.concat [ commit_info; branches_info; failed_steps_info ] in
+  let msg = String.concat "\n" @@ List.concat [ commit_info; branches_info ] in
   let attachment =
     {
       empty_attachments with
@@ -501,5 +472,5 @@ let validate_signature ?(version = "v0") ?signing_key ~headers body =
   | None -> Error "unable to find header X-Slack-Request-Timestamp"
   | Some timestamp ->
     let basestring = Printf.sprintf "%s:%s:%s" version timestamp body in
-    let expected_signature = Printf.sprintf "%s=%s" version (Util.sign_string_sha256 ~key ~basestring) in
+    let expected_signature = Printf.sprintf "%s=%s" version (Common.sign_string_sha256 ~key ~basestring) in
     if String.equal expected_signature signature then Ok () else Error "signatures don't match"

lib/state.atd

@@ -11,7 +11,6 @@ type ci_commit = {
   sha: string;
   author: string;
   commit_message: string;
-  url: string;
   build_link: string option;
   last_updated: string;
 }

lib/state.ml

@@ -34,7 +34,6 @@ let set_repo_pipeline_status { state } repo_url ~pipeline (notification : Github
             State_t.sha = notification.sha;
             author = notification.commit.commit.author.email;
             commit_message = notification.commit.commit.message;
-            url = notification.commit.html_url;
             last_updated = notification.updated_at;
             build_link = notification.target_url;
           }
@@ -144,4 +143,4 @@ let save { state; _ } path =
   try
     Files.save_as path (fun oc -> output_string oc data);
     Ok ()
-  with exn -> Util.fmt_error ~exn "failed to save state to file %s" path
+  with exn -> fmt_error ~exn "failed to save state to file %s" path