diff --git a/lib/action.ml b/lib/action.ml index f78eb1d..8a6dd5b 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -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 @@ -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 @@ -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 = diff --git a/lib/slack.ml b/lib/slack.ml index 25c2773..15d850d 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -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 diff --git a/lib/state.atd b/lib/state.atd index 2250efd..431cec3 100644 --- a/lib/state.atd +++ b/lib/state.atd @@ -7,7 +7,7 @@ type user_id = string wrap type any_channel = string wrap -type ci_commit_new = { +type ci_commit = { sha: string; author: string; commit_message: string; @@ -18,11 +18,11 @@ 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; @@ -30,35 +30,13 @@ type build_status_new = { } (* 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 = { @@ -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 : pipeline_statuses_new; ~pipeline_statuses : pipeline_statuses; ~pipeline_commits : pipeline_commits; ~slack_threads : slack_threads; diff --git a/lib/state.ml b/lib/state.ml index f75771f..b70ade4 100644 --- a/lib/state.ml +++ b/lib/state.ml @@ -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 @@ -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 = @@ -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 @@ -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 @@ -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 diff --git a/lib/util.ml b/lib/util.ml index d3bb34b..c7ae622 100644 --- a/lib/util.ml +++ b/lib/util.ml @@ -1,4 +1,5 @@ open Devkit +open Common let fmt_error ?exn fmt = Printf.ksprintf @@ -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