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

@@ -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 ->

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"

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,29 +146,29 @@ 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
     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
     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
 

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 .../<base_pipeline_name>/builds/<build_number>.
-         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 .../<base_pipeline_name>/builds/<build_number>.
+       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