Skip to content

Commit

Permalink
refactor failed builds checks for DRY
Browse files Browse the repository at this point in the history
  • Loading branch information
@thatportugueseguy
thatportugueseguy committed Jan 30, 2025
1 parent 68787f1 commit 1e6ccd7
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 191 deletions.
85 changes: 22 additions & 63 deletions lib/action.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,8 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) (Buildkite_api :
let open Util.Build in
let repo = n.repository in
let cfg = Context.find_repo_config_exn ctx repo.url in
let context = n.context in
let repo_state = State.find_or_add_repo ctx.state repo.url in
let is_main_branch =
match cfg.main_branch_name with
| None -> false
| Some main_branch -> List.exists (fun ({ name } : branch) -> String.equal name main_branch) n.branches
in
let is_main_branch = is_main_branch cfg n in
let get_dm_id ~notify_dm =
let email = n.commit.commit.author.email in
match notify_dm with
Expand Down Expand Up @@ -207,16 +202,6 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) (Buildkite_api :
Lwt.return (List.map Status_notification.inject_channel chans))
in
let get_failed_builds_channel_id () =
let has_failed_builds_channel =
match cfg.status_rules.allowed_pipelines with
| None -> false
| Some allowed_pipelines ->
List.exists
(fun ({ failed_builds_channel; name; _ } : Config_t.pipeline) ->
is_main_branch && name = context && Option.is_some failed_builds_channel)
allowed_pipelines
in

(* only notify the failed builds channels for full failed or canceled builds on the main branch
that have failed steps that weren't failing in previous builds *)
let%lwt should_notify_fail =
Expand All @@ -229,20 +214,11 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) (Buildkite_api :

(* only notify the failed builds channels for successful builds on the main branch if they fix the pipeline *)
let should_notify_success =
match parse_context ~context with
| None -> false
| Some { pipeline_name; _ } ->
match n.target_url with
| None -> false
| Some build_url ->
match List.hd n.branches with
| exception _ -> false
| branch ->
let previous_build_is_failed =
match StringMap.find_opt pipeline_name repo_state.pipeline_statuses with
| None -> false
| Some branch_statuses ->
match StringMap.find_opt branch.name branch_statuses with
match get_branch_builds n repo_state with
| None -> false
| Some build_statuses ->
let current_build_number = get_build_number_exn ~build_url in
Expand All @@ -262,17 +238,12 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) (Buildkite_api :
let latest_build = previous_builds |> IntMap.max_binding |> snd in
latest_build.status = Failure)
in
is_success_build n && has_failed_builds_channel && previous_build_is_failed
is_success_build n && previous_build_is_failed && has_failed_builds_channel cfg n && is_main_branch
in
match should_notify_fail || should_notify_success, cfg.status_rules.allowed_pipelines with
| false, _ | _, None -> Lwt.return []
| true, Some allowed_pipelines ->
let to_failed_builds_channel_id ({ name; failed_builds_channel; _ } : Config_t.pipeline) =
match String.equal name context, failed_builds_channel with
| true, Some failed_builds_channel -> Some [ Status_notification.inject_channel failed_builds_channel ]
| _ -> None
in
Lwt.return @@ (List.find_map to_failed_builds_channel_id allowed_pipelines |> Option.default [])
match should_notify_fail || should_notify_success, get_pipeline_config cfg n with
| true, Some ({ failed_builds_channel = Some failed_builds_channel; _ } : Config_t.pipeline) ->
Lwt.return [ Status_notification.inject_channel failed_builds_channel ]
| false, _ | _, None | true, Some _ -> Lwt.return []
in
let action_on_match (branches : branch list) ~notify_channels ~notify_dm =
let%lwt direct_message = get_dm_id ~notify_dm in
Expand All @@ -293,34 +264,22 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) (Buildkite_api :
match n.target_url with
| None -> n.branches
| Some build_url ->
let pipeline_name =
(* We only want to track messages for the base pipeline, not the steps *)
match parse_context ~context with
| Some { pipeline_name; _ } -> pipeline_name
| None -> context
let status_switched (_ : branch) =
get_branch_builds n repo_state
|> Option.map_default
(fun (build_statuses : State_t.build_status IntMap.t) ->
let current = get_build_number_exn ~build_url in
let previous_builds = IntMap.filter (fun build_num _ -> build_num < current) build_statuses in
match IntMap.is_empty previous_builds with
| true ->
(* if we have no previous builds, it means they were successful and cleaned from state *)
n.state <> Github_t.Success
| false ->
let _, previous_build = IntMap.max_binding previous_builds in
previous_build.status <> n.state)
(n.state <> Github_t.Success)
in
(match StringMap.find_opt pipeline_name repo_state.pipeline_statuses with
| None ->
(* this is the first notification for a pipeline, so no need to filter branches *)
n.branches
| Some branch_statuses ->
let has_same_status (branch : branch) =
match StringMap.find_opt branch.name branch_statuses with
| Some build_statuses ->
let current = get_build_number_exn ~build_url in
let previous_builds = IntMap.filter (fun build_num _ -> build_num < current) build_statuses in
(match IntMap.is_empty previous_builds with
| true ->
(* if we have no previous builds, it means they were successful and cleaned from state *)
n.state = Github_t.Success
| false ->
let _, previous_build = IntMap.max_binding previous_builds in
previous_build.status = n.state)
| None ->
(* if we don't have any builds for this branch yet, it's the first notification for this pipeline *)
false
in
List.filter (Fun.negate has_same_status) n.branches)
List.filter status_switched n.branches
in
let notify_dm = notify_dm && not (State.mem_repo_pipeline_commits ctx.state n) in
action_on_match branches ~notify_channels ~notify_dm
Expand Down
7 changes: 1 addition & 6 deletions lib/context.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,7 @@ let hook_of_channel ctx channel_name =
let is_pipeline_allowed ctx repo_url (n : Github_t.status_notification) =
match find_repo_config ctx repo_url with
| None -> true
| Some config ->
match config.status_rules.allowed_pipelines with
| Some allowed_pipelines
when List.exists (fun (p : Config_t.pipeline) -> String.equal p.name n.context) allowed_pipelines ->
true
| _ -> false
| Some config -> Option.is_some (Util.Build.get_pipeline_config config n)

let refresh_secrets ctx =
let open Util in
Expand Down
33 changes: 12 additions & 21 deletions lib/slack.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,25 +308,20 @@ let generate_push_notification notification channel =

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

let generate_status_notification ?slack_user_id ?failed_steps (cfg : Config_t.config)
(notification : status_notification) channel =
let { commit; state; description; target_url; context; repository; _ } = notification in
let generate_status_notification ?slack_user_id ?failed_steps (cfg : Config_t.config) (n : status_notification) channel
=
let { commit; state; description; target_url; context; repository; _ } = n in
let ({ commit : inner_commit; sha; html_url; _ } : status_commit) = commit in
let ({ message; author; _ } : inner_commit) = commit in
let is_buildkite = String.starts_with context ~prefix:"buildkite" in
let is_failed_build_notification =
let is_failed_builds_channel =
match cfg.status_rules.allowed_pipelines with
| None -> false
| Some pipelines ->
List.exists
(fun ({ name; failed_builds_channel; _ } : Config_t.pipeline) ->
String.equal name context
&& Option.map_default Status_notification.(equal channel $ inject_channel) false failed_builds_channel)
pipelines
match Build.get_pipeline_config cfg n with
| Some ({ failed_builds_channel = Some failed_builds_channel; _ } : Config_t.pipeline) ->
Status_notification.(equal channel (inject_channel failed_builds_channel))
| None | Some _ -> false
in
Build.(is_failed_build notification || is_canceled_build notification)
&& (is_failed_builds_channel || Status_notification.is_user channel)
Build.(is_failed_build n || is_canceled_build n) && (is_failed_builds_channel || Status_notification.is_user channel)
in
let color_info = if state = Success then "good" else "danger" in
let build_desc =
Expand Down Expand Up @@ -354,7 +349,7 @@ let generate_status_notification ?slack_user_id ?failed_steps (cfg : Config_t.co
in
let commit_info = [ sprintf "*Commit*: `<%s|%s>` %s" html_url (git_short_sha_hash sha) author_mention ] in
let branches_info =
match List.map (fun ({ name } : branch) -> name) notification.branches with
match List.map (fun ({ name } : branch) -> name) n.branches with
| [] -> [] (* happens when branch is force-pushed by the time CI notification arrives *)
| notification_branches ->
let branches =
Expand Down Expand Up @@ -407,7 +402,7 @@ let generate_status_notification ?slack_user_id ?failed_steps (cfg : Config_t.co
match is_failed_build_notification with
| false -> Some (String.concat "\n" @@ List.concat [ commit_info; branches_info ])
| true ->
let pipeline = notification.context in
let pipeline = n.context in
let slack_step_link (s : Buildkite_t.failed_step) =
let step = Stre.drop_prefix s.name (pipeline ^ "/") in
sprintf "<%s|%s>" s.build_url step
Expand All @@ -432,13 +427,9 @@ let generate_commit_comment_notification ~slack_match_func api_commit notificati
(pp_link ~url:comment.html_url (git_short_sha_hash commit_id))
(first_line (Mrkdwn.escape_mrkdwn commit.message))
in
let path =
match comment.path with
| None -> None
| Some p -> Some (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url p)
in
let footer = Option.map (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url) comment.path in
make_message ~text:summary
?attachments:(format_attachments ~slack_match_func ~footer:path ~body:(Some comment.body))
?attachments:(format_attachments ~slack_match_func ~footer ~body:(Some comment.body))
~channel ()

let validate_signature ?(version = "v0") ?signing_key ~headers body =
Expand Down
103 changes: 45 additions & 58 deletions lib/state.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,28 @@ let find_or_add_repo' state repo_url =
let set_repo_state { state } repo_url repo_state = Stringtbl.replace state.repos repo_url repo_state
let find_or_add_repo { state } repo_url = find_or_add_repo' state repo_url

(** Updates the builds map in the branches statuses.
[default_builds_map] (optional, defaults to an empty map) is the builds map to use if we don't have the current
branch in the branches statuses.
[f] is the function to use to update the builds map. Takes the [State_t.build_status] Map for the current branch
as argument.
[branches_statuses] is the branches statuses Map to update. Returns the updated branches statuses Map. *)
let update_builds_in_branches ~branches ?(default_builds_map = IntMap.empty) ~f branches_statuses =
let current_statuses = Option.default StringMap.empty branches_statuses in
let updated_statuses =
List.map
(fun (branch : Github_t.branch) ->
let builds_map =
Option.map_default
(fun branches_statuses ->
match StringMap.find_opt branch.name branches_statuses with
| Some builds_map -> f builds_map
| None -> default_builds_map)
default_builds_map branches_statuses
in
branch.name, builds_map)
branches
let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
(* Updates the builds map in the branches statuses.
[default_builds_map] (optional, defaults to an empty map) is the builds map to use if we don't have the current
branch in the branches statuses.
[f] is the function to use to update the builds map. Takes the [State_t.build_status] Map for the current branch
as argument.
[branches_statuses] is the branches statuses Map to update. Returns the updated branches statuses Map. *)
let update_builds_in_branches ?(default_builds_map = IntMap.empty) ~f branches_statuses =
let current_statuses = Option.default StringMap.empty branches_statuses in
let repo_state = find_or_add_repo' state n.repository.url in
let updated_statuses =
List.map
(fun (branch : Github_t.branch) ->
( branch.name,
match Util.Build.get_branch_builds n repo_state with
| None -> default_builds_map
| Some builds_map -> f builds_map ))
n.branches
in
Some (List.fold_left (fun m (key, data) -> StringMap.add key data m) current_statuses updated_statuses)
in
Some (List.fold_left (fun m (key, data) -> StringMap.add key data m) current_statuses updated_statuses)

let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
match n.target_url with
| None ->
(* if we don't have a target_url value, we don't have a build number and cant't track build state *)
Expand All @@ -66,6 +63,7 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
| true -> Some (Timestamp.wrap_with_fallback n.updated_at)
| false -> None
in

(* Even if this is an initial build state, we can't just set an "empty" state because we don't know
the order we will get the status notifications in. *)
let init_build_state =
Expand All @@ -89,8 +87,10 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
finished_at;
}
in
let update_build_status builds_map build_number =
match IntMap.find_opt build_number builds_map with

let update_build_status () =
let repo_state = find_or_add_repo { state } n.repository.url in
match Util.Build.get_current_build n repo_state with
| None -> init_build_state
| Some ({ failed_steps; is_finished; _ } as current_build_status : State_t.build_status) ->
let failed_steps =
Expand All @@ -114,14 +114,16 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
failed_steps;
}
in

let update_pipeline_status =
update_builds_in_branches ~branches:n.branches
~default_builds_map:(IntMap.singleton build_number init_build_state) ~f:(fun builds_map ->
let updated_status = update_build_status builds_map build_number in
IntMap.add build_number updated_status builds_map)
let updated_status = update_build_status () in
update_builds_in_branches
~default_builds_map:(IntMap.singleton build_number init_build_state)
~f:(IntMap.add build_number updated_status)
in

let rm_successful_build =
update_builds_in_branches ~branches:n.branches ~f:(fun builds_map ->
update_builds_in_branches ~f:(fun builds_map ->
let threshold = Util.Build.stale_build_threshold in
let is_past_threshold (build_status : State_t.build_status) threshold =
let open Ptime in
Expand All @@ -144,8 +146,9 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
(* keep all other builds *)
true))
in

let rm_successful_step =
update_builds_in_branches ~branches:n.branches ~f:(fun builds_map ->
update_builds_in_branches ~f:(fun builds_map ->
IntMap.mapi
(fun build_number' (build_status : State_t.build_status) ->
let failed_steps =
Expand All @@ -158,6 +161,7 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
{ build_status with failed_steps })
builds_map)
in

let repo_state = find_or_add_repo' state n.repository.url in
(match n.state with
| Success ->
Expand All @@ -183,47 +187,30 @@ let set_repo_pipeline_commit { state } (n : Github_t.status_notification) =
pipeline_name
| None -> n.context
in
let to_branch_commits branches_commits (branch : Github_t.branch) =
let to_branch_commits (branch : Github_t.branch) =
let single_commit = { State_t.s1 = StringSet.add n.sha StringSet.empty; s2 = StringSet.empty } in
let updated_commits =
Option.map_default
(fun (branches_commits : State_t.commit_sets StringMap.t) ->
match StringMap.find_opt branch.name branches_commits with
| None -> single_commit
| Some branch_commits ->
let { State_t.s1; s2 } = branch_commits in
let s1 = StringSet.add n.sha s1 in
let s1, s2 = if StringSet.cardinal s1 > rotation_threshold then StringSet.empty, s1 else s1, s2 in
{ State_t.s1; s2 })
single_commit branches_commits
match Util.Build.get_branch_commits n repo_state with
| None -> single_commit
| Some { State_t.s1; s2 } ->
let s1 = StringSet.add n.sha s1 in
let s1, s2 = if StringSet.cardinal s1 > rotation_threshold then StringSet.empty, s1 else s1, s2 in
{ State_t.s1; s2 }
in
branch.name, updated_commits
in
let set_commit commits =
let current_commits = Option.default StringMap.empty commits in
let updated_commits = List.map (to_branch_commits commits) n.branches in
let updated_commits = List.map to_branch_commits n.branches in
Some (List.fold_left (fun m (key, data) -> StringMap.add key data m) current_commits updated_commits)
in
repo_state.pipeline_commits <- StringMap.update pipeline_name set_commit repo_state.pipeline_commits

let mem_repo_pipeline_commits { state } (n : Github_t.status_notification) =
let pipeline_name =
match Util.Build.parse_context ~context:n.context with
| Some { pipeline_name; _ } ->
(* We only want to track messages for the base pipeline, not the steps *)
pipeline_name
| None -> n.context
in
let repo_state = find_or_add_repo' state n.repository.url in
match StringMap.find_opt pipeline_name repo_state.pipeline_commits with
match Util.Build.get_branch_commits n repo_state with
| None -> false
| Some pipeline_commits ->
List.exists
(fun (b : Github_t.branch) ->
match StringMap.find_opt b.name pipeline_commits with
| None -> false
| Some { State_t.s1; s2 } -> StringSet.mem n.sha s1 || StringSet.mem n.sha s2)
n.branches
| Some { State_t.s1; s2 } -> StringSet.mem n.sha s1 || StringSet.mem n.sha s2

let has_pr_thread { state } ~repo_url ~pr_url =
let repo_state = find_or_add_repo' state repo_url in
Expand Down
Loading

0 comments on commit 1e6ccd7

Please sign in to comment.