Skip to content

Commit

Permalink
cleanup and handle new state structure
Browse files Browse the repository at this point in the history
  • Loading branch information
@thatportugueseguy
thatportugueseguy committed Dec 6, 2024
1 parent cb21c1e commit b65b355
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 164 deletions.
56 changes: 23 additions & 33 deletions lib/action.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,13 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
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 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 action_on_match (branches : branch list) ~notify_channels ~notify_dm =
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%lwt direct_message =
if notify_dm then begin
match%lwt Slack_api.lookup_user ~ctx ~cfg ~email:n.commit.commit.author.email () with
Expand All @@ -175,25 +174,31 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
| false, _ | _, [] -> Lwt.return []
| _ ->
(* non-main branch build notifications go to default channel to reduce spam in topic channels *)
match cfg.main_branch_name, is_main_branch with
| None, _ | _, false ->
match is_main_branch with
| false ->
Lwt.return (Option.map_default (fun c -> [ Slack_channel.to_any c ]) [] cfg.prefix_rules.default_channel)
| Some _, true ->
let sha = n.commit.sha in
(match%lwt Github_api.get_api_commit ~ctx ~repo ~sha with
| true ->
(match%lwt Github_api.get_api_commit ~ctx ~repo ~sha:n.commit.sha with
| Error e -> action_error e
| Ok commit ->
let chans = partition_commit cfg commit.files in
Lwt.return (List.map Slack_channel.to_any chans))
in
(* only notify the failed builds channels for full failed builds on the main branch *)
let notify_failed_builds_channel =
(* we only notify the failed builds channels for failed builds on the main branch *)
Util.Build.is_failed_build n && Option.is_some cfg.status_rules.allowed_pipelines && is_main_branch
Util.Build.is_failed_build n
&& is_main_branch
&& Option.map_default
(fun allowed_pipelines ->
List.exists
(fun { failed_builds_channel; name } -> name = n.context && Option.is_some failed_builds_channel)
allowed_pipelines)
false cfg.status_rules.allowed_pipelines
in
match notify_failed_builds_channel, cfg.status_rules.allowed_pipelines with
| false, _ | true, None -> Lwt.return (direct_message @ chans)
| false, _ | _, None -> Lwt.return (direct_message @ chans)
| true, Some allowed_pipelines ->
(* if we have a failed build and a failed builds channel, we send one notification there too,
(* if we have a failed builds channel configured, we send one notification there too,
but we don't notify the same channel twice *)
let chans =
List.find_map
Expand All @@ -209,31 +214,16 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
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 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
let notify_dm = notify_dm && not (State.mem_repo_pipeline_commits ctx.state n) in
action_on_match n.branches ~notify_channels ~notify_dm
end
else Lwt.return []
in
State.set_repo_pipeline_status_new ctx.state n;
State.set_repo_pipeline_status ctx.state n;
Lwt.return recipients

let partition_commit_comment (ctx : Context.t) n =
Expand Down
8 changes: 4 additions & 4 deletions lib/slack.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,11 +403,11 @@ let generate_status_notification ~(ctx : Context.t) ?slack_user_id (cfg : Config
| true ->
let repo_state = State.find_or_add_repo ctx.state repository.url in
let pipeline = notification.context in
let slack_step_link (s, l) =
let step = Stre.drop_prefix s (pipeline ^ "/") in
Printf.sprintf "<%s|%s> " l step
let slack_step_link (s : State_t.failed_step) =
let step = Stre.drop_prefix s.name (pipeline ^ "/") in
Printf.sprintf "<%s|%s> " (Option.default "" s.build_link) step
in
(match Build.new_failed_steps notification repo_state pipeline with
(match Build.new_failed_steps notification repo_state with
| [] -> []
| steps -> [ sprintf "*Steps broken*: %s" (String.concat ", " (List.map slack_step_link steps)) ])
in
Expand Down
33 changes: 5 additions & 28 deletions lib/state.atd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ type user_id = string wrap <ocaml t="Common.Slack_user_id.t" wrap="Common.Slack_
type channel_id = string wrap <ocaml t="Common.Slack_channel.Ident.t" wrap="Common.Slack_channel.Ident.inject" unwrap="Common.Slack_channel.Ident.project">
type any_channel = string wrap <ocaml t="Common.Slack_channel.Any.t" wrap="Common.Slack_channel.Any.inject" unwrap="Common.Slack_channel.Any.project">

type ci_commit_new = {
type ci_commit = {
sha: string;
author: string;
commit_message: string;
Expand All @@ -18,47 +18,25 @@ type failed_step = {
build_link: string option;
}

type build_status_new = {
type build_status = {
status: status_state;
build_number: string;
build_link: string option;
commit: ci_commit_new;
commit: ci_commit;
is_finished: bool;
failed_steps: failed_step list;
created_at: string;
finished_at: string nullable;
}

(* A map from builds numbers to build statuses *)
type build_statuses = build_status_new map_as_object
type build_statuses = build_status map_as_object

(* A map from branch names to [build_statuses] maps *)
type branch_statuses_new = build_statuses map_as_object
type branch_statuses = build_statuses map_as_object

(* A map from pipeline names to [branch_statuses] maps.
This tracks the last build state matched by the status_rules for each pipeline and branch *)
type pipeline_statuses_new = branch_statuses_new map_as_object

type ci_commit = {
sha: string;
author: string;
commit_message: string;
build_link: string option;
last_updated: string;
}

type build_status = {
status: status_state;
?original_failed_commit:ci_commit nullable;
?current_failed_commit:ci_commit nullable;
}

(* A map from branch names to build statuses *)
type branch_statuses = build_status map_as_object

(* A map from pipeline names to [branch_statuses] maps. This tracks the
last build state matched by the status_rules for each pipeline and
branch *)
type pipeline_statuses = branch_statuses map_as_object

type commit_sets = {
Expand All @@ -80,7 +58,6 @@ type slack_threads = slack_thread list map_as_object

(* The runtime state of a given GitHub repository *)
type repo_state = {
~pipeline_statuses_new <ocaml mutable default="Common.StringMap.empty">: pipeline_statuses_new;
~pipeline_statuses <ocaml mutable default="Common.StringMap.empty">: pipeline_statuses;
~pipeline_commits <ocaml mutable default="Common.StringMap.empty">: pipeline_commits;
~slack_threads <ocaml mutable default="Common.StringMap.empty">: slack_threads;
Expand Down
97 changes: 18 additions & 79 deletions lib/state.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@ let log = Log.from "state"
type t = { state : State_t.state }

let empty_repo_state () : State_t.repo_state =
{
pipeline_statuses_new = StringMap.empty;
pipeline_statuses = StringMap.empty;
pipeline_commits = StringMap.empty;
slack_threads = StringMap.empty;
}
{ pipeline_statuses = StringMap.empty; pipeline_commits = StringMap.empty; slack_threads = StringMap.empty }

let empty () : t =
let state = State_t.{ repos = Stringtbl.empty (); bot_user_id = None } in
Expand All @@ -28,7 +23,7 @@ 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

let set_repo_pipeline_status_new { state } (n : Github_t.status_notification) =
let set_repo_pipeline_status { state } (n : Github_t.status_notification) =
let target_url = Option.get n.target_url in
let context = n.context in
let { Util.Build.is_pipeline_step; pipeline_name }, build_number =
Expand All @@ -55,8 +50,8 @@ let set_repo_pipeline_status_new { state } (n : Github_t.status_notification) =
in
let update_build_status builds_map build_number =
match StringMap.find_opt build_number builds_map with
| Some (current_build_status : State_t.build_status_new) ->
let is_finished = is_pipeline_step = false && (n.state = Success || n.state = Failure || n.state = Error) in
| Some (current_build_status : State_t.build_status) ->
let is_finished = (not is_pipeline_step) && (n.state = Success || n.state = Failure || n.state = Error) in
let finished_at = if is_finished then Some n.updated_at else None in
let failed_steps =
if is_pipeline_step && n.state = Failure then
Expand Down Expand Up @@ -92,75 +87,13 @@ let set_repo_pipeline_status_new { state } (n : Github_t.status_notification) =
| Success ->
(* if the build/step is successful, we remove it from the state to avoid the file to grow too much.
We only clean up on the whole pipeline, not on individual steps *)
(* TODO: what do we want here? Should it be taken care of by another function? *)
(* FIXME: this is not working as expected. We deleting all the pipeline statuses, rather than just the build that finished *)
(* TODO: distinguish between pipeline and step statuses. Pipeline success deletes the whole build,
while step success deletes the step *)
if not is_pipeline_step then
repo_state.pipeline_statuses_new <- StringMap.remove pipeline_name repo_state.pipeline_statuses_new
repo_state.pipeline_statuses <- StringMap.remove pipeline_name repo_state.pipeline_statuses
| _ ->
repo_state.pipeline_statuses_new <-
StringMap.update pipeline_name update_branch_status repo_state.pipeline_statuses_new

let set_repo_pipeline_status { state } repo_url ~pipeline (notification : Github_t.status_notification) =
let branches = notification.branches in
let set_branch_status per_branch_statuses =
let current_fail_state =
match notification.state with
| Failure | Error ->
Some
{
State_t.sha = notification.sha;
author = notification.commit.commit.author.email;
commit_message = notification.commit.commit.message;
last_updated = notification.updated_at;
build_link = notification.target_url;
}
| _ -> None
in
let initial_build_status_state =
{ State_t.status = notification.state; original_failed_commit = None; current_failed_commit = None }
in
let new_statuses =
List.map
(fun (branch : Github_t.branch) ->
let step_status =
Option.map_default
(fun pipeline_branches_statuses ->
match StringMap.find_opt branch.name pipeline_branches_statuses with
| Some (current_build_status : State_t.build_status) ->
let new_state = notification.state in
let original_failed_commit, current_failed_commit =
match new_state with
| Success -> None, None
| Pending ->
(* when new jobs are pending, we keep the existing state *)
current_build_status.original_failed_commit, current_build_status.current_failed_commit
| Failure | Error ->
(* if we don't have a failed step yet, set it *)
(* if we have a failed build and are retrying failed jobs: *)
(* - if we retried the original commit job, update the timestamp *)
(* - if we have a different commit that is failing that step, update the new failing commit *)
match current_build_status.original_failed_commit with
| None -> current_fail_state, None
| Some original_failed_commit ->
match original_failed_commit.sha = notification.sha with
| true -> current_fail_state, current_build_status.current_failed_commit
| false -> current_build_status.original_failed_commit, current_fail_state
in
{ State_t.status = new_state; original_failed_commit; current_failed_commit }
| None -> initial_build_status_state)
initial_build_status_state per_branch_statuses
in
branch.name, step_status)
branches
in
let init = Option.default StringMap.empty per_branch_statuses in
Some (List.fold_left (fun m (key, data) -> StringMap.add key data m) init new_statuses)
in
let repo_state = find_or_add_repo' state repo_url in
match notification.state with
| Success ->
(* if the build/step is successful, we remove it from the state to avoid the file to grow too much *)
repo_state.pipeline_statuses <- StringMap.remove pipeline repo_state.pipeline_statuses
| _ -> repo_state.pipeline_statuses <- StringMap.update pipeline set_branch_status repo_state.pipeline_statuses
repo_state.pipeline_statuses <- StringMap.update pipeline_name update_branch_status repo_state.pipeline_statuses

let set_repo_pipeline_commit { state } (n : Github_t.status_notification) =
let rotation_threshold = 1000 in
Expand All @@ -179,11 +112,17 @@ let set_repo_pipeline_commit { state } (n : Github_t.status_notification) =
in
repo_state.pipeline_commits <- StringMap.update pipeline set_commit repo_state.pipeline_commits

let mem_repo_pipeline_commits { state } repo_url ~pipeline ~commit =
let repo_state = find_or_add_repo' state repo_url in
let mem_repo_pipeline_commits { state } (n : Github_t.status_notification) =
let pipeline =
(* We only need to track messages for the base pipeline, not the steps *)
match Util.Build.parse_context ~context:n.context ~build_url:(Option.get n.target_url) with
| Ok { Util.Build.pipeline_name; _ } -> pipeline_name
| Error _ -> n.context
in
let repo_state = find_or_add_repo' state n.repository.url in
match StringMap.find_opt pipeline repo_state.pipeline_commits with
| None -> false
| Some { State_t.s1; s2 } -> StringSet.mem commit s1 || StringSet.mem commit s2
| 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
50 changes: 30 additions & 20 deletions lib/util.ml
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
open Devkit
open Common

let fmt_error ?exn fmt =
Printf.ksprintf
Expand Down Expand Up @@ -70,27 +71,36 @@ module Build = struct
let is_failed_build (n : Github_t.status_notification) =
n.state = Failure && Re2.matches buildkite_is_failed_re (Option.default "" n.description)

let new_failed_steps (n : Github_t.status_notification) (repo_state : State_t.repo_state) pipeline =
let to_failed_steps branch step statuses acc =
(* check if step of an allowed pipeline *)
match step with
| step when step = pipeline -> acc
| step when not @@ Devkit.Stre.starts_with step pipeline -> acc
| _ ->
match Common.StringMap.find_opt branch statuses with
| Some (s : State_t.build_status) when s.status = Failure ->
(match s.current_failed_commit, s.original_failed_commit with
| Some _, _ ->
(* if we have a value for current_failed_commit, this step was already failed and notified *)
acc
| None, Some { build_link = Some build_link; sha; _ } when sha = n.commit.sha ->
(* we need to check the value of the commit sha to avoid false positives *)
(step, build_link) :: acc
| _ -> acc)
| _ -> acc
in
let new_failed_steps (n : Github_t.status_notification) (repo_state : State_t.repo_state) =
let build_url = Option.get n.target_url in
let { pipeline_name = pipeline; _ } = Result.get_ok @@ parse_context ~context:n.context ~build_url in
match n.state = Failure, n.branches with
| false, _ -> []
| true, [ branch ] -> Common.StringMap.fold (to_failed_steps branch.name) repo_state.pipeline_statuses []
| true, [ branch ] ->
(match StringMap.find_opt pipeline repo_state.pipeline_statuses with
| Some branches_statuses ->
(match StringMap.find_opt branch.name branches_statuses with
| Some builds_maps ->
let current_build_number =
match get_build_number ~context:n.context ~build_url with
| Ok build_number -> build_number
| Error msg -> failwith msg
in
let to_previous_failed_steps n build_number (build_status : State_t.build_status) acc =
match int_of_string build_number >= n with
| true -> acc
| false -> build_status.failed_steps @ acc
in
let previous_failed_steps =
StringMap.fold (to_previous_failed_steps @@ int_of_string current_build_number) builds_maps []
|> List.sort_uniq Stdlib.compare
in
let current_build =
(* We assume that when this function is called, the current build is finished *)
Option.get @@ StringMap.find_opt current_build_number builds_maps
in
List.filter (fun step -> not @@ List.mem step previous_failed_steps) current_build.failed_steps
| None -> [])
| None -> [])
| true, _ -> []
end

0 comments on commit b65b355

Please sign in to comment.