Skip to content

Commit 8774ef9

Browse files
thatportugueseguythatportugueseguy
authored
DM only when user breaks a step (#160)
* notify only for steps that the user broke * extract utility functions to own module * update notification logic * little cleanup * update/create mock payloads and states for the tests * update state and fix test status.failure_test * show old and new failed steps on messages
1 parent 263c32b commit 8774ef9

27 files changed

+1842
-193
lines changed

‎lib/action.ml

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
5858
n.commits
5959
|> List.filter (fun c ->
6060
let skip = Github.is_merge_commit_to_ignore ~cfg ~branch c in
61-
if skip then log#info "main branch merge, ignoring %s: %s" c.id (first_line c.message);
61+
if skip then log#info "main branch merge, ignoring %s: %s" c.id (Util.first_line c.message);
6262
not skip)
6363
|> List.concat_map (fun commit ->
6464
let rules = List.filter (filter_by_branch ~distinct:commit.distinct) rules in
@@ -128,12 +128,16 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
128128
in
129129
if matched_channel_names = [] then default else matched_channel_names
130130

131+
let buildkite_is_failed_re = Re2.create_exn {|^Build #\d+ failed|}
132+
131133
let partition_status (ctx : Context.t) (n : status_notification) =
132134
let repo = n.repository in
133135
let cfg = Context.find_repo_config_exn ctx repo.url in
134136
let pipeline = n.context in
135137
let current_status = n.state in
136138
let rules = cfg.status_rules.rules in
139+
let repo_state = State.find_or_add_repo ctx.state n.repository.url in
140+
137141
let action_on_match (branches : branch list) ~notify_channels ~notify_dm =
138142
let%lwt direct_message =
139143
if notify_dm then begin
@@ -159,9 +163,10 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
159163
match cfg.main_branch_name with
160164
| None -> Lwt.return default
161165
| Some main_branch_name ->
162-
(* non-main branch build notifications go to default channel to reduce spam in topic channels *)
163166
match List.exists (fun ({ name } : branch) -> String.equal name main_branch_name) branches with
164-
| false -> Lwt.return default
167+
| false ->
168+
(* non-main branch build notifications go to default channel to reduce spam in topic channels *)
169+
Lwt.return default
165170
| true ->
166171
let sha = n.commit.sha in
167172
(match%lwt Github_api.get_api_commit ~ctx ~repo ~sha with
@@ -170,29 +175,36 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
170175
in
171176
Lwt.return (direct_message @ chans)
172177
in
178+
173179
let%lwt recipients =
174180
if Context.is_pipeline_allowed ctx repo.url ~pipeline then begin
175-
let repo_state = State.find_or_add_repo ctx.state repo.url in
176181
match Rule.Status.match_rules ~rules n with
177182
| Some (Ignore, _, _) | None -> Lwt.return []
178183
| Some (Allow, notify_channels, notify_dm) -> action_on_match n.branches ~notify_channels ~notify_dm
179184
| Some (Allow_once, notify_channels, notify_dm) ->
180-
let branches =
181-
match StringMap.find_opt pipeline repo_state.pipeline_statuses with
182-
| Some branch_statuses ->
183-
let has_same_status_as_prev (branch : branch) =
184-
match StringMap.find_opt branch.name branch_statuses with
185-
| Some { status; _ } when status = current_status -> true
186-
| _ -> false
187-
in
188-
let branches = List.filter (Fun.negate has_same_status_as_prev) n.branches in
189-
branches
190-
| None -> n.branches
191-
in
192-
let notify_dm =
193-
notify_dm && not (State.mem_repo_pipeline_commits ctx.state repo.url ~pipeline ~commit:n.sha)
185+
let full_build_failed =
186+
n.state = Failure && Re2.matches buildkite_is_failed_re (Option.default "" n.description)
194187
in
195-
action_on_match branches ~notify_channels ~notify_dm
188+
(* for failing states, if we only allow one notification, it must be the final one, not an intermediate one *)
189+
(match n.state <> Failure || full_build_failed with
190+
| false -> Lwt.return []
191+
| true ->
192+
let branches =
193+
match StringMap.find_opt pipeline repo_state.pipeline_statuses with
194+
| Some branch_statuses ->
195+
let has_same_status_as_prev (branch : branch) =
196+
match StringMap.find_opt branch.name branch_statuses with
197+
| Some { status; _ } when status = current_status -> true
198+
| _ -> false
199+
in
200+
let branches = List.filter (Fun.negate has_same_status_as_prev) n.branches in
201+
branches
202+
| None -> n.branches
203+
in
204+
let notify_dm =
205+
notify_dm && not (State.mem_repo_pipeline_commits ctx.state repo.url ~pipeline ~commit:n.sha)
206+
in
207+
action_on_match branches ~notify_channels ~notify_dm)
196208
end
197209
else Lwt.return []
198210
in
@@ -265,7 +277,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
265277
Lwt.return notifs
266278
| Status n ->
267279
let%lwt channels = partition_status ctx n in
268-
let notifs = List.map (generate_status_notification cfg n) channels in
280+
let notifs = List.map (generate_status_notification ctx cfg n) channels in
269281
Lwt.return notifs
270282

271283
let send_notifications (ctx : Context.t) notifications =

‎lib/api_remote.ml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
open Printf
22
open Devkit
33
open Common
4+
open Util
45

56
module Github : Api.Github = struct
67
let commits_url ~(repo : Github_t.repository) ~sha =

‎lib/common.ml

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -40,32 +40,3 @@ module Re2 = struct
4040
let wrap s = create_exn s
4141
let unwrap = Re2.to_string
4242
end
43-
44-
open Devkit
45-
46-
let fmt_error ?exn fmt =
47-
Printf.ksprintf
48-
(fun s ->
49-
match exn with
50-
| Some exn -> Error (s ^ " : exn " ^ Exn.str exn)
51-
| None -> Error s)
52-
fmt
53-
54-
let first_line s =
55-
match String.split_on_char '\n' s with
56-
| x :: _ -> x
57-
| [] -> s
58-
59-
let decode_string_pad s = Stre.rstrip ~chars:"= \n\r\t" s |> Base64.decode_exn ~pad:false
60-
61-
let http_request ?headers ?body meth path =
62-
let setup h =
63-
Curl.set_followlocation h true;
64-
Curl.set_maxredirs h 1
65-
in
66-
match%lwt Web.http_request_lwt ~setup ~ua:"monorobot" ~verbose:true ?headers ?body meth path with
67-
| `Ok s -> Lwt.return @@ Ok s
68-
| `Error e -> Lwt.return @@ Error e
69-
70-
let sign_string_sha256 ~key ~basestring =
71-
Cstruct.of_string basestring |> Nocrypto.Hash.SHA256.hmac ~key:(Cstruct.of_string key) |> Hex.of_cstruct |> Hex.show

‎lib/context.ml

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,11 @@ let is_pipeline_allowed ctx repo_url ~pipeline =
7979
| None -> true
8080
| Some config ->
8181
match config.status_rules.allowed_pipelines with
82-
| Some allowed_pipelines when not @@ List.exists (String.equal pipeline) allowed_pipelines -> false
82+
| Some allowed_pipelines when not @@ List.mem pipeline allowed_pipelines -> false
8383
| _ -> true
8484

8585
let refresh_secrets ctx =
86+
let open Util in
8687
let path = ctx.secrets_filepath in
8788
match Config_j.secrets_of_string (Std.input_file path) with
8889
| exception exn -> fmt_error ~exn "failed to read secrets from file %s" path
@@ -104,7 +105,7 @@ let refresh_state ctx =
104105
log#info "loading saved state from file %s" path;
105106
(* todo: extract state related parts to state.ml *)
106107
match State_j.state_of_string (Std.input_file path) with
107-
| exception exn -> fmt_error ~exn "failed to read state from file %s" path
108+
| exception exn -> Util.fmt_error ~exn "failed to read state from file %s" path
108109
| state -> Ok { ctx with state = { State.state } }
109110
end
110111
else Ok ctx

‎lib/github.ml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ let is_merge_commit_to_ignore ~(cfg : Config_t.config) ~branch commit =
4949
Merge remote-tracking branch 'origin/develop' into feature_branch
5050
Merge remote-tracking branch 'origin/develop' (the default message pattern generated by GitHub "Update with merge commit" button)
5151
*)
52-
let title = Common.first_line commit.message in
52+
let title = Util.first_line commit.message in
5353
begin
5454
match Re2.find_submatches_exn merge_commit_re title with
5555
| [| Some _; Some incoming_branch; receiving_branch |] ->

‎lib/slack.ml

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
open Printf
22
open Common
3+
open Util
34
open Mrkdwn
45
open Github_j
56
open Slack_j
@@ -240,15 +241,18 @@ let generate_push_notification notification channel =
240241

241242
let buildkite_description_re = Re2.create_exn {|^Build #(\d+)(.*)|}
242243

243-
let generate_status_notification (cfg : Config_t.config) (notification : status_notification) channel =
244-
let { commit; state; description; target_url; context; repository; _ } = notification in
244+
let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (notification : status_notification) channel
245+
=
246+
let { commit; state; description; target_url; context = pipeline; repository; _ } = notification in
245247
let ({ commit : inner_commit; sha; author; html_url; _ } : status_commit) = commit in
246248
let ({ message; _ } : inner_commit) = commit in
247-
let is_buildkite = String.starts_with context ~prefix:"buildkite" in
248-
let color_info =
249-
match state with
250-
| Success -> "good"
251-
| _ -> "danger"
249+
let repo_state = State.find_or_add_repo ctx.state repository.url in
250+
let is_buildkite = String.starts_with pipeline ~prefix:"buildkite" in
251+
let color_info = if state = Success then "good" else "danger" in
252+
let new_failed_steps = Build.new_failed_steps notification repo_state pipeline in
253+
let old_failed_steps =
254+
Build.all_failed_steps notification repo_state pipeline
255+
|> List.filter (fun (s, _) -> not @@ List.mem_assoc s new_failed_steps)
252256
in
253257
let description_info =
254258
match description with
@@ -262,11 +266,25 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_
262266
(* Specific to buildkite *)
263267
match Re2.find_submatches_exn buildkite_description_re s with
264268
| [| Some _; Some build_nr; Some rest |] ->
265-
(* We use a zero-with space \u{200B} to prevent slack from interpreting #XXXXXX as a color *)
266-
sprintf "Build <%s|#\u{200B}%s>%s" target_url build_nr rest
267-
| _ -> s
269+
let fail_info =
270+
match old_failed_steps, new_failed_steps with
271+
| [], _ ->
272+
(* if we don't any have failed steps, or we only have new failed steps
273+
we just print the msg from the status notification. *)
274+
""
275+
| _ :: _, [] ->
276+
"\n\
277+
These failing steps are from a previous commmit. Please merge the latest develop or contact the author."
278+
| _ :: _, _ :: _ ->
279+
"\nSome of the steps were broken in previous commit(s), but this commit also broke some step(s)."
280+
in
281+
(* We use a zero-width space \u{200B} to prevent slack from interpreting #XXXXXX as a color *)
282+
sprintf "Build <%s|#\u{200B}%s>%s.%s" target_url build_nr rest fail_info
283+
| _ | (exception _) ->
284+
(* we either match on the first case or get an exception *)
285+
s
268286
in
269-
Some (sprintf "*Description*: %s." text)
287+
Some (sprintf "*Description*: %s" text)
270288
in
271289
let commit_info =
272290
[
@@ -291,6 +309,17 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_
291309
in
292310
[ sprintf "*%s*: %s" (pluralize ~suf:"es" ~len:(List.length branches) "Branch") (String.concat ", " branches) ]
293311
in
312+
let failed_steps_info =
313+
let open Util.Slack in
314+
let to_steps m ss =
315+
match ss with
316+
| [] -> []
317+
| _ -> [ sprintf "*%s*: %s" m (String.concat ", " (List.map (slack_step_link notification.context) ss)) ]
318+
in
319+
let old_failed_steps' = to_steps "Failing steps" old_failed_steps in
320+
let new_failed_steps' = to_steps "New steps broken" new_failed_steps in
321+
old_failed_steps' @ new_failed_steps'
322+
in
294323
let summary =
295324
let state_info =
296325
match state with
@@ -308,7 +337,7 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_
308337
| Some target_url ->
309338
let default_summary =
310339
sprintf "<%s|[%s]> CI Build Status notification for <%s|%s>: %s" repository.url repository.full_name target_url
311-
context state_info
340+
pipeline state_info
312341
in
313342
if not is_buildkite then default_summary
314343
else (
@@ -321,9 +350,9 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_
321350
in
322351
match pipeline_url with
323352
| None -> default_summary
324-
| Some pipeline_url -> sprintf "<%s|[%s]>: Build %s for \"%s\"" pipeline_url context state_info commit_message)
353+
| Some pipeline_url -> sprintf "<%s|[%s]>: Build %s for \"%s\"" pipeline_url pipeline state_info commit_message)
325354
in
326-
let msg = String.concat "\n" @@ List.concat [ commit_info; branches_info ] in
355+
let msg = String.concat "\n" @@ List.concat [ commit_info; branches_info; failed_steps_info ] in
327356
let attachment =
328357
{
329358
empty_attachments with
@@ -368,5 +397,5 @@ let validate_signature ?(version = "v0") ?signing_key ~headers body =
368397
| None -> Error "unable to find header X-Slack-Request-Timestamp"
369398
| Some timestamp ->
370399
let basestring = Printf.sprintf "%s:%s:%s" version timestamp body in
371-
let expected_signature = Printf.sprintf "%s=%s" version (Common.sign_string_sha256 ~key ~basestring) in
400+
let expected_signature = Printf.sprintf "%s=%s" version (Util.sign_string_sha256 ~key ~basestring) in
372401
if String.equal expected_signature signature then Ok () else Error "signatures don't match"

‎lib/state.atd

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ type ci_commit = {
77
sha: string;
88
author: string;
99
commit_message: string;
10+
url: string;
1011
build_link: string option;
1112
last_updated: string;
1213
}

‎lib/state.ml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ let set_repo_pipeline_status { state } repo_url ~pipeline (notification : Github
3434
State_t.sha = notification.sha;
3535
author = notification.commit.commit.author.email;
3636
commit_message = notification.commit.commit.message;
37+
url = notification.commit.html_url;
3738
last_updated = notification.updated_at;
3839
build_link = notification.target_url;
3940
}
@@ -111,4 +112,4 @@ let save { state; _ } path =
111112
try
112113
Files.save_as path (fun oc -> output_string oc data);
113114
Ok ()
114-
with exn -> fmt_error ~exn "failed to save state to file %s" path
115+
with exn -> Util.fmt_error ~exn "failed to save state to file %s" path

‎lib/util.ml

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
open Devkit
2+
3+
module StringMap = Common.StringMap
4+
5+
let fmt_error ?exn fmt =
6+
Printf.ksprintf
7+
(fun s ->
8+
match exn with
9+
| Some exn -> Error (s ^ " : exn " ^ Exn.str exn)
10+
| None -> Error s)
11+
fmt
12+
13+
let first_line s =
14+
match String.split_on_char '\n' s with
15+
| x :: _ -> x
16+
| [] -> s
17+
18+
let decode_string_pad s = Stre.rstrip ~chars:"= \n\r\t" s |> Base64.decode_exn ~pad:false
19+
20+
let http_request ?headers ?body meth path =
21+
let setup h =
22+
Curl.set_followlocation h true;
23+
Curl.set_maxredirs h 1
24+
in
25+
match%lwt Web.http_request_lwt ~setup ~ua:"monorobot" ~verbose:true ?headers ?body meth path with
26+
| `Ok s -> Lwt.return @@ Ok s
27+
| `Error e -> Lwt.return @@ Error e
28+
29+
let sign_string_sha256 ~key ~basestring =
30+
Cstruct.of_string basestring |> Nocrypto.Hash.SHA256.hmac ~key:(Cstruct.of_string key) |> Hex.of_cstruct |> Hex.show
31+
32+
module Build = struct
33+
let new_failed_steps (n : Github_t.status_notification) (repo_state : State_t.repo_state) pipeline =
34+
let to_failed_steps branch step statuses acc =
35+
(* check if step of an allowed pipeline *)
36+
match step with
37+
| step when step = pipeline -> acc
38+
| step when not @@ Devkit.Stre.starts_with step pipeline -> acc
39+
| _ ->
40+
match StringMap.find_opt branch statuses with
41+
| Some (s : State_t.build_status) when s.status = Failure ->
42+
(match s.current_failed_commit, s.original_failed_commit with
43+
| Some _, _ ->
44+
(* if we have a value for current_failed_commit, this step was already failed and notified *)
45+
acc
46+
| None, Some { build_link = Some build_link; sha; url; _ } when sha = n.commit.sha ->
47+
(* we need to check the value of the commit sha to avoid false positives *)
48+
(step, (build_link, url)) :: acc
49+
| _ -> acc)
50+
| _ -> acc
51+
in
52+
match n.state = Failure, n.branches with
53+
| false, _ -> []
54+
| true, [ branch ] -> StringMap.fold (to_failed_steps branch.name) repo_state.pipeline_statuses []
55+
| true, _ -> []
56+
57+
let all_failed_steps (n : Github_t.status_notification) (repo_state : State_t.repo_state) pipeline =
58+
try
59+
(* notification.branches should always have _at least_ (and at most) one element,
60+
but we wrap this block in a try/with to make sure we don't raise from List.hd *)
61+
let branch = List.hd n.branches in
62+
StringMap.fold
63+
(fun step statuses acc ->
64+
(* check if step of an allowed pipeline *)
65+
match step with
66+
| step when step = pipeline -> acc
67+
| step when not @@ Devkit.Stre.starts_with step pipeline -> acc
68+
| _ ->
69+
match StringMap.find_opt branch.name statuses with
70+
| Some (s : State_t.build_status) when s.status = Failure ->
71+
(match s.original_failed_commit with
72+
| Some { build_link = Some build_link; url; _ } -> (step, (build_link, url)) :: acc
73+
| _ -> acc)
74+
| _ -> acc)
75+
repo_state.pipeline_statuses []
76+
with _e -> []
77+
end
78+
79+
module Slack = struct
80+
let slack_step_link pipeline (s, (l, url)) =
81+
let step = Devkit.Stre.drop_prefix s (pipeline ^ "/") in
82+
Printf.sprintf "<%s|%s> (<%s|gh>)" l step url
83+
end

0 commit comments

Comments
 (0)