Skip to content

Commit 703ceab

Browse files
thatportugueseguythatportugueseguy
committed
Revert "DM only when user breaks a step (#160)"
This reverts commit 8774ef9.
1 parent 8774ef9 commit 703ceab

27 files changed

+193
-1842
lines changed

‎lib/action.ml

Lines changed: 20 additions & 32 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 (Util.first_line c.message);
61+
if skip then log#info "main branch merge, ignoring %s: %s" c.id (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,16 +128,12 @@ 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-
133131
let partition_status (ctx : Context.t) (n : status_notification) =
134132
let repo = n.repository in
135133
let cfg = Context.find_repo_config_exn ctx repo.url in
136134
let pipeline = n.context in
137135
let current_status = n.state in
138136
let rules = cfg.status_rules.rules in
139-
let repo_state = State.find_or_add_repo ctx.state n.repository.url in
140-
141137
let action_on_match (branches : branch list) ~notify_channels ~notify_dm =
142138
let%lwt direct_message =
143139
if notify_dm then begin
@@ -163,10 +159,9 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
163159
match cfg.main_branch_name with
164160
| None -> Lwt.return default
165161
| Some main_branch_name ->
162+
(* non-main branch build notifications go to default channel to reduce spam in topic channels *)
166163
match List.exists (fun ({ name } : branch) -> String.equal name main_branch_name) branches with
167-
| false ->
168-
(* non-main branch build notifications go to default channel to reduce spam in topic channels *)
169-
Lwt.return default
164+
| false -> Lwt.return default
170165
| true ->
171166
let sha = n.commit.sha in
172167
(match%lwt Github_api.get_api_commit ~ctx ~repo ~sha with
@@ -175,36 +170,29 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
175170
in
176171
Lwt.return (direct_message @ chans)
177172
in
178-
179173
let%lwt recipients =
180174
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
181176
match Rule.Status.match_rules ~rules n with
182177
| Some (Ignore, _, _) | None -> Lwt.return []
183178
| Some (Allow, notify_channels, notify_dm) -> action_on_match n.branches ~notify_channels ~notify_dm
184179
| Some (Allow_once, notify_channels, notify_dm) ->
185-
let full_build_failed =
186-
n.state = Failure && Re2.matches buildkite_is_failed_re (Option.default "" n.description)
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)
187194
in
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)
195+
action_on_match branches ~notify_channels ~notify_dm
208196
end
209197
else Lwt.return []
210198
in
@@ -277,7 +265,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
277265
Lwt.return notifs
278266
| Status n ->
279267
let%lwt channels = partition_status ctx n in
280-
let notifs = List.map (generate_status_notification ctx cfg n) channels in
268+
let notifs = List.map (generate_status_notification cfg n) channels in
281269
Lwt.return notifs
282270

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

‎lib/api_remote.ml

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

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

‎lib/common.ml

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,32 @@ 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: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,10 @@ 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.mem pipeline allowed_pipelines -> false
82+
| Some allowed_pipelines when not @@ List.exists (String.equal pipeline) allowed_pipelines -> false
8383
| _ -> true
8484

8585
let refresh_secrets ctx =
86-
let open Util in
8786
let path = ctx.secrets_filepath in
8887
match Config_j.secrets_of_string (Std.input_file path) with
8988
| exception exn -> fmt_error ~exn "failed to read secrets from file %s" path
@@ -105,7 +104,7 @@ let refresh_state ctx =
105104
log#info "loading saved state from file %s" path;
106105
(* todo: extract state related parts to state.ml *)
107106
match State_j.state_of_string (Std.input_file path) with
108-
| exception exn -> Util.fmt_error ~exn "failed to read state from file %s" path
107+
| exception exn -> fmt_error ~exn "failed to read state from file %s" path
109108
| state -> Ok { ctx with state = { State.state } }
110109
end
111110
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 = Util.first_line commit.message in
52+
let title = Common.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: 15 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
open Printf
22
open Common
3-
open Util
43
open Mrkdwn
54
open Github_j
65
open Slack_j
@@ -241,18 +240,15 @@ let generate_push_notification notification channel =
241240

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

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
243+
let generate_status_notification (cfg : Config_t.config) (notification : status_notification) channel =
244+
let { commit; state; description; target_url; context; repository; _ } = notification in
247245
let ({ commit : inner_commit; sha; author; html_url; _ } : status_commit) = commit in
248246
let ({ message; _ } : inner_commit) = commit in
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)
247+
let is_buildkite = String.starts_with context ~prefix:"buildkite" in
248+
let color_info =
249+
match state with
250+
| Success -> "good"
251+
| _ -> "danger"
256252
in
257253
let description_info =
258254
match description with
@@ -266,25 +262,11 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
266262
(* Specific to buildkite *)
267263
match Re2.find_submatches_exn buildkite_description_re s with
268264
| [| Some _; Some build_nr; Some rest |] ->
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
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
286268
in
287-
Some (sprintf "*Description*: %s" text)
269+
Some (sprintf "*Description*: %s." text)
288270
in
289271
let commit_info =
290272
[
@@ -309,17 +291,6 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
309291
in
310292
[ sprintf "*%s*: %s" (pluralize ~suf:"es" ~len:(List.length branches) "Branch") (String.concat ", " branches) ]
311293
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
323294
let summary =
324295
let state_info =
325296
match state with
@@ -337,7 +308,7 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
337308
| Some target_url ->
338309
let default_summary =
339310
sprintf "<%s|[%s]> CI Build Status notification for <%s|%s>: %s" repository.url repository.full_name target_url
340-
pipeline state_info
311+
context state_info
341312
in
342313
if not is_buildkite then default_summary
343314
else (
@@ -350,9 +321,9 @@ let generate_status_notification (ctx : Context.t) (cfg : Config_t.config) (noti
350321
in
351322
match pipeline_url with
352323
| None -> default_summary
353-
| Some pipeline_url -> sprintf "<%s|[%s]>: Build %s for \"%s\"" pipeline_url pipeline state_info commit_message)
324+
| Some pipeline_url -> sprintf "<%s|[%s]>: Build %s for \"%s\"" pipeline_url context state_info commit_message)
354325
in
355-
let msg = String.concat "\n" @@ List.concat [ commit_info; branches_info; failed_steps_info ] in
326+
let msg = String.concat "\n" @@ List.concat [ commit_info; branches_info ] in
356327
let attachment =
357328
{
358329
empty_attachments with
@@ -397,5 +368,5 @@ let validate_signature ?(version = "v0") ?signing_key ~headers body =
397368
| None -> Error "unable to find header X-Slack-Request-Timestamp"
398369
| Some timestamp ->
399370
let basestring = Printf.sprintf "%s:%s:%s" version timestamp body in
400-
let expected_signature = Printf.sprintf "%s=%s" version (Util.sign_string_sha256 ~key ~basestring) in
371+
let expected_signature = Printf.sprintf "%s=%s" version (Common.sign_string_sha256 ~key ~basestring) in
401372
if String.equal expected_signature signature then Ok () else Error "signatures don't match"

‎lib/state.atd

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ type ci_commit = {
77
sha: string;
88
author: string;
99
commit_message: string;
10-
url: string;
1110
build_link: string option;
1211
last_updated: string;
1312
}

‎lib/state.ml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ 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;
3837
last_updated = notification.updated_at;
3938
build_link = notification.target_url;
4039
}
@@ -112,4 +111,4 @@ let save { state; _ } path =
112111
try
113112
Files.save_as path (fun oc -> output_string oc data);
114113
Ok ()
115-
with exn -> Util.fmt_error ~exn "failed to save state to file %s" path
114+
with exn -> fmt_error ~exn "failed to save state to file %s" path

‎lib/util.ml

Lines changed: 0 additions & 83 deletions
This file was deleted.

0 commit comments

Comments
 (0)