Skip to content

Commit d2f47e3

Browse files
thatportugueseguythatportugueseguy
authored
Handle steps state per build (#167)
* add builds map to pipeline state * cleanup and handle new state structure * Update and clean state management * handle out-of-order first notifications * fix tests
1 parent 0b27d0b commit d2f47e3

29 files changed

+994
-1034
lines changed

‎lib/action.ml

Lines changed: 58 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -148,20 +148,20 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
148148
let partition_status (ctx : Context.t) (n : status_notification) =
149149
let repo = n.repository in
150150
let cfg = Context.find_repo_config_exn ctx repo.url in
151-
let pipeline = n.context in
152-
let current_status = n.state in
151+
let context = n.context in
153152
let rules = cfg.status_rules.rules in
154-
let is_main_branch =
155-
match cfg.main_branch_name with
156-
| None -> false
157-
| Some main_branch -> List.exists (fun ({ name } : branch) -> String.equal name main_branch) n.branches
158-
in
153+
let repo_state = State.find_or_add_repo ctx.state repo.url in
159154
let action_on_match (branches : branch list) ~notify_channels ~notify_dm =
155+
let is_main_branch =
156+
match cfg.main_branch_name with
157+
| None -> false
158+
| Some main_branch -> List.exists (fun ({ name } : branch) -> String.equal name main_branch) n.branches
159+
in
160160
let%lwt direct_message =
161161
if notify_dm then begin
162162
match%lwt Slack_api.lookup_user ~ctx ~cfg ~email:n.commit.commit.author.email () with
163163
| Ok res ->
164-
State.set_repo_pipeline_commit ctx.state repo.url ~pipeline ~commit:n.sha;
164+
State.set_repo_pipeline_commit ctx.state n;
165165
(* To send a DM, channel parameter is set to the user id of the recipient *)
166166
Lwt.return [ Slack_user_id.to_channel_id res.user.id ]
167167
| Error e ->
@@ -175,30 +175,37 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
175175
| false, _ | _, [] -> Lwt.return []
176176
| _ ->
177177
(* non-main branch build notifications go to default channel to reduce spam in topic channels *)
178-
match cfg.main_branch_name, is_main_branch with
179-
| None, _ | _, false ->
178+
match is_main_branch with
179+
| false ->
180180
Lwt.return (Option.map_default (fun c -> [ Slack_channel.to_any c ]) [] cfg.prefix_rules.default_channel)
181-
| Some _, true ->
182-
let sha = n.commit.sha in
183-
(match%lwt Github_api.get_api_commit ~ctx ~repo ~sha with
181+
| true ->
182+
(match%lwt Github_api.get_api_commit ~ctx ~repo ~sha:n.commit.sha with
184183
| Error e -> action_error e
185184
| Ok commit ->
186185
let chans = partition_commit cfg commit.files in
187186
Lwt.return (List.map Slack_channel.to_any chans))
188187
in
188+
(* only notify the failed builds channels for full failed builds with new failed steps on the main branch *)
189189
let notify_failed_builds_channel =
190-
(* we only notify the failed builds channels for failed builds on the main branch *)
191-
Util.Build.is_failed_build n && Option.is_some cfg.status_rules.allowed_pipelines && is_main_branch
190+
is_main_branch
191+
&& Util.Build.is_failed_build n
192+
&& Util.Build.new_failed_steps n repo_state <> []
193+
&& Option.map_default
194+
(fun allowed_pipelines ->
195+
List.exists
196+
(fun { failed_builds_channel; name } -> name = context && Option.is_some failed_builds_channel)
197+
allowed_pipelines)
198+
false cfg.status_rules.allowed_pipelines
192199
in
193200
match notify_failed_builds_channel, cfg.status_rules.allowed_pipelines with
194-
| false, _ | true, None -> Lwt.return (direct_message @ chans)
201+
| false, _ | _, None -> Lwt.return (direct_message @ chans)
195202
| true, Some allowed_pipelines ->
196-
(* if we have a failed build and a failed builds channel, we send one notification there too,
203+
(* if we have a failed builds channel configured, we send one notification there too,
197204
but we don't notify the same channel twice *)
198205
let chans =
199206
List.find_map
200207
(fun ({ name; failed_builds_channel } : Config_t.pipeline) ->
201-
match String.equal name n.context, failed_builds_channel with
208+
match String.equal name context, failed_builds_channel with
202209
| true, Some failed_builds_channel -> Some (Slack_channel.to_any failed_builds_channel :: chans)
203210
| _ -> None)
204211
allowed_pipelines
@@ -208,32 +215,50 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
208215
Lwt.return (direct_message @ chans)
209216
in
210217
let%lwt recipients =
211-
if Context.is_pipeline_allowed ctx repo.url ~pipeline then begin
212-
let repo_state = State.find_or_add_repo ctx.state repo.url in
218+
if Context.is_pipeline_allowed ctx repo.url ~context then begin
213219
match Rule.Status.match_rules ~rules n with
214220
| Some (Ignore, _, _) | None -> Lwt.return []
215221
| Some (Allow, notify_channels, notify_dm) -> action_on_match n.branches ~notify_channels ~notify_dm
216222
| Some (Allow_once, notify_channels, notify_dm) ->
217223
let branches =
218-
match StringMap.find_opt pipeline repo_state.pipeline_statuses with
219-
| Some branch_statuses ->
220-
let has_same_status_as_prev (branch : branch) =
221-
match StringMap.find_opt branch.name branch_statuses with
222-
| Some { status; _ } when status = current_status -> true
223-
| _ -> false
224-
in
225-
let branches = List.filter (Fun.negate has_same_status_as_prev) n.branches in
226-
branches
224+
match n.target_url with
227225
| None -> n.branches
226+
| Some build_url ->
227+
let pipeline_name =
228+
(* We only need to track messages for the base pipeline, not the steps *)
229+
match Util.Build.parse_context ~context ~build_url with
230+
| Ok { Util.Build.pipeline_name; _ } -> pipeline_name
231+
| Error _ -> context
232+
in
233+
(match StringMap.find_opt pipeline_name repo_state.pipeline_statuses with
234+
| None ->
235+
(* this is the first notification for a pipeline, so no need to filter branches *)
236+
n.branches
237+
| Some branch_statuses ->
238+
let has_same_status (branch : branch) =
239+
match StringMap.find_opt branch.name branch_statuses with
240+
| Some build_statuses ->
241+
let current = Util.Build.get_build_number_exn ~context ~build_url in
242+
let previous_builds = StringMap.filter (fun build_num _ -> build_num < current) build_statuses in
243+
(match StringMap.is_empty previous_builds with
244+
| true ->
245+
(* if we have no previous builds, it means they were successful and cleaned from state *)
246+
n.state = Github_t.Success
247+
| false ->
248+
let _, previous_build = StringMap.max_binding previous_builds in
249+
previous_build.status = n.state)
250+
| None ->
251+
(* if we don't have any builds for this branch yet, it's the first notification for this pipeline *)
252+
false
253+
in
254+
List.filter (Fun.negate has_same_status) n.branches)
228255
in
229-
let notify_dm =
230-
notify_dm && not (State.mem_repo_pipeline_commits ctx.state repo.url ~pipeline ~commit:n.sha)
231-
in
256+
let notify_dm = notify_dm && not (State.mem_repo_pipeline_commits ctx.state n) in
232257
action_on_match branches ~notify_channels ~notify_dm
233258
end
234259
else Lwt.return []
235260
in
236-
State.set_repo_pipeline_status ctx.state repo.url ~pipeline n;
261+
State.set_repo_pipeline_status ctx.state n;
237262
Lwt.return recipients
238263

239264
let partition_commit_comment (ctx : Context.t) n =

‎lib/context.ml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,13 +76,13 @@ let hook_of_channel ctx channel_name =
7676
(** [is_pipeline_allowed ctx repo_url ~pipeline] returns [true] if [status_rules]
7777
doesn't define a whitelist of allowed pipelines in the config of [repo_url],
7878
or if the list contains [pipeline]; returns [false] otherwise. *)
79-
let is_pipeline_allowed ctx repo_url ~pipeline =
79+
let is_pipeline_allowed ctx repo_url ~context =
8080
match find_repo_config ctx repo_url with
8181
| None -> true
8282
| Some config ->
8383
match config.status_rules.allowed_pipelines with
8484
| Some allowed_pipelines
85-
when not @@ List.exists (fun (p : Config_t.pipeline) -> String.equal p.name pipeline) allowed_pipelines ->
85+
when not @@ List.exists (fun (p : Config_t.pipeline) -> String.equal p.name context) allowed_pipelines ->
8686
false
8787
| _ -> true
8888

‎lib/slack.ml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -398,16 +398,16 @@ let generate_status_notification ~(ctx : Context.t) ?slack_user_id (cfg : Config
398398
&& Option.map_default Slack_channel.(equal channel $ to_any) false failed_builds_channel)
399399
pipelines
400400
in
401-
match Util.Build.is_failed_build notification && is_failed_builds_channel with
401+
match Build.is_failed_build notification && is_failed_builds_channel with
402402
| false -> []
403403
| true ->
404404
let repo_state = State.find_or_add_repo ctx.state repository.url in
405405
let pipeline = notification.context in
406-
let slack_step_link (s, l) =
407-
let step = Stre.drop_prefix s (pipeline ^ "/") in
408-
Printf.sprintf "<%s|%s> " l step
406+
let slack_step_link (s : State_t.failed_step) =
407+
let step = Stre.drop_prefix s.name (pipeline ^ "/") in
408+
Printf.sprintf "<%s|%s> " s.build_url step
409409
in
410-
(match Build.new_failed_steps notification repo_state pipeline with
410+
(match Build.new_failed_steps notification repo_state with
411411
| [] -> []
412412
| steps -> [ sprintf "*Steps broken*: %s" (String.concat ", " (List.map slack_step_link steps)) ])
413413
in

‎lib/state.atd

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,22 +11,32 @@ type ci_commit = {
1111
sha: string;
1212
author: string;
1313
commit_message: string;
14-
build_link: string option;
15-
last_updated: string;
14+
}
15+
16+
type failed_step = {
17+
name: string;
18+
build_url: string;
1619
}
1720

1821
type build_status = {
1922
status: status_state;
20-
?original_failed_commit:ci_commit nullable;
21-
?current_failed_commit:ci_commit nullable;
23+
build_number: string;
24+
build_url: string;
25+
commit: ci_commit;
26+
is_finished: bool;
27+
failed_steps: failed_step list;
28+
created_at: string;
29+
finished_at: string nullable;
2230
}
2331

24-
(* A map from branch names to build statuses *)
25-
type branch_statuses = build_status map_as_object
32+
(* A map from builds numbers to build statuses *)
33+
type build_statuses = build_status map_as_object
34+
35+
(* A map from branch names to [build_statuses] maps *)
36+
type branch_statuses = build_statuses map_as_object
2637

27-
(* A map from pipeline names to [branch_statuses] maps. This tracks the
28-
last build state matched by the status_rules for each pipeline and
29-
branch *)
38+
(* A map from pipeline names to [branch_statuses] maps.
39+
This tracks the last build state matched by the status_rules for each pipeline and branch *)
3040
type pipeline_statuses = branch_statuses map_as_object
3141

3242
type commit_sets = {

0 commit comments

Comments
 (0)