From e162a5a1ea1bbf2815e87f997d3460807ddb679d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Nogueira?= Date: Tue, 17 Dec 2024 13:35:53 +0000 Subject: [PATCH] small cleanup on the code * remove unnecessary qualifications of Printf and Devkit * make comment in new_failed_steps fn clearer * remove unused `get_build_number` fn, refactor `get_build_number_exn` * update return type of `parse_context` --- lib/action.ml | 4 ++-- lib/slack.ml | 8 ++++---- lib/state.ml | 22 +++++++++++++--------- lib/util.ml | 46 ++++++++++++++++++++-------------------------- 4 files changed, 39 insertions(+), 41 deletions(-) diff --git a/lib/action.ml b/lib/action.ml index 35e42b2..1a893f0 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -227,8 +227,8 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct let pipeline_name = (* We only need to track messages for the base pipeline, not the steps *) match Util.Build.parse_context ~context ~build_url with - | Ok { Util.Build.pipeline_name; _ } -> pipeline_name - | Error _ -> context + | Some { Util.Build.pipeline_name; _ } -> pipeline_name + | None -> context in (match StringMap.find_opt pipeline_name repo_state.pipeline_statuses with | None -> diff --git a/lib/slack.ml b/lib/slack.ml index b981867..aa6d458 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -380,7 +380,7 @@ let generate_status_notification ~(ctx : Context.t) ?slack_user_id (cfg : Config let pipeline_url = match String.split_on_char '/' target_url with | "https:" :: "" :: "buildkite.com" :: org :: pipeline :: "builds" :: _ -> - Some (Printf.sprintf "https://buildkite.com/%s/%s" org pipeline) + Some (sprintf "https://buildkite.com/%s/%s" org pipeline) | _ -> None in match pipeline_url with @@ -405,7 +405,7 @@ let generate_status_notification ~(ctx : Context.t) ?slack_user_id (cfg : Config let pipeline = notification.context in let slack_step_link (s : State_t.failed_step) = let step = Stre.drop_prefix s.name (pipeline ^ "/") in - Printf.sprintf "<%s|%s> " s.build_url step + sprintf "<%s|%s> " s.build_url step in (match Build.new_failed_steps notification repo_state with | [] -> [] @@ -449,6 +449,6 @@ let validate_signature ?(version = "v0") ?signing_key ~headers body = match List.assoc_opt "x-slack-request-timestamp" headers with | None -> Error "unable to find header X-Slack-Request-Timestamp" | Some timestamp -> - let basestring = Printf.sprintf "%s:%s:%s" version timestamp body in - let expected_signature = Printf.sprintf "%s=%s" version (Util.sign_string_sha256 ~key ~basestring) in + let basestring = sprintf "%s:%s:%s" version timestamp body in + let expected_signature = sprintf "%s=%s" version (Util.sign_string_sha256 ~key ~basestring) in if String.equal expected_signature signature then Ok () else Error "signatures don't match" diff --git a/lib/state.ml b/lib/state.ml index 3d17cad..38ff355 100644 --- a/lib/state.ml +++ b/lib/state.ml @@ -56,7 +56,11 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) = let context = n.context in let { Util.Build.is_pipeline_step; pipeline_name } = Util.Build.parse_context_exn ~context ~build_url in let build_number = Util.Build.get_build_number_exn ~context ~build_url in - let is_finished = (not is_pipeline_step) && (n.state = Success || n.state = Failure || n.state = Error) in + let is_finished = + match is_pipeline_step, n.state with + | true, (Success | Failure | Error) -> true + | _ -> false + in let finished_at = match is_finished with | true -> Some n.updated_at @@ -142,11 +146,11 @@ let set_repo_pipeline_status { state } (n : Github_t.status_notification) = let set_repo_pipeline_commit { state } (n : Github_t.status_notification) = let rotation_threshold = 1000 in let repo_state = find_or_add_repo' state n.repository.url in - let pipeline = + let pipeline_name = (* We only need to track messages for the base pipeline, not the steps *) match Util.Build.parse_context ~context:n.context ~build_url:(Option.default "" n.target_url) with - | Ok { Util.Build.pipeline_name; _ } -> pipeline_name - | Error _ -> n.context + | Some { Util.Build.pipeline_name; _ } -> pipeline_name + | None -> n.context in let set_commit commits = let { State_t.s1; s2 } = Option.default { State_t.s1 = StringSet.empty; s2 = StringSet.empty } commits in @@ -154,17 +158,17 @@ let set_repo_pipeline_commit { state } (n : Github_t.status_notification) = let s1, s2 = if StringSet.cardinal s1 > rotation_threshold then StringSet.empty, s1 else s1, s2 in Some { State_t.s1; s2 } in - repo_state.pipeline_commits <- StringMap.update pipeline set_commit repo_state.pipeline_commits + 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 = + let pipeline_name = (* We only need to track messages for the base pipeline, not the steps *) match Util.Build.parse_context ~context:n.context ~build_url:(Option.default "" n.target_url) with - | Ok { Util.Build.pipeline_name; _ } -> pipeline_name - | Error _ -> n.context + | Some { Util.Build.pipeline_name; _ } -> pipeline_name + | None -> 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 + match StringMap.find_opt pipeline_name repo_state.pipeline_commits with | None -> false | Some { State_t.s1; s2 } -> StringSet.mem n.sha s1 || StringSet.mem n.sha s2 diff --git a/lib/util.ml b/lib/util.ml index b52364b..e0a9b6b 100644 --- a/lib/util.ml +++ b/lib/util.ml @@ -39,46 +39,40 @@ module Build = struct let parse_context ~context ~build_url = let rec get_name context build_url = match String.length context with - | 0 -> Error "failed to get pipeline name from notification - empty string" - | _ when Devkit.Stre.exists build_url (context ^ "/") -> + | 0 -> None + | _ when Stre.exists build_url (context ^ "/") -> (* We need to add the "/" to context to avoid partial matches between the context and the build_url *) - Ok context + Some context | _ -> (* Matches the notification context against the build_url to get the base pipeline name. Drop path levels from the context until we find a match with the build_url. This is the base name. *) match String.rindex_opt context '/' with | Some idx -> get_name (String.sub context 0 idx) build_url - | None -> - Error - (Printf.sprintf "failed to get pipeline name from notification. Context: %s, Build URL: %s" context build_url) + | None -> None in (* if we have a buildkite pipeline, we need to strip the `buildkite/` prefix to get the real name *) let context' = Stre.drop_prefix context "buildkite/" in let pipeline_name = get_name context' build_url in - Result.map (fun pipeline_name -> { is_pipeline_step = pipeline_name <> context'; pipeline_name }) pipeline_name + Option.map (fun pipeline_name -> { is_pipeline_step = pipeline_name <> context'; pipeline_name }) pipeline_name let parse_context_exn ~context ~build_url = + match String.length context with + | 0 -> failwith "failed to get pipeline name from notification - empty string" + | _ -> match parse_context ~context ~build_url with - | Ok c -> c - | Error msg -> failwith msg - - let get_build_number ~context ~build_url = - match parse_context ~context ~build_url with - | Error msg -> Error msg - | Ok { pipeline_name; _ } -> - (* build urls are in the form of ...//builds/. - Pipeline steps have an html anchor after the build number *) - let re = Re2.create_exn (Printf.sprintf ".*/%s/builds/(\\d+)#?" pipeline_name) in - (match Re2.find_submatches_exn re build_url with - | [| _; Some build_number |] -> Ok build_number - | _ | (exception _) -> - (* we either match on the first case or get an exception. *) - Error "failed to get build number from url") + | Some c -> c + | None -> + failwith + (Printf.sprintf "failed to get pipeline name from notification. Context: %s, Build URL: %s" context build_url) let get_build_number_exn ~context ~build_url = - match get_build_number ~context ~build_url with - | Ok n -> n - | Error msg -> failwith msg + let { pipeline_name; _ } = parse_context_exn ~context ~build_url in + (* build urls are in the form of ...//builds/. + Pipeline steps have an html anchor after the build number *) + let re = Re2.create_exn (Printf.sprintf ".*/%s/builds/(\\d+)#?" pipeline_name) in + match Re2.find_first_exn ~sub:(`Index 1) re build_url with + | build_number -> build_number + | exception _ -> failwith "failed to get build number from url" let is_failed_build (n : Github_t.status_notification) = n.state = Failure && Re2.matches buildkite_is_failed_re (Option.default "" n.description) @@ -109,7 +103,7 @@ module Build = struct |> List.sort_uniq Stdlib.compare in let current_build = - (* When this function is called, the current build is finished *) + (* We will not get an exception here because we checked that the build is failed and finished *) Option.get @@ StringMap.find_opt current_build_number builds_maps in List.filter