From 3ea807bb485f6efcc48da92b30c4407fb27fe131 Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 13 Mar 2023 08:31:39 +0000 Subject: [PATCH 1/9] slack, api: add update message --- lib/api.ml | 3 ++- lib/api_local.ml | 26 ++++++++++++++++++++++++++ lib/api_remote.ml | 36 ++++++++++++++++++++++++++++++++++-- lib/slack.atd | 14 ++++++++++++++ src/monorobot.ml | 2 +- 5 files changed, 77 insertions(+), 4 deletions(-) diff --git a/lib/api.ml b/lib/api.ml index c1be62a0..a01cf91b 100644 --- a/lib/api.ml +++ b/lib/api.ml @@ -18,7 +18,8 @@ module type Github = sig end module type Slack = sig - val send_notification : ctx:Context.t -> msg:post_message_req -> unit slack_response Lwt.t + val send_notification : ctx:Context.t -> msg:post_message_req -> post_message_res slack_response Lwt.t + val update_notification : ctx:Context.t -> msg:update_message_req -> unit slack_response Lwt.t val send_chat_unfurl : ctx:Context.t -> diff --git a/lib/api_local.ml b/lib/api_local.ml index 524c241d..2319bec6 100644 --- a/lib/api_local.ml +++ b/lib/api_local.ml @@ -53,6 +53,7 @@ end (** The base implementation for local check payload debugging and mocking tests *) module Slack_base : Api.Slack = struct let send_notification ~ctx:_ ~msg:_ = Lwt.return @@ Error "undefined for local setup" + let update_notification ~ctx:_ ~msg:_ = Lwt.return @@ Error "undefined for local setup" let send_chat_unfurl ~ctx:_ ~channel:_ ~ts:_ ~unfurls:_ () = Lwt.return @@ Error "undefined for local setup" let send_auth_test ~ctx:_ () = Lwt.return @@ Error "undefined for local setup" end @@ -65,6 +66,14 @@ module Slack : Api.Slack = struct let json = msg |> Slack_j.string_of_post_message_req |> Yojson.Basic.from_string |> Yojson.Basic.pretty_to_string in Stdio.printf "will notify #%s\n" msg.channel; Stdio.printf "%s\n" json; + Lwt.return @@ Ok ({ channel = msg.channel; ts = "SOMETS" } : Slack_t.post_message_res) + + let update_notification ~ctx:_ ~msg = + let json = + msg |> Slack_j.string_of_update_message_req |> Yojson.Basic.from_string |> Yojson.Basic.pretty_to_string + in + Stdio.printf "will update msg in #%s\n" msg.channel; + Stdio.printf "%s\n" json; Lwt.return @@ Ok () let send_chat_unfurl ~ctx:_ ~channel ~ts ~unfurls () = @@ -91,6 +100,14 @@ module Slack_simple : Api.Slack = struct | None -> "" | Some s -> sprintf " with %S" s ); + Lwt.return @@ Ok ({ channel = msg.channel; ts = "SOMETS" } : Slack_t.post_message_res) + + let update_notification ~ctx:_ ~(msg : Slack_t.update_message_req) = + log#info "will update msg in #%s: %s\n" msg.channel + ( match msg.Slack_t.text with + | None -> "" + | Some s -> sprintf " with %S" s + ); Lwt.return @@ Ok () let send_chat_unfurl ~ctx:_ ~channel ~ts:_ ~(unfurls : Slack_t.message_attachment Common.StringMap.t) () = @@ -117,6 +134,15 @@ module Slack_json : Api.Slack = struct let url = Uri.add_query_param url ("msg", [ json ]) in log#info "%s" (Uri.to_string url); log#info "%s" json; + Lwt.return @@ Ok ({ channel = msg.channel; ts = "SOMETS" } : Slack_t.post_message_res) + + let update_notification ~ctx:_ ~(msg : Slack_t.update_message_req) = + log#info "will notify %s" msg.channel; + let json = Slack_j.string_of_update_message_req msg in + let url = Uri.of_string "https://api.slack.com/docs/messages/builder" in + let url = Uri.add_query_param url ("msg", [ json ]) in + log#info "%s" (Uri.to_string url); + log#info "%s" json; Lwt.return @@ Ok () let send_chat_unfurl ~ctx:_ ~channel ~ts:_ ~(unfurls : Slack_t.message_attachment Common.StringMap.t) () = diff --git a/lib/api_remote.ml b/lib/api_remote.ml index 525d527f..1c738055 100644 --- a/lib/api_remote.ml +++ b/lib/api_remote.ml @@ -139,12 +139,44 @@ module Slack : Api.Slack = struct log#info "data: %s" data; if webhook_mode then begin match%lwt http_request ~body ~headers `POST url with - | Ok _res -> Lwt.return @@ Ok () + | Ok res -> Lwt.return_ok @@ Slack_j.post_message_res_of_string res | Error e -> Lwt.return @@ build_error (query_error_msg url e) end else begin match%lwt slack_api_request ~body ~headers `POST url Slack_j.read_post_message_res with - | Ok _res -> Lwt.return @@ Ok () + | Ok res -> Lwt.return_ok res + | Error e -> Lwt.return @@ build_error e + end + + (** [update_notification ctx msg] update a message at [msg.ts] in [msg.channel] + with the payload [msg]; uses web API with access token if available, or with + webhook otherwise *) + let update_notification ~(ctx : Context.t) ~(msg : Slack_t.update_message_req) = + log#info "sending to %s" msg.channel; + let build_error e = fmt_error "%s\nfailed to send Slack notification" e in + let secrets = Context.get_secrets_exn ctx in + let headers, url, webhook_mode = + match Context.hook_of_channel ctx msg.channel with + | Some url -> [], Some url, true + | None -> + match secrets.slack_access_token with + | Some access_token -> [ bearer_token_header access_token ], Some "https://slack.com/api/chat.update", false + | None -> [], None, false + in + match url with + | None -> Lwt.return @@ build_error @@ sprintf "no token or webhook configured to notify channel %s" msg.channel + | Some url -> + let data = Slack_j.string_of_update_message_req msg in + let body = `Raw ("application/json", data) in + log#info "data: %s" data; + if webhook_mode then begin + match%lwt http_request ~body ~headers `POST url with + | Ok (_res : string) -> Lwt.return_ok () + | Error e -> Lwt.return @@ build_error (query_error_msg url e) + end + else begin + match%lwt slack_api_request ~body ~headers `POST url Slack_j.read_update_message_res with + | Ok (_res : Slack_t.update_message_res) -> Lwt.return_ok () | Error e -> Lwt.return @@ build_error e end diff --git a/lib/slack.atd b/lib/slack.atd index 285a12b8..acc741b4 100644 --- a/lib/slack.atd +++ b/lib/slack.atd @@ -65,6 +65,20 @@ type post_message_req = { type post_message_res = { channel: string; + ts: string; +} + +type update_message_req = { + channel: string; + ts: string; + ?text: string nullable; + ?attachments: message_attachment list nullable; + ?blocks: message_block list nullable; +} + +type update_message_res = { + channel: string; + ts: string; } type link_shared_link = { diff --git a/src/monorobot.ml b/src/monorobot.ml index b4aa1394..7fa9e6e6 100644 --- a/src/monorobot.ml +++ b/src/monorobot.ml @@ -57,7 +57,7 @@ let check_slack_action file secrets = | Error e -> log#error "%s" e; Lwt.return_unit - | Ok () -> Lwt.return_unit + | Ok (_res : Slack_t.post_message_res) -> Lwt.return_unit ) (* flags *) From 737863cd2ab21f6fd0309727dbec626dcd998f75 Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 13 Mar 2023 08:32:19 +0000 Subject: [PATCH 2/9] action, github: support edited comments update + add tests --- lib/action.ml | 37 ++- lib/github.atd | 2 +- lib/slack.ml | 224 ++++++++++-------- lib/state.atd | 8 +- lib/state.ml | 12 +- mock_states/issue_comment.edited.json | 17 ++ .../pull_request_review_comment.edited.json | 17 ++ .../status.state_hide_success_test.json | 8 +- ...hide_success_test_disallowed_pipeline.json | 8 +- test/slack_payloads.expected | 58 +++++ 10 files changed, 275 insertions(+), 116 deletions(-) create mode 100644 mock_states/issue_comment.edited.json create mode 100644 mock_states/pull_request_review_comment.edited.json diff --git a/lib/action.ml b/lib/action.ml index 9cc90492..16e91de1 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -58,12 +58,12 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct let partition_pr_review_comment cfg (n : pr_review_comment_notification) = match n.action with - | Created -> partition_label cfg n.pull_request.labels + | Created | Edited -> partition_label cfg n.pull_request.labels | _ -> [] let partition_issue_comment cfg (n : issue_comment_notification) = match n.action with - | Created -> partition_label cfg n.issue.labels + | Created | Edited -> partition_label cfg n.issue.labels | _ -> [] let partition_pr_review cfg (n : pr_review_notification) = @@ -186,13 +186,13 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | Pull_request n -> partition_pr cfg n |> List.map ~f:(generate_pull_request_notification n) |> Lwt.return | PR_review n -> partition_pr_review cfg n |> List.map ~f:(generate_pr_review_notification n) |> Lwt.return | PR_review_comment n -> - partition_pr_review_comment cfg n |> List.map ~f:(generate_pr_review_comment_notification n) |> Lwt.return + partition_pr_review_comment cfg n |> List.map ~f:(generate_pr_review_comment_notification ctx n) |> Lwt.return | Issue n -> partition_issue cfg n |> List.map ~f:(generate_issue_notification n) |> Lwt.return | Issue_comment n -> - partition_issue_comment cfg n |> List.map ~f:(generate_issue_comment_notification n) |> Lwt.return + partition_issue_comment cfg n |> List.map ~f:(generate_issue_comment_notification ctx n) |> Lwt.return | Commit_comment n -> let%lwt channels, api_commit = partition_commit_comment ctx n in - let notifs = List.map ~f:(generate_commit_comment_notification api_commit n) channels in + let notifs = List.map ~f:(generate_commit_comment_notification ctx api_commit n) channels in Lwt.return notifs | Status n -> let%lwt channels = partition_status ctx n in @@ -200,12 +200,27 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct Lwt.return notifs | _ -> Lwt.return [] - let send_notifications (ctx : Context.t) notifications = - let notify (msg : Slack_t.post_message_req) = - match%lwt Slack_api.send_notification ~ctx ~msg with - | Ok () -> Lwt.return_unit - | Error e -> action_error e + let send_notifications (ctx : Context.t) (req : Github.t) notifications = + let update_comment_mapping message = + match req with + | Github.PR_review_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message + | Issue_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message + | Commit_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message + | _ -> Lwt.return_unit in + let notify = function + | New_message (msg : Slack_t.post_message_req) -> + ( match%lwt Slack_api.send_notification ~ctx ~msg with + | Ok res -> update_comment_mapping res + | Error e -> action_error e + ) + | Update_message msg -> + ( match%lwt Slack_api.update_notification ~ctx ~msg with + | Ok () -> Lwt.return_unit + | Error e -> action_error e + ) + in + Lwt_list.iter_s notify notifications (** [refresh_repo_config ctx n] fetches the latest repo config if it's @@ -278,7 +293,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | Error e -> action_error e | Ok () -> let%lwt notifications = generate_notifications ctx payload in - let%lwt () = Lwt.join [ send_notifications ctx notifications; do_github_tasks ctx repo payload ] in + let%lwt () = Lwt.join [ send_notifications ctx payload notifications; do_github_tasks ctx repo payload ] in ( match ctx.state_filepath with | None -> Lwt.return_unit | Some path -> diff --git a/lib/github.atd b/lib/github.atd index 43c55fa4..dd354dae 100644 --- a/lib/github.atd +++ b/lib/github.atd @@ -287,7 +287,7 @@ type api_commit = { } type commit_comment_notification = { - action: string; + action: comment_action; repository: repository; comment: comment; sender: github_user; diff --git a/lib/slack.ml b/lib/slack.ml index bb22457e..5a81b676 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -5,6 +5,10 @@ open Mrkdwn open Github_j open Slack_j +type notify = + | New_message of Slack_t.post_message_req + | Update_message of Slack_t.update_message_req + let empty_attachments = { mrkdwn_in = None; @@ -55,21 +59,22 @@ let generate_pull_request_notification notification channel = sprintf "<%s|[%s]> Pull request #%d %s %s by *%s*" repository.url repository.full_name number (pp_link ~url:html_url title) action sender.login in - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt body; - }; - ]; - blocks = None; - } + New_message + { + channel; + text = Some summary; + attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = mrkdwn_of_markdown_opt body; + }; + ]; + blocks = None; + } let generate_pr_review_notification notification channel = let { action; sender; pull_request; review; repository } = notification in @@ -93,28 +98,29 @@ let generate_pr_review_notification notification channel = sprintf "<%s|[%s]> *%s* <%s|%s> #%d %s" repository.url repository.full_name sender.login review.html_url action_str number (pp_link ~url:html_url title) in - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt review.body; - }; - ]; - blocks = None; - } + New_message + { + channel; + text = Some summary; + attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = mrkdwn_of_markdown_opt review.body; + }; + ]; + blocks = None; + } -let generate_pr_review_comment_notification notification channel = +let generate_pr_review_comment_notification (ctx : Context.t) notification channel = let { action; pull_request; sender; comment; repository } = notification in let ({ number; title; html_url; _ } : pull_request) = pull_request in let action_str = match action with - | Created -> "commented" + | Created | Edited -> "commented" | _ -> invalid_arg (sprintf "Monorobot doesn't know how to generate notification for the unexpected event %s" @@ -130,22 +136,27 @@ let generate_pr_review_comment_notification notification channel = | None -> None | Some a -> Some (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url a) in - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - footer = file; - text = Some (mrkdwn_of_markdown comment.body); - }; - ]; - blocks = None; - } + let attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + footer = file; + text = Some (mrkdwn_of_markdown comment.body); + }; + ] + in + match action with + | Created -> New_message { channel; text = Some summary; attachments; blocks = None } + | Edited -> + ( match State.get_comment_map ctx.state repository.url comment.id with + | Some ({ channel; ts } : post_message_res) -> + Update_message { channel; ts; text = Some summary; attachments; blocks = None } + | None -> invalid_arg (sprintf "could not find comment %d in %s" comment.id repository.url) + ) + | _ -> invalid_arg "impossible" let generate_issue_notification notification channel = let ({ action; sender; issue; repository } : issue_notification) = notification in @@ -166,28 +177,29 @@ let generate_issue_notification notification channel = sprintf "<%s|[%s]> Issue #%d %s %s by *%s*" repository.url repository.full_name number (pp_link ~url:html_url title) action sender.login in - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt body; - }; - ]; - blocks = None; - } + New_message + { + channel; + text = Some summary; + attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = mrkdwn_of_markdown_opt body; + }; + ]; + blocks = None; + } -let generate_issue_comment_notification notification channel = +let generate_issue_comment_notification (ctx : Context.t) notification channel = let { action; issue; sender; comment; repository } = notification in let { number; title; _ } = issue in let action_str = match action with - | Created -> "commented" + | Created | Edited -> "commented" | _ -> invalid_arg (sprintf @@ -199,21 +211,26 @@ let generate_issue_comment_notification notification channel = sprintf "<%s|[%s]> *%s* <%s|%s> on #%d %s" repository.url repository.full_name sender.login comment.html_url action_str number (pp_link ~url:issue.html_url title) in - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = Some (mrkdwn_of_markdown comment.body); - }; - ]; - blocks = None; - } + let attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = Some (mrkdwn_of_markdown comment.body); + }; + ] + in + match action with + | Created -> New_message { channel; text = Some summary; attachments; blocks = None } + | Edited -> + ( match State.get_comment_map ctx.state repository.url comment.id with + | Some ({ channel; ts } : post_message_res) -> + Update_message { channel; ts; text = Some summary; attachments; blocks = None } + | None -> invalid_arg (sprintf "could not find comment %d in %s" comment.id repository.url) + ) + | _ -> invalid_arg "impossible" let git_short_sha_hash hash = String.sub ~pos:0 ~len:8 hash @@ -271,21 +288,22 @@ let generate_push_notification notification channel = sender.login in let commits = pp_list_with_previews ~pp_item:pp_commit commits in - { - channel; - text = Some title; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "fields" ]; - color = Some "#ccc"; - fields = Some [ { value = String.concat ~sep:"\n" commits; title = None; short = false } ]; - }; - ]; - blocks = None; - } + New_message + { + channel; + text = Some title; + attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "fields" ]; + color = Some "#ccc"; + fields = Some [ { value = String.concat ~sep:"\n" commits; title = None; short = false } ]; + }; + ]; + blocks = None; + } let generate_status_notification (cfg : Config_t.config) (notification : status_notification) channel = let { commit; state; description; target_url; context; repository; _ } = notification in @@ -347,11 +365,11 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_ fields = Some [ { title = None; value = msg; short = false } ]; } in - { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } + New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } -let generate_commit_comment_notification api_commit notification channel = +let generate_commit_comment_notification (ctx : Context.t) api_commit notification channel = let { commit; _ } = api_commit in - let { sender; comment; repository; _ } = notification in + let { sender; comment; repository; action; _ } = notification in let commit_id = match comment.commit_id with | None -> invalid_arg "commit id not found" @@ -376,7 +394,15 @@ let generate_commit_comment_notification api_commit notification channel = text = Some (mrkdwn_of_markdown comment.body); } in - { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } + match action with + | Created -> New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } + | Edited -> + ( match State.get_comment_map ctx.state repository.url comment.id with + | Some ({ channel; ts } : post_message_res) -> + Update_message { channel; ts; text = Some summary; attachments = Some [ attachment ]; blocks = None } + | None -> invalid_arg (sprintf "could not find comment %d in %s" comment.id repository.url) + ) + | _ -> invalid_arg "impossible" let validate_signature ?(version = "v0") ?signing_key ~headers body = match signing_key with diff --git a/lib/state.atd b/lib/state.atd index 86b18f06..1daacccc 100644 --- a/lib/state.atd +++ b/lib/state.atd @@ -10,13 +10,17 @@ type branch_statuses = status_state map_as_object branch *) type pipeline_statuses = branch_statuses map_as_object +type post_message_res = abstract +type slack_comment_map = post_message_res map_as_object + (* The runtime state of a given GitHub repository *) type repo_state = { - pipeline_statuses : pipeline_statuses + pipeline_statuses : pipeline_statuses; + comments_map : slack_comment_map; } (* The serializable runtime state of the bot *) type state = { repos : repo_state table_as_object; ?bot_user_id : string nullable; -} \ No newline at end of file +} diff --git a/lib/state.ml b/lib/state.ml index 6a53f361..74181d60 100644 --- a/lib/state.ml +++ b/lib/state.ml @@ -7,7 +7,7 @@ type t = { lock : Lwt_mutex.t; (** protect access to mutable string map `pipeline_statuses` *) } -let empty_repo_state () : State_t.repo_state = { pipeline_statuses = StringMap.empty } +let empty_repo_state () : State_t.repo_state = { pipeline_statuses = StringMap.empty; comments_map = StringMap.empty } let empty () : t = let state = State_t.{ repos = Stringtbl.empty (); bot_user_id = None } in @@ -34,6 +34,16 @@ let set_repo_pipeline_status { state; lock } repo_url ~pipeline ~(branches : Git repo_state.pipeline_statuses <- Map.update repo_state.pipeline_statuses pipeline ~f:set_branch_status; Lwt.return_unit +let add_comment_map { state; lock } repo_url comment_id post_message_res = + Lwt_mutex.with_lock lock @@ fun () -> + let repo_state = find_or_add_repo' state repo_url in + repo_state.comments_map <- Map.set repo_state.comments_map ~key:(Int.to_string comment_id) ~data:post_message_res; + Lwt.return_unit + +let get_comment_map { state; _ } repo_url comment_id = + let repo_state = find_or_add_repo' state repo_url in + Map.find repo_state.comments_map (Int.to_string comment_id) + let set_bot_user_id { state; _ } user_id = state.State_t.bot_user_id <- Some user_id let get_bot_user_id { state; _ } = state.State_t.bot_user_id let log = Log.from "state" diff --git a/mock_states/issue_comment.edited.json b/mock_states/issue_comment.edited.json new file mode 100644 index 00000000..75f9eb41 --- /dev/null +++ b/mock_states/issue_comment.edited.json @@ -0,0 +1,17 @@ + +{ + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } + }, + "comments_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } +} diff --git a/mock_states/pull_request_review_comment.edited.json b/mock_states/pull_request_review_comment.edited.json new file mode 100644 index 00000000..75f9eb41 --- /dev/null +++ b/mock_states/pull_request_review_comment.edited.json @@ -0,0 +1,17 @@ + +{ + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } + }, + "comments_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } +} diff --git a/mock_states/status.state_hide_success_test.json b/mock_states/status.state_hide_success_test.json index 0a4b7dec..75f9eb41 100644 --- a/mock_states/status.state_hide_success_test.json +++ b/mock_states/status.state_hide_success_test.json @@ -7,5 +7,11 @@ "buildkite/pipeline2": { "master": "success" } + }, + "comments_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } } -} \ No newline at end of file +} diff --git a/mock_states/status.state_hide_success_test_disallowed_pipeline.json b/mock_states/status.state_hide_success_test_disallowed_pipeline.json index 25b4592a..fce76c0e 100644 --- a/mock_states/status.state_hide_success_test_disallowed_pipeline.json +++ b/mock_states/status.state_hide_success_test_disallowed_pipeline.json @@ -3,5 +3,11 @@ "buildkite/pipeline2": { "master": "failure" } + }, + "comments_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } } -} \ No newline at end of file +} diff --git a/test/slack_payloads.expected b/test/slack_payloads.expected index 1e3c84fe..6a961b27 100644 --- a/test/slack_payloads.expected +++ b/test/slack_payloads.expected @@ -171,6 +171,34 @@ will notify #frontend-bot ] } ===== file ../mock_payloads/issue_comment.edited.json ===== +will update msg in #some channel +{ + "channel": "some channel", + "ts": "some ts", + "text": " *xinyuluo* on #4 ", + "attachments": [ + { + "fallback": null, + "mrkdwn_in": [ "text" ], + "color": "#ccc", + "text": "issue comment edited test" + } + ] +} +will update msg in #some channel +{ + "channel": "some channel", + "ts": "some ts", + "text": " *xinyuluo* on #4 ", + "attachments": [ + { + "fallback": null, + "mrkdwn_in": [ "text" ], + "color": "#ccc", + "text": "issue comment edited test" + } + ] +} ===== file ../mock_payloads/issues.closed.json ===== will notify #backend { @@ -354,6 +382,36 @@ will notify #backend } ===== file ../mock_payloads/pull_request_review_comment.deleted.json ===== ===== file ../mock_payloads/pull_request_review_comment.edited.json ===== +will update msg in #some channel +{ + "channel": "some channel", + "ts": "some ts", + "text": " *xinyuluo* commented on #4 ", + "attachments": [ + { + "fallback": null, + "mrkdwn_in": [ "text" ], + "color": "#ccc", + "text": "Edited test comment", + "footer": "New comment by xinyuluo in " + } + ] +} +will update msg in #some channel +{ + "channel": "some channel", + "ts": "some ts", + "text": " *xinyuluo* commented on #4 ", + "attachments": [ + { + "fallback": null, + "mrkdwn_in": [ "text" ], + "color": "#ccc", + "text": "Edited test comment", + "footer": "New comment by xinyuluo in " + } + ] +} ===== file ../mock_payloads/push.branch_filter_default.json ===== will notify #all-push-events { From fbbe2fdd43d7c63b1c46d9c9cc214181b7466da9 Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 13 Mar 2023 09:19:20 +0000 Subject: [PATCH 3/9] github, slack: support pr review updates --- lib/action.ml | 5 ++- lib/github.atd | 1 + lib/slack.ml | 40 ++++++++++--------- lib/state.atd | 5 ++- lib/state.ml | 13 +++++- mock_states/issue_comment.edited.json | 6 +++ mock_states/pull_request_review.edited.json | 23 +++++++++++ .../pull_request_review_comment.edited.json | 6 +++ .../status.state_hide_success_test.json | 6 +++ ...hide_success_test_disallowed_pipeline.json | 6 +++ test/slack_payloads.expected | 28 +++++++++++++ 11 files changed, 116 insertions(+), 23 deletions(-) create mode 100644 mock_states/pull_request_review.edited.json diff --git a/lib/action.ml b/lib/action.ml index 16e91de1..5dfac09b 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -79,7 +79,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct in both cases, since pull_request_review_comment is already handled by another type of event, information in the pull_request_review payload does not provide any insightful information and will thus be ignored. *) - | Submitted, _, _ -> partition_label cfg n.pull_request.labels + | (Submitted | Edited), _, _ -> partition_label cfg n.pull_request.labels | _ -> [] let partition_commit (cfg : Config_t.config) files = @@ -184,7 +184,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | Github.Push n -> partition_push cfg n |> List.map ~f:(fun (channel, n) -> generate_push_notification n channel) |> Lwt.return | Pull_request n -> partition_pr cfg n |> List.map ~f:(generate_pull_request_notification n) |> Lwt.return - | PR_review n -> partition_pr_review cfg n |> List.map ~f:(generate_pr_review_notification n) |> Lwt.return + | PR_review n -> partition_pr_review cfg n |> List.map ~f:(generate_pr_review_notification ctx n) |> Lwt.return | PR_review_comment n -> partition_pr_review_comment cfg n |> List.map ~f:(generate_pr_review_comment_notification ctx n) |> Lwt.return | Issue n -> partition_issue cfg n |> List.map ~f:(generate_issue_notification n) |> Lwt.return @@ -206,6 +206,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | Github.PR_review_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message | Issue_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message | Commit_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message + | PR_review n -> State.add_review_map ctx.state n.repository.url n.review.id message | _ -> Lwt.return_unit in let notify = function diff --git a/lib/github.atd b/lib/github.atd index dd354dae..e8a443b2 100644 --- a/lib/github.atd +++ b/lib/github.atd @@ -185,6 +185,7 @@ type pr_notification = { type review = { ?body: string nullable; + id: int; html_url: string; state: string; } diff --git a/lib/slack.ml b/lib/slack.ml index 5a81b676..644483d7 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -76,12 +76,12 @@ let generate_pull_request_notification notification channel = blocks = None; } -let generate_pr_review_notification notification channel = +let generate_pr_review_notification (ctx : Context.t) notification channel = let { action; sender; pull_request; review; repository } = notification in let ({ number; title; html_url; _ } : pull_request) = pull_request in let action_str = match action with - | Submitted -> + | Submitted | Edited -> ( match review.state with | "commented" -> "commented on" | "approved" -> "approved" @@ -98,22 +98,26 @@ let generate_pr_review_notification notification channel = sprintf "<%s|[%s]> *%s* <%s|%s> #%d %s" repository.url repository.full_name sender.login review.html_url action_str number (pp_link ~url:html_url title) in - New_message - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt review.body; - }; - ]; - blocks = None; - } + let attachments = + Some + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = mrkdwn_of_markdown_opt review.body; + }; + ] + in + match action with + | Submitted -> New_message { channel; text = Some summary; attachments; blocks = None } + | Edited -> + ( match State.get_review_map ctx.state repository.url review.id with + | Some ({ channel; ts } : post_message_res) -> + Update_message { channel; ts; text = Some summary; attachments; blocks = None } + | None -> invalid_arg (sprintf "could not find comment %d in %s" review.id repository.url) + ) + | _ -> invalid_arg "impossible" let generate_pr_review_comment_notification (ctx : Context.t) notification channel = let { action; pull_request; sender; comment; repository } = notification in diff --git a/lib/state.atd b/lib/state.atd index 1daacccc..3edd31f9 100644 --- a/lib/state.atd +++ b/lib/state.atd @@ -11,12 +11,13 @@ type branch_statuses = status_state map_as_object type pipeline_statuses = branch_statuses map_as_object type post_message_res = abstract -type slack_comment_map = post_message_res map_as_object +type post_msg_map = post_message_res map_as_object (* The runtime state of a given GitHub repository *) type repo_state = { pipeline_statuses : pipeline_statuses; - comments_map : slack_comment_map; + comments_map : post_msg_map; + reviews_map : post_msg_map; } (* The serializable runtime state of the bot *) diff --git a/lib/state.ml b/lib/state.ml index 74181d60..967df958 100644 --- a/lib/state.ml +++ b/lib/state.ml @@ -7,7 +7,8 @@ type t = { lock : Lwt_mutex.t; (** protect access to mutable string map `pipeline_statuses` *) } -let empty_repo_state () : State_t.repo_state = { pipeline_statuses = StringMap.empty; comments_map = StringMap.empty } +let empty_repo_state () : State_t.repo_state = + { pipeline_statuses = StringMap.empty; comments_map = StringMap.empty; reviews_map = StringMap.empty } let empty () : t = let state = State_t.{ repos = Stringtbl.empty (); bot_user_id = None } in @@ -44,6 +45,16 @@ let get_comment_map { state; _ } repo_url comment_id = let repo_state = find_or_add_repo' state repo_url in Map.find repo_state.comments_map (Int.to_string comment_id) +let add_review_map { state; lock } repo_url review_id post_message_res = + Lwt_mutex.with_lock lock @@ fun () -> + let repo_state = find_or_add_repo' state repo_url in + repo_state.reviews_map <- Map.set repo_state.reviews_map ~key:(Int.to_string review_id) ~data:post_message_res; + Lwt.return_unit + +let get_review_map { state; _ } repo_url review_id = + let repo_state = find_or_add_repo' state repo_url in + Map.find repo_state.reviews_map (Int.to_string review_id) + let set_bot_user_id { state; _ } user_id = state.State_t.bot_user_id <- Some user_id let get_bot_user_id { state; _ } = state.State_t.bot_user_id let log = Log.from "state" diff --git a/mock_states/issue_comment.edited.json b/mock_states/issue_comment.edited.json index 75f9eb41..571cc185 100644 --- a/mock_states/issue_comment.edited.json +++ b/mock_states/issue_comment.edited.json @@ -13,5 +13,11 @@ "channel": "some channel", "ts": "some ts" } + }, + "reviews_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } } } diff --git a/mock_states/pull_request_review.edited.json b/mock_states/pull_request_review.edited.json new file mode 100644 index 00000000..571cc185 --- /dev/null +++ b/mock_states/pull_request_review.edited.json @@ -0,0 +1,23 @@ + +{ + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } + }, + "comments_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + }, + "reviews_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } +} diff --git a/mock_states/pull_request_review_comment.edited.json b/mock_states/pull_request_review_comment.edited.json index 75f9eb41..571cc185 100644 --- a/mock_states/pull_request_review_comment.edited.json +++ b/mock_states/pull_request_review_comment.edited.json @@ -13,5 +13,11 @@ "channel": "some channel", "ts": "some ts" } + }, + "reviews_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } } } diff --git a/mock_states/status.state_hide_success_test.json b/mock_states/status.state_hide_success_test.json index 75f9eb41..571cc185 100644 --- a/mock_states/status.state_hide_success_test.json +++ b/mock_states/status.state_hide_success_test.json @@ -13,5 +13,11 @@ "channel": "some channel", "ts": "some ts" } + }, + "reviews_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } } } diff --git a/mock_states/status.state_hide_success_test_disallowed_pipeline.json b/mock_states/status.state_hide_success_test_disallowed_pipeline.json index fce76c0e..2516b176 100644 --- a/mock_states/status.state_hide_success_test_disallowed_pipeline.json +++ b/mock_states/status.state_hide_success_test_disallowed_pipeline.json @@ -9,5 +9,11 @@ "channel": "some channel", "ts": "some ts" } + }, + "reviews_map": { + "0": { + "channel": "some channel", + "ts": "some ts" + } } } diff --git a/test/slack_payloads.expected b/test/slack_payloads.expected index 6a961b27..9a494e55 100644 --- a/test/slack_payloads.expected +++ b/test/slack_payloads.expected @@ -335,6 +335,34 @@ will notify #frontend-bot } ===== file ../mock_payloads/pull_request_review.dismissed.json ===== ===== file ../mock_payloads/pull_request_review.edited.json ===== +will update msg in #some channel +{ + "channel": "some channel", + "ts": "some ts", + "text": " *xinyuluo* #4 ", + "attachments": [ + { + "fallback": null, + "mrkdwn_in": [ "text" ], + "color": "#ccc", + "text": "pr review comment test" + } + ] +} +will update msg in #some channel +{ + "channel": "some channel", + "ts": "some ts", + "text": " *xinyuluo* #4 ", + "attachments": [ + { + "fallback": null, + "mrkdwn_in": [ "text" ], + "color": "#ccc", + "text": "pr review comment test" + } + ] +} ===== file ../mock_payloads/pull_request_review.request_changes.json ===== will notify #default { From 4afb57bfc90f13acc49a93972434e13ed25db07a Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 20 Mar 2023 09:42:40 +0000 Subject: [PATCH 4/9] state, slack: map comments + reviews to its issue --- lib/action.ml | 12 ++- lib/slack.ml | 26 ++--- lib/state.atd | 11 ++- lib/state.ml | 33 +++++-- mock_states/issue_comment.edited.json | 40 ++++---- mock_states/pull_request_review.edited.json | 40 ++++---- .../pull_request_review_comment.edited.json | 40 ++++---- .../status.state_hide_success_test.json | 40 ++++---- ...hide_success_test_disallowed_pipeline.json | 36 +++---- test/test.ml | 96 ++++++++++--------- 10 files changed, 207 insertions(+), 167 deletions(-) diff --git a/lib/action.ml b/lib/action.ml index 5dfac09b..75c08491 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -192,7 +192,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct partition_issue_comment cfg n |> List.map ~f:(generate_issue_comment_notification ctx n) |> Lwt.return | Commit_comment n -> let%lwt channels, api_commit = partition_commit_comment ctx n in - let notifs = List.map ~f:(generate_commit_comment_notification ctx api_commit n) channels in + let notifs = List.map ~f:(generate_commit_comment_notification api_commit n) channels in Lwt.return notifs | Status n -> let%lwt channels = partition_status ctx n in @@ -203,10 +203,12 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct let send_notifications (ctx : Context.t) (req : Github.t) notifications = let update_comment_mapping message = match req with - | Github.PR_review_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message - | Issue_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message - | Commit_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message - | PR_review n -> State.add_review_map ctx.state n.repository.url n.review.id message + | Github.PR_review_comment n -> + State.add_comment_msg ctx.state n.repository.url ~issue_num:n.pull_request.number n.comment.id message + | Issue_comment n -> + State.add_comment_msg ctx.state n.repository.url ~issue_num:n.issue.number n.comment.id message + | PR_review n -> + State.add_review_msg ctx.state n.repository.url ~issue_num:n.pull_request.number n.review.id message | _ -> Lwt.return_unit in let notify = function diff --git a/lib/slack.ml b/lib/slack.ml index 644483d7..6bb310eb 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -112,10 +112,10 @@ let generate_pr_review_notification (ctx : Context.t) notification channel = match action with | Submitted -> New_message { channel; text = Some summary; attachments; blocks = None } | Edited -> - ( match State.get_review_map ctx.state repository.url review.id with + ( match State.get_review_msg ctx.state repository.url ~issue_num:number review.id with | Some ({ channel; ts } : post_message_res) -> Update_message { channel; ts; text = Some summary; attachments; blocks = None } - | None -> invalid_arg (sprintf "could not find comment %d in %s" review.id repository.url) + | None -> invalid_arg (sprintf "could not find review %d in %s for PR #%n" review.id repository.url number) ) | _ -> invalid_arg "impossible" @@ -155,10 +155,10 @@ let generate_pr_review_comment_notification (ctx : Context.t) notification chann match action with | Created -> New_message { channel; text = Some summary; attachments; blocks = None } | Edited -> - ( match State.get_comment_map ctx.state repository.url comment.id with + ( match State.get_comment_msg ctx.state repository.url ~issue_num:number comment.id with | Some ({ channel; ts } : post_message_res) -> Update_message { channel; ts; text = Some summary; attachments; blocks = None } - | None -> invalid_arg (sprintf "could not find comment %d in %s" comment.id repository.url) + | None -> invalid_arg (sprintf "could not find comment %d in %s for PR #%n" comment.id repository.url number) ) | _ -> invalid_arg "impossible" @@ -229,10 +229,10 @@ let generate_issue_comment_notification (ctx : Context.t) notification channel = match action with | Created -> New_message { channel; text = Some summary; attachments; blocks = None } | Edited -> - ( match State.get_comment_map ctx.state repository.url comment.id with + ( match State.get_comment_msg ctx.state repository.url ~issue_num:number comment.id with | Some ({ channel; ts } : post_message_res) -> Update_message { channel; ts; text = Some summary; attachments; blocks = None } - | None -> invalid_arg (sprintf "could not find comment %d in %s" comment.id repository.url) + | None -> invalid_arg (sprintf "could not find comment %d in %s for issue %n" comment.id repository.url number) ) | _ -> invalid_arg "impossible" @@ -371,9 +371,9 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_ in New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } -let generate_commit_comment_notification (ctx : Context.t) api_commit notification channel = +let generate_commit_comment_notification api_commit notification channel = let { commit; _ } = api_commit in - let { sender; comment; repository; action; _ } = notification in + let { sender; comment; repository; _ } = notification in let commit_id = match comment.commit_id with | None -> invalid_arg "commit id not found" @@ -398,15 +398,7 @@ let generate_commit_comment_notification (ctx : Context.t) api_commit notificati text = Some (mrkdwn_of_markdown comment.body); } in - match action with - | Created -> New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } - | Edited -> - ( match State.get_comment_map ctx.state repository.url comment.id with - | Some ({ channel; ts } : post_message_res) -> - Update_message { channel; ts; text = Some summary; attachments = Some [ attachment ]; blocks = None } - | None -> invalid_arg (sprintf "could not find comment %d in %s" comment.id repository.url) - ) - | _ -> invalid_arg "impossible" + New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } let validate_signature ?(version = "v0") ?signing_key ~headers body = match signing_key with diff --git a/lib/state.atd b/lib/state.atd index 3edd31f9..0092df4c 100644 --- a/lib/state.atd +++ b/lib/state.atd @@ -12,12 +12,17 @@ type pipeline_statuses = branch_statuses map_as_object type post_message_res = abstract type post_msg_map = post_message_res map_as_object +type issue_state = { + comments : post_msg_map; + reviews : post_msg_map; +} -(* The runtime state of a given GitHub repository *) +(* The runtime state of a given GitHub repository + NB: issue number is the same as PR number for github APIs +*) type repo_state = { pipeline_statuses : pipeline_statuses; - comments_map : post_msg_map; - reviews_map : post_msg_map; + issue_tbl : issue_state table_as_object; } (* The serializable runtime state of the bot *) diff --git a/lib/state.ml b/lib/state.ml index 967df958..20b2c11b 100644 --- a/lib/state.ml +++ b/lib/state.ml @@ -7,8 +7,8 @@ type t = { lock : Lwt_mutex.t; (** protect access to mutable string map `pipeline_statuses` *) } -let empty_repo_state () : State_t.repo_state = - { pipeline_statuses = StringMap.empty; comments_map = StringMap.empty; reviews_map = StringMap.empty } +let empty_repo_state () : State_t.repo_state = { pipeline_statuses = StringMap.empty; issue_tbl = Stringtbl.empty () } +let empty_issue_state () : State_t.issue_state = { comments = StringMap.empty; reviews = StringMap.empty } let empty () : t = let state = State_t.{ repos = Stringtbl.empty (); bot_user_id = None } in @@ -16,6 +16,9 @@ let empty () : t = let find_or_add_repo' state repo_url = Stringtbl.find_or_add state.State_t.repos repo_url ~default:empty_repo_state +let find_or_add_issue' (repo_state : State_t.repo_state) issue_num = + Stringtbl.find_or_add repo_state.issue_tbl (Int.to_string issue_num) ~default:empty_issue_state + let set_repo_state { state; lock } repo_url repo_state = Lwt_mutex.with_lock lock @@ fun () -> Stringtbl.set state.repos ~key:repo_url ~data:repo_state; @@ -35,25 +38,35 @@ let set_repo_pipeline_status { state; lock } repo_url ~pipeline ~(branches : Git repo_state.pipeline_statuses <- Map.update repo_state.pipeline_statuses pipeline ~f:set_branch_status; Lwt.return_unit -let add_comment_map { state; lock } repo_url comment_id post_message_res = +let add_comment_msg { state; lock } repo_url ~issue_num comment_id post_message_res = Lwt_mutex.with_lock lock @@ fun () -> let repo_state = find_or_add_repo' state repo_url in - repo_state.comments_map <- Map.set repo_state.comments_map ~key:(Int.to_string comment_id) ~data:post_message_res; + let issue_tbl = find_or_add_issue' repo_state issue_num in + issue_tbl.comments <- Map.set issue_tbl.comments ~key:(Int.to_string comment_id) ~data:post_message_res; Lwt.return_unit -let get_comment_map { state; _ } repo_url comment_id = +let get_comment_msg { state; _ } repo_url ~issue_num comment_id = let repo_state = find_or_add_repo' state repo_url in - Map.find repo_state.comments_map (Int.to_string comment_id) + let issue_tbl = find_or_add_issue' repo_state issue_num in + Map.find issue_tbl.comments (Int.to_string comment_id) -let add_review_map { state; lock } repo_url review_id post_message_res = +let add_review_msg { state; lock } repo_url ~issue_num review_id post_message_res = Lwt_mutex.with_lock lock @@ fun () -> let repo_state = find_or_add_repo' state repo_url in - repo_state.reviews_map <- Map.set repo_state.reviews_map ~key:(Int.to_string review_id) ~data:post_message_res; + let issue_tbl = find_or_add_issue' repo_state issue_num in + issue_tbl.reviews <- Map.set issue_tbl.reviews ~key:(Int.to_string review_id) ~data:post_message_res; Lwt.return_unit -let get_review_map { state; _ } repo_url review_id = +let get_review_msg { state; _ } repo_url ~issue_num review_id = let repo_state = find_or_add_repo' state repo_url in - Map.find repo_state.reviews_map (Int.to_string review_id) + let issue_tbl = find_or_add_issue' repo_state issue_num in + Map.find issue_tbl.reviews (Int.to_string review_id) + +let close_issue { state; lock } repo_url issue_num = + Lwt_mutex.with_lock lock @@ fun () -> + let repo_state = find_or_add_repo' state repo_url in + Stringtbl.remove repo_state.issue_tbl issue_num; + Lwt.return_unit let set_bot_user_id { state; _ } user_id = state.State_t.bot_user_id <- Some user_id let get_bot_user_id { state; _ } = state.State_t.bot_user_id diff --git a/mock_states/issue_comment.edited.json b/mock_states/issue_comment.edited.json index 571cc185..43fc8d81 100644 --- a/mock_states/issue_comment.edited.json +++ b/mock_states/issue_comment.edited.json @@ -1,23 +1,27 @@ { - "pipeline_statuses": { - "default": { - "master": "failure" + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } }, - "buildkite/pipeline2": { - "master": "success" + "issue_tbl" : { + "4" : { + "comments": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + }, + "reviews": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } + } } - }, - "comments_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - }, - "reviews_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - } } diff --git a/mock_states/pull_request_review.edited.json b/mock_states/pull_request_review.edited.json index 571cc185..a1075134 100644 --- a/mock_states/pull_request_review.edited.json +++ b/mock_states/pull_request_review.edited.json @@ -1,23 +1,27 @@ { - "pipeline_statuses": { - "default": { - "master": "failure" + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } }, - "buildkite/pipeline2": { - "master": "success" + "issue_tbl" : { + "4": { + "comments": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + }, + "reviews": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } + } } - }, - "comments_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - }, - "reviews_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - } } diff --git a/mock_states/pull_request_review_comment.edited.json b/mock_states/pull_request_review_comment.edited.json index 571cc185..a1075134 100644 --- a/mock_states/pull_request_review_comment.edited.json +++ b/mock_states/pull_request_review_comment.edited.json @@ -1,23 +1,27 @@ { - "pipeline_statuses": { - "default": { - "master": "failure" + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } }, - "buildkite/pipeline2": { - "master": "success" + "issue_tbl" : { + "4": { + "comments": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + }, + "reviews": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } + } } - }, - "comments_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - }, - "reviews_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - } } diff --git a/mock_states/status.state_hide_success_test.json b/mock_states/status.state_hide_success_test.json index 571cc185..a1075134 100644 --- a/mock_states/status.state_hide_success_test.json +++ b/mock_states/status.state_hide_success_test.json @@ -1,23 +1,27 @@ { - "pipeline_statuses": { - "default": { - "master": "failure" + "pipeline_statuses": { + "default": { + "master": "failure" + }, + "buildkite/pipeline2": { + "master": "success" + } }, - "buildkite/pipeline2": { - "master": "success" + "issue_tbl" : { + "4": { + "comments": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + }, + "reviews": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } + } } - }, - "comments_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - }, - "reviews_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - } } diff --git a/mock_states/status.state_hide_success_test_disallowed_pipeline.json b/mock_states/status.state_hide_success_test_disallowed_pipeline.json index 2516b176..7a18ef9c 100644 --- a/mock_states/status.state_hide_success_test_disallowed_pipeline.json +++ b/mock_states/status.state_hide_success_test_disallowed_pipeline.json @@ -1,19 +1,23 @@ { - "pipeline_statuses": { - "buildkite/pipeline2": { - "master": "failure" + "pipeline_statuses": { + "buildkite/pipeline2": { + "master": "failure" + } + }, + "issue_tbl" : { + "4": { + "comments": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + }, + "reviews": { + "0": { + "channel": "some channel", + "ts": "some ts" + } + } + } } - }, - "comments_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - }, - "reviews_map": { - "0": { - "channel": "some channel", - "ts": "some ts" - } - } } diff --git a/test/test.ml b/test/test.ml index e7f0df93..d169ad8f 100644 --- a/test/test.ml +++ b/test/test.ml @@ -27,52 +27,60 @@ let get_mock_slack_events () = let process_gh_payload ~(secrets : Config_t.secrets) ~config (kind, path, state_path) = let headers = [ "x-github-event", kind ] in - let make_test_context event = - let repo = Github.repo_of_notification @@ Github.parse_exn headers event in - (* overwrite repo url in secrets with that of notification for this test case *) - let secrets = { secrets with repos = [ { url = repo.url; gh_token = None; gh_hook_token = None } ] } in - let ctx = Context.make () in - ctx.secrets <- Some secrets; - let%lwt _ = State.find_or_add_repo ctx.state repo.url in - match state_path with - | None -> - Context.set_repo_config ctx repo.url config; - Lwt.return ctx - | Some state_path -> - match Common.get_local_file state_path with - | Error e -> - log#error "failed to read %s: %s" state_path e; - Lwt.return ctx - | Ok file -> - let repo_state = State_j.repo_state_of_string file in - let%lwt () = State.set_repo_state ctx.state repo.url repo_state in - Context.set_repo_config ctx repo.url config; - Lwt.return ctx - in - Stdio.printf "===== file %s =====\n" path; - let headers = [ "x-github-event", kind ] in - match Common.get_local_file path with - | Error e -> Lwt.return @@ log#error "failed to read %s: %s" path e - | Ok event -> - let%lwt ctx = make_test_context event in - let%lwt _ctx = Action_local.process_github_notification ctx headers event in - Lwt.return_unit + try%lwt + let make_test_context event = + let repo = Github.repo_of_notification @@ Github.parse_exn headers event in + (* overwrite repo url in secrets with that of notification for this test case *) + let secrets = { secrets with repos = [ { url = repo.url; gh_token = None; gh_hook_token = None } ] } in + let ctx = Context.make () in + ctx.secrets <- Some secrets; + let%lwt _ = State.find_or_add_repo ctx.state repo.url in + match state_path with + | None -> + Context.set_repo_config ctx repo.url config; + Lwt.return ctx + | Some state_path -> + match Common.get_local_file state_path with + | Error e -> + log#error "failed to read %s: %s" state_path e; + Lwt.return ctx + | Ok file -> + let repo_state = State_j.repo_state_of_string file in + let%lwt () = State.set_repo_state ctx.state repo.url repo_state in + Context.set_repo_config ctx repo.url config; + Lwt.return ctx + in + Stdio.printf "===== file %s =====\n" path; + let headers = [ "x-github-event", kind ] in + match Common.get_local_file path with + | Error e -> Lwt.return @@ log#error "failed to read %s: %s" path e + | Ok event -> + let%lwt ctx = make_test_context event in + let%lwt _ctx = Action_local.process_github_notification ctx headers event in + Lwt.return_unit + with exn -> + log#error ~exn "failed to run github payload test from %s" path; + Caml.exit 1 let process_slack_event ~(secrets : Config_t.secrets) path = - let ctx = Context.make () in - ctx.secrets <- Some secrets; - State.set_bot_user_id ctx.state "bot_user"; - Stdio.printf "===== file %s =====\n" path; - match Common.get_local_file path with - | Error e -> Lwt.return @@ log#error "failed to read %s: %s" path e - | Ok body -> - match Slack_j.event_notification_of_string body with - | Url_verification _ -> Lwt.return () - | Event_callback notification -> - match notification.event with - | Link_shared event -> - let%lwt _ctx = Action_local.process_link_shared_event ctx event in - Lwt.return_unit + try%lwt + let ctx = Context.make () in + ctx.secrets <- Some secrets; + State.set_bot_user_id ctx.state "bot_user"; + Stdio.printf "===== file %s =====\n" path; + match Common.get_local_file path with + | Error e -> Lwt.return @@ log#error "failed to read %s: %s" path e + | Ok body -> + match Slack_j.event_notification_of_string body with + | Url_verification _ -> Lwt.return () + | Event_callback notification -> + match notification.event with + | Link_shared event -> + let%lwt _ctx = Action_local.process_link_shared_event ctx event in + Lwt.return_unit + with exn -> + log#error ~exn "failed to run slack unfurl test from %s" path; + Caml.exit 1 let () = let payloads = get_mock_payloads () in From 9d2d6bc89202753e2e2bbb984936b9989d6973d4 Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 20 Mar 2023 09:54:15 +0000 Subject: [PATCH 5/9] action, state: clean up when receive PR closed event --- lib/action.ml | 7 +++++++ lib/state.ml | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/action.ml b/lib/action.ml index 75c08491..1f87a8cc 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -271,6 +271,12 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct end | _ -> Lwt.return_unit + let cleanup_state (ctx : Context.t) (payload : Github.t) = + match payload with + | Github.Pull_request { action = Closed; pull_request = { number; _ }; repository = { url; _ }; _ } -> + State.close_issue ctx.state url number + | _ -> Lwt.return_unit + let process_github_notification (ctx : Context.t) headers body = let validate_signature secrets payload = let repo = Github.repo_of_notification payload in @@ -300,6 +306,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct ( match ctx.state_filepath with | None -> Lwt.return_unit | Some path -> + let%lwt () = cleanup_state ctx payload in ( match%lwt State.save ctx.state path with | Ok () -> Lwt.return_unit | Error e -> action_error e diff --git a/lib/state.ml b/lib/state.ml index 20b2c11b..c3e9db7d 100644 --- a/lib/state.ml +++ b/lib/state.ml @@ -65,7 +65,7 @@ let get_review_msg { state; _ } repo_url ~issue_num review_id = let close_issue { state; lock } repo_url issue_num = Lwt_mutex.with_lock lock @@ fun () -> let repo_state = find_or_add_repo' state repo_url in - Stringtbl.remove repo_state.issue_tbl issue_num; + Stringtbl.remove repo_state.issue_tbl (Int.to_string issue_num); Lwt.return_unit let set_bot_user_id { state; _ } user_id = state.State_t.bot_user_id <- Some user_id From 0bfc75dbfdba4940c4bac1343b5b3597cfdbc747 Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 20 Mar 2023 10:10:37 +0000 Subject: [PATCH 6/9] action: also clean issue closed --- lib/action.ml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/action.ml b/lib/action.ml index 1f87a8cc..71516281 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -273,7 +273,8 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct let cleanup_state (ctx : Context.t) (payload : Github.t) = match payload with - | Github.Pull_request { action = Closed; pull_request = { number; _ }; repository = { url; _ }; _ } -> + | Github.Pull_request { action = Closed; pull_request = { number; _ }; repository = { url; _ }; _ } + | Issue { action = Closed; issue = { number; _ }; repository = { url; _ }; _ } -> State.close_issue ctx.state url number | _ -> Lwt.return_unit From e2cfd9cecaa40bf5cd775c47adf4c0c4f1365554 Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Sat, 22 Apr 2023 09:44:17 +0000 Subject: [PATCH 7/9] action, slack: separate content generation + stateful update --- lib/action.ml | 99 +++++++- lib/slack.atd | 5 +- lib/slack.ml | 220 +++++++----------- lib/state.atd | 7 +- mock_states/issue_comment.edited.json | 10 +- mock_states/pull_request_review.edited.json | 10 +- .../pull_request_review_comment.edited.json | 10 +- .../status.state_hide_success_test.json | 10 +- ...hide_success_test_disallowed_pipeline.json | 10 +- test/slack_payloads.expected | 48 ++-- 10 files changed, 214 insertions(+), 215 deletions(-) diff --git a/lib/action.ml b/lib/action.ml index 71516281..c9cecb14 100644 --- a/lib/action.ml +++ b/lib/action.ml @@ -174,6 +174,46 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | Some sender_login -> List.exists cfg.ignored_users ~f:(String.equal sender_login) | None -> false + let generate_stateful_notification (ctx : Context.t) content req channel = + match req with + | Github.PR_review_comment n -> + ( match n.action with + | Github_t.Created -> Some (generate_new_message content channel) + | Edited -> + ( match State.get_comment_msg ctx.state n.repository.url ~issue_num:n.pull_request.number n.comment.id with + | None -> + log#warn "could not find comment %d in %s for PR #%n" n.comment.id n.repository.url n.pull_request.number; + None + | Some ts -> Some (generate_update_message content ~ts channel) + ) + | _ -> None + ) + | Issue_comment n -> + ( match n.action with + | Github_t.Created -> Some (generate_new_message content channel) + | Edited -> + ( match State.get_comment_msg ctx.state n.repository.url ~issue_num:n.issue.number n.comment.id with + | None -> + log#warn "could not find comment %d in %s for issue #%n" n.comment.id n.repository.url n.issue.number; + None + | Some ts -> Some (generate_update_message content ~ts channel) + ) + | _ -> None + ) + | PR_review n -> + ( match n.action with + | Github_t.Submitted -> Some (generate_new_message content channel) + | Edited -> + ( match State.get_review_msg ctx.state n.repository.url ~issue_num:n.pull_request.number n.review.id with + | None -> + log#warn "could not find review %d in %s for PR #%n" n.review.id n.repository.url n.pull_request.number; + None + | Some ts -> Some (generate_update_message content ~ts channel) + ) + | _ -> None + ) + | _ -> Some (generate_new_message content channel) + let generate_notifications (ctx : Context.t) (req : Github.t) = let repo = Github.repo_of_notification req in let cfg = Context.find_repo_config_exn ctx repo.url in @@ -182,21 +222,57 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | false -> match req with | Github.Push n -> - partition_push cfg n |> List.map ~f:(fun (channel, n) -> generate_push_notification n channel) |> Lwt.return - | Pull_request n -> partition_pr cfg n |> List.map ~f:(generate_pull_request_notification n) |> Lwt.return - | PR_review n -> partition_pr_review cfg n |> List.map ~f:(generate_pr_review_notification ctx n) |> Lwt.return + partition_push cfg n + |> List.map ~f:(fun (channel, n) -> + generate_stateful_notification ctx (generate_push_notification_content n) req channel + ) + |> Lwt.return + | Pull_request n -> + partition_pr cfg n + |> List.map ~f:(fun channel -> + generate_stateful_notification ctx (generate_pull_request_notification_content n) req channel + ) + |> Lwt.return + | PR_review n -> + partition_pr_review cfg n + |> List.map ~f:(fun channel -> + generate_stateful_notification ctx (generate_pr_review_notification_content n) req channel + ) + |> Lwt.return | PR_review_comment n -> - partition_pr_review_comment cfg n |> List.map ~f:(generate_pr_review_comment_notification ctx n) |> Lwt.return - | Issue n -> partition_issue cfg n |> List.map ~f:(generate_issue_notification n) |> Lwt.return + partition_pr_review_comment cfg n + |> List.map ~f:(fun channel -> + generate_stateful_notification ctx (generate_pr_review_comment_notification_content n) req channel + ) + |> Lwt.return + | Issue n -> + partition_issue cfg n + |> List.map ~f:(fun channel -> + generate_stateful_notification ctx (generate_issue_notification_content n) req channel + ) + |> Lwt.return | Issue_comment n -> - partition_issue_comment cfg n |> List.map ~f:(generate_issue_comment_notification ctx n) |> Lwt.return + partition_issue_comment cfg n + |> List.map ~f:(fun channel -> + (* not sure why but when I remove "fun channel" it evaluates the [generate_issue_comment_notification_content n] before the partition so I got errors but when I add this, it's completely fine, probably some compiler optimization? *) + generate_stateful_notification ctx (generate_issue_comment_notification_content n) req channel + ) + |> Lwt.return | Commit_comment n -> let%lwt channels, api_commit = partition_commit_comment ctx n in - let notifs = List.map ~f:(generate_commit_comment_notification api_commit n) channels in + let notifs = + List.map + ~f:(generate_stateful_notification ctx (generate_commit_comment_notification_content api_commit n) req) + channels + in Lwt.return notifs | Status n -> let%lwt channels = partition_status ctx n in - let notifs = List.map ~f:(generate_status_notification cfg n) channels in + let notifs = + List.map + ~f:(fun channel -> generate_stateful_notification ctx (generate_status_notification_content cfg n) req channel) + channels + in Lwt.return notifs | _ -> Lwt.return [] @@ -214,7 +290,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct let notify = function | New_message (msg : Slack_t.post_message_req) -> ( match%lwt Slack_api.send_notification ~ctx ~msg with - | Ok res -> update_comment_mapping res + | Ok res -> update_comment_mapping res.ts | Error e -> action_error e ) | Update_message msg -> @@ -303,7 +379,10 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct | Error e -> action_error e | Ok () -> let%lwt notifications = generate_notifications ctx payload in - let%lwt () = Lwt.join [ send_notifications ctx payload notifications; do_github_tasks ctx repo payload ] in + let%lwt () = + Lwt.join + [ send_notifications ctx payload (List.filter_opt notifications); do_github_tasks ctx repo payload ] + in ( match ctx.state_filepath with | None -> Lwt.return_unit | Some path -> diff --git a/lib/slack.atd b/lib/slack.atd index acc741b4..a977eef9 100644 --- a/lib/slack.atd +++ b/lib/slack.atd @@ -69,11 +69,8 @@ type post_message_res = { } type update_message_req = { - channel: string; + inherit post_message_req; ts: string; - ?text: string nullable; - ?attachments: message_attachment list nullable; - ?blocks: message_block list nullable; } type update_message_res = { diff --git a/lib/slack.ml b/lib/slack.ml index 6bb310eb..b67e4c81 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -5,10 +5,6 @@ open Mrkdwn open Github_j open Slack_j -type notify = - | New_message of Slack_t.post_message_req - | Update_message of Slack_t.update_message_req - let empty_attachments = { mrkdwn_in = None; @@ -40,7 +36,7 @@ let show_labels = function let pluralize name num suffix = if num = 1 then sprintf "%s" name else String.concat [ name; suffix ] -let generate_pull_request_notification notification channel = +let generate_pull_request_notification_content notification = let { action; number; sender; pull_request; repository } = notification in let ({ body; title; html_url; labels; merged; _ } : pull_request) = pull_request in let action, body = @@ -59,24 +55,12 @@ let generate_pull_request_notification notification channel = sprintf "<%s|[%s]> Pull request #%d %s %s by *%s*" repository.url repository.full_name number (pp_link ~url:html_url title) action sender.login in - New_message - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt body; - }; - ]; - blocks = None; - } + let attachments = + [ { empty_attachments with mrkdwn_in = Some [ "text" ]; color = Some "#ccc"; text = mrkdwn_of_markdown_opt body } ] + in + attachments, summary -let generate_pr_review_notification (ctx : Context.t) notification channel = +let generate_pr_review_notification_content notification = let { action; sender; pull_request; review; repository } = notification in let ({ number; title; html_url; _ } : pull_request) = pull_request in let action_str = @@ -99,27 +83,18 @@ let generate_pr_review_notification (ctx : Context.t) notification channel = number (pp_link ~url:html_url title) in let attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt review.body; - }; - ] + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = mrkdwn_of_markdown_opt review.body; + }; + ] in - match action with - | Submitted -> New_message { channel; text = Some summary; attachments; blocks = None } - | Edited -> - ( match State.get_review_msg ctx.state repository.url ~issue_num:number review.id with - | Some ({ channel; ts } : post_message_res) -> - Update_message { channel; ts; text = Some summary; attachments; blocks = None } - | None -> invalid_arg (sprintf "could not find review %d in %s for PR #%n" review.id repository.url number) - ) - | _ -> invalid_arg "impossible" + attachments, summary -let generate_pr_review_comment_notification (ctx : Context.t) notification channel = +let generate_pr_review_comment_notification_content notification = let { action; pull_request; sender; comment; repository } = notification in let ({ number; title; html_url; _ } : pull_request) = pull_request in let action_str = @@ -141,28 +116,19 @@ let generate_pr_review_comment_notification (ctx : Context.t) notification chann | Some a -> Some (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url a) in let attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - footer = file; - text = Some (mrkdwn_of_markdown comment.body); - }; - ] + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + footer = file; + text = Some (mrkdwn_of_markdown comment.body); + }; + ] in - match action with - | Created -> New_message { channel; text = Some summary; attachments; blocks = None } - | Edited -> - ( match State.get_comment_msg ctx.state repository.url ~issue_num:number comment.id with - | Some ({ channel; ts } : post_message_res) -> - Update_message { channel; ts; text = Some summary; attachments; blocks = None } - | None -> invalid_arg (sprintf "could not find comment %d in %s for PR #%n" comment.id repository.url number) - ) - | _ -> invalid_arg "impossible" + attachments, summary -let generate_issue_notification notification channel = +let generate_issue_notification_content notification = let ({ action; sender; issue; repository } : issue_notification) = notification in let { number; body; title; html_url; labels; _ } = issue in let action, body = @@ -181,24 +147,12 @@ let generate_issue_notification notification channel = sprintf "<%s|[%s]> Issue #%d %s %s by *%s*" repository.url repository.full_name number (pp_link ~url:html_url title) action sender.login in - New_message - { - channel; - text = Some summary; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = mrkdwn_of_markdown_opt body; - }; - ]; - blocks = None; - } + let attachments = + [ { empty_attachments with mrkdwn_in = Some [ "text" ]; color = Some "#ccc"; text = mrkdwn_of_markdown_opt body } ] + in + attachments, summary -let generate_issue_comment_notification (ctx : Context.t) notification channel = +let generate_issue_comment_notification_content notification = let { action; issue; sender; comment; repository } = notification in let { number; title; _ } = issue in let action_str = @@ -216,25 +170,16 @@ let generate_issue_comment_notification (ctx : Context.t) notification channel = action_str number (pp_link ~url:issue.html_url title) in let attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "text" ]; - color = Some "#ccc"; - text = Some (mrkdwn_of_markdown comment.body); - }; - ] + [ + { + empty_attachments with + mrkdwn_in = Some [ "text" ]; + color = Some "#ccc"; + text = Some (mrkdwn_of_markdown comment.body); + }; + ] in - match action with - | Created -> New_message { channel; text = Some summary; attachments; blocks = None } - | Edited -> - ( match State.get_comment_msg ctx.state repository.url ~issue_num:number comment.id with - | Some ({ channel; ts } : post_message_res) -> - Update_message { channel; ts; text = Some summary; attachments; blocks = None } - | None -> invalid_arg (sprintf "could not find comment %d in %s for issue %n" comment.id repository.url number) - ) - | _ -> invalid_arg "impossible" + attachments, summary let git_short_sha_hash hash = String.sub ~pos:0 ~len:8 hash @@ -266,7 +211,7 @@ let pp_list_with_previews ~pp_item list = end else List.map ~f:pp_item list -let generate_push_notification notification channel = +let generate_push_notification_content notification = let { sender; created; deleted; forced; compare; commits; repository; _ } = notification in let commits_branch = Github.commits_branch_of_ref notification.ref in let tree_url = String.concat ~sep:"/" [ repository.url; "tree"; Uri.pct_encode commits_branch ] in @@ -292,24 +237,19 @@ let generate_push_notification notification channel = sender.login in let commits = pp_list_with_previews ~pp_item:pp_commit commits in - New_message - { - channel; - text = Some title; - attachments = - Some - [ - { - empty_attachments with - mrkdwn_in = Some [ "fields" ]; - color = Some "#ccc"; - fields = Some [ { value = String.concat ~sep:"\n" commits; title = None; short = false } ]; - }; - ]; - blocks = None; - } + let attachments = + [ + { + empty_attachments with + mrkdwn_in = Some [ "fields" ]; + color = Some "#ccc"; + fields = Some [ { value = String.concat ~sep:"\n" commits; title = None; short = false } ]; + }; + ] + in + attachments, title -let generate_status_notification (cfg : Config_t.config) (notification : status_notification) channel = +let generate_status_notification_content (cfg : Config_t.config) (notification : status_notification) = let { commit; state; description; target_url; context; repository; _ } = notification in let ({ commit : inner_commit; sha; author; html_url; _ } : status_commit) = commit in let ({ message; _ } : inner_commit) = commit in @@ -360,18 +300,20 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_ context state_info in let msg = String.concat ~sep:"\n" @@ List.concat [ commit_info; branches_info ] in - let attachment = - { - empty_attachments with - mrkdwn_in = Some [ "fields"; "text" ]; - color = Some color_info; - text = description_info; - fields = Some [ { title = None; value = msg; short = false } ]; - } + let attachments = + [ + { + empty_attachments with + mrkdwn_in = Some [ "fields"; "text" ]; + color = Some color_info; + text = description_info; + fields = Some [ { title = None; value = msg; short = false } ]; + }; + ] in - New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } + attachments, summary -let generate_commit_comment_notification api_commit notification channel = +let generate_commit_comment_notification_content api_commit notification = let { commit; _ } = api_commit in let { sender; comment; repository; _ } = notification in let commit_id = @@ -389,16 +331,28 @@ let generate_commit_comment_notification api_commit notification channel = | None -> None | Some p -> Some (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url p) in - let attachment = - { - empty_attachments with - mrkdwn_in = Some [ "pretext"; "text" ]; - color = Some "#ccc"; - footer = path; - text = Some (mrkdwn_of_markdown comment.body); - } + let attachments = + [ + { + empty_attachments with + mrkdwn_in = Some [ "pretext"; "text" ]; + color = Some "#ccc"; + footer = path; + text = Some (mrkdwn_of_markdown comment.body); + }; + ] in - New_message { channel; text = Some summary; attachments = Some [ attachment ]; blocks = None } + attachments, summary + +type send_notification_mode = + | New_message of Slack_t.post_message_req + | Update_message of Slack_t.update_message_req + +let generate_new_message (attachments, summary) channel = + New_message { channel; text = Some summary; attachments = Some attachments; blocks = None } + +let generate_update_message (attachments, summary) ~ts channel = + Update_message { channel; ts; text = Some summary; attachments = Some attachments; blocks = None } let validate_signature ?(version = "v0") ?signing_key ~headers body = match signing_key with diff --git a/lib/state.atd b/lib/state.atd index 0092df4c..506f1cb3 100644 --- a/lib/state.atd +++ b/lib/state.atd @@ -10,11 +10,10 @@ type branch_statuses = status_state map_as_object branch *) type pipeline_statuses = branch_statuses map_as_object -type post_message_res = abstract -type post_msg_map = post_message_res map_as_object +type ts_map = string map_as_object type issue_state = { - comments : post_msg_map; - reviews : post_msg_map; + comments : ts_map; + reviews : ts_map; } (* The runtime state of a given GitHub repository diff --git a/mock_states/issue_comment.edited.json b/mock_states/issue_comment.edited.json index 43fc8d81..b127ac8b 100644 --- a/mock_states/issue_comment.edited.json +++ b/mock_states/issue_comment.edited.json @@ -11,16 +11,10 @@ "issue_tbl" : { "4" : { "comments": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "1.00" }, "reviews": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "2.00" } } } diff --git a/mock_states/pull_request_review.edited.json b/mock_states/pull_request_review.edited.json index a1075134..7834820b 100644 --- a/mock_states/pull_request_review.edited.json +++ b/mock_states/pull_request_review.edited.json @@ -11,16 +11,10 @@ "issue_tbl" : { "4": { "comments": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "1.00" }, "reviews": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "2.00" } } } diff --git a/mock_states/pull_request_review_comment.edited.json b/mock_states/pull_request_review_comment.edited.json index a1075134..7834820b 100644 --- a/mock_states/pull_request_review_comment.edited.json +++ b/mock_states/pull_request_review_comment.edited.json @@ -11,16 +11,10 @@ "issue_tbl" : { "4": { "comments": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "1.00" }, "reviews": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "2.00" } } } diff --git a/mock_states/status.state_hide_success_test.json b/mock_states/status.state_hide_success_test.json index a1075134..7834820b 100644 --- a/mock_states/status.state_hide_success_test.json +++ b/mock_states/status.state_hide_success_test.json @@ -11,16 +11,10 @@ "issue_tbl" : { "4": { "comments": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "1.00" }, "reviews": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "2.00" } } } diff --git a/mock_states/status.state_hide_success_test_disallowed_pipeline.json b/mock_states/status.state_hide_success_test_disallowed_pipeline.json index 7a18ef9c..f85336ab 100644 --- a/mock_states/status.state_hide_success_test_disallowed_pipeline.json +++ b/mock_states/status.state_hide_success_test_disallowed_pipeline.json @@ -7,16 +7,10 @@ "issue_tbl" : { "4": { "comments": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "1.00" }, "reviews": { - "0": { - "channel": "some channel", - "ts": "some ts" - } + "0": "2.00" } } } diff --git a/test/slack_payloads.expected b/test/slack_payloads.expected index 9a494e55..d8ec7477 100644 --- a/test/slack_payloads.expected +++ b/test/slack_payloads.expected @@ -171,10 +171,9 @@ will notify #frontend-bot ] } ===== file ../mock_payloads/issue_comment.edited.json ===== -will update msg in #some channel +will update msg in #a1-bot { - "channel": "some channel", - "ts": "some ts", + "channel": "a1-bot", "text": " *xinyuluo* on #4 ", "attachments": [ { @@ -183,12 +182,12 @@ will update msg in #some channel "color": "#ccc", "text": "issue comment edited test" } - ] + ], + "ts": "1.00" } -will update msg in #some channel +will update msg in #backend { - "channel": "some channel", - "ts": "some ts", + "channel": "backend", "text": " *xinyuluo* on #4 ", "attachments": [ { @@ -197,7 +196,8 @@ will update msg in #some channel "color": "#ccc", "text": "issue comment edited test" } - ] + ], + "ts": "1.00" } ===== file ../mock_payloads/issues.closed.json ===== will notify #backend @@ -335,10 +335,9 @@ will notify #frontend-bot } ===== file ../mock_payloads/pull_request_review.dismissed.json ===== ===== file ../mock_payloads/pull_request_review.edited.json ===== -will update msg in #some channel +will update msg in #a1-bot { - "channel": "some channel", - "ts": "some ts", + "channel": "a1-bot", "text": " *xinyuluo* #4 ", "attachments": [ { @@ -347,12 +346,12 @@ will update msg in #some channel "color": "#ccc", "text": "pr review comment test" } - ] + ], + "ts": "2.00" } -will update msg in #some channel +will update msg in #backend { - "channel": "some channel", - "ts": "some ts", + "channel": "backend", "text": " *xinyuluo* #4 ", "attachments": [ { @@ -361,7 +360,8 @@ will update msg in #some channel "color": "#ccc", "text": "pr review comment test" } - ] + ], + "ts": "2.00" } ===== file ../mock_payloads/pull_request_review.request_changes.json ===== will notify #default @@ -410,10 +410,9 @@ will notify #backend } ===== file ../mock_payloads/pull_request_review_comment.deleted.json ===== ===== file ../mock_payloads/pull_request_review_comment.edited.json ===== -will update msg in #some channel +will update msg in #a1-bot { - "channel": "some channel", - "ts": "some ts", + "channel": "a1-bot", "text": " *xinyuluo* commented on #4 ", "attachments": [ { @@ -423,12 +422,12 @@ will update msg in #some channel "text": "Edited test comment", "footer": "New comment by xinyuluo in " } - ] + ], + "ts": "1.00" } -will update msg in #some channel +will update msg in #backend { - "channel": "some channel", - "ts": "some ts", + "channel": "backend", "text": " *xinyuluo* commented on #4 ", "attachments": [ { @@ -438,7 +437,8 @@ will update msg in #some channel "text": "Edited test comment", "footer": "New comment by xinyuluo in " } - ] + ], + "ts": "1.00" } ===== file ../mock_payloads/push.branch_filter_default.json ===== will notify #all-push-events From 5df2e8bb2f49cbed1ca69a19020476b492561a9d Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Sat, 22 Apr 2023 10:22:30 +0000 Subject: [PATCH 8/9] minor --- lib/github.atd | 2 +- lib/state.atd | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/github.atd b/lib/github.atd index e8a443b2..2a424942 100644 --- a/lib/github.atd +++ b/lib/github.atd @@ -288,7 +288,7 @@ type api_commit = { } type commit_comment_notification = { - action: comment_action; + action: string; repository: repository; comment: comment; sender: github_user; diff --git a/lib/state.atd b/lib/state.atd index 506f1cb3..5f73e78a 100644 --- a/lib/state.atd +++ b/lib/state.atd @@ -16,11 +16,10 @@ type issue_state = { reviews : ts_map; } -(* The runtime state of a given GitHub repository - NB: issue number is the same as PR number for github APIs -*) +(* The runtime state of a given GitHub repository *) type repo_state = { pipeline_statuses : pipeline_statuses; + (* issue number is the same as PR number for github APIs *) issue_tbl : issue_state table_as_object; } From 817c2c327b8ee04da035e942ef8207fdad0cfdba Mon Sep 17 00:00:00 2001 From: Sewen Thy Date: Mon, 24 Apr 2023 03:34:19 +0000 Subject: [PATCH 9/9] slack: refactor api_remote webhook calls into function --- lib/api_remote.ml | 80 +++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 51 deletions(-) diff --git a/lib/api_remote.ml b/lib/api_remote.ml index 1c738055..0aba78e2 100644 --- a/lib/api_remote.ml +++ b/lib/api_remote.ml @@ -117,68 +117,46 @@ module Slack : Api.Slack = struct (* must read whole response to update lexer state *) ignore (Slack_j.read_ok_res s l) + let webhook_channel_request (ctx : Context.t) ~channel ~build_error read body = + match Context.hook_of_channel ctx channel with + | Some url -> + ( match%lwt http_request ~body ~headers:[] `POST url with + | Ok res -> Lwt.return_ok @@ read res + | Error e -> Lwt.return @@ build_error (query_error_msg url e) + ) + | None -> Lwt.return @@ build_error (sprintf "no hook for channel: %s" channel) + (** [send_notification ctx msg] notifies [msg.channel] with the payload [msg]; uses web API with access token if available, or with webhook otherwise *) let send_notification ~(ctx : Context.t) ~(msg : Slack_t.post_message_req) = log#info "sending to %s" msg.channel; let build_error e = fmt_error "%s\nfailed to send Slack notification" e in - let secrets = Context.get_secrets_exn ctx in - let headers, url, webhook_mode = - match Context.hook_of_channel ctx msg.channel with - | Some url -> [], Some url, true - | None -> - match secrets.slack_access_token with - | Some access_token -> [ bearer_token_header access_token ], Some "https://slack.com/api/chat.postMessage", false - | None -> [], None, false - in - match url with - | None -> Lwt.return @@ build_error @@ sprintf "no token or webhook configured to notify channel %s" msg.channel - | Some url -> - let data = Slack_j.string_of_post_message_req msg in - let body = `Raw ("application/json", data) in - log#info "data: %s" data; - if webhook_mode then begin - match%lwt http_request ~body ~headers `POST url with - | Ok res -> Lwt.return_ok @@ Slack_j.post_message_res_of_string res - | Error e -> Lwt.return @@ build_error (query_error_msg url e) - end - else begin - match%lwt slack_api_request ~body ~headers `POST url Slack_j.read_post_message_res with - | Ok res -> Lwt.return_ok res - | Error e -> Lwt.return @@ build_error e - end + let data = Slack_j.string_of_post_message_req msg in + let body = `Raw ("application/json", data) in + log#info "data: %s" data; + match%lwt webhook_channel_request ctx ~channel:msg.channel ~build_error Slack_j.post_message_res_of_string body with + | Ok res -> Lwt.return_ok res + | Error e -> + log#warn "failed to run webhook call: %s" e; + request_token_auth ~name:"post message to channel" ~body ~ctx `POST "chat.postMessage" + Slack_j.read_post_message_res (** [update_notification ctx msg] update a message at [msg.ts] in [msg.channel] with the payload [msg]; uses web API with access token if available, or with webhook otherwise *) let update_notification ~(ctx : Context.t) ~(msg : Slack_t.update_message_req) = log#info "sending to %s" msg.channel; - let build_error e = fmt_error "%s\nfailed to send Slack notification" e in - let secrets = Context.get_secrets_exn ctx in - let headers, url, webhook_mode = - match Context.hook_of_channel ctx msg.channel with - | Some url -> [], Some url, true - | None -> - match secrets.slack_access_token with - | Some access_token -> [ bearer_token_header access_token ], Some "https://slack.com/api/chat.update", false - | None -> [], None, false - in - match url with - | None -> Lwt.return @@ build_error @@ sprintf "no token or webhook configured to notify channel %s" msg.channel - | Some url -> - let data = Slack_j.string_of_update_message_req msg in - let body = `Raw ("application/json", data) in - log#info "data: %s" data; - if webhook_mode then begin - match%lwt http_request ~body ~headers `POST url with - | Ok (_res : string) -> Lwt.return_ok () - | Error e -> Lwt.return @@ build_error (query_error_msg url e) - end - else begin - match%lwt slack_api_request ~body ~headers `POST url Slack_j.read_update_message_res with - | Ok (_res : Slack_t.update_message_res) -> Lwt.return_ok () - | Error e -> Lwt.return @@ build_error e - end + let build_error e = fmt_error "%s\nfailed to update Slack notification" e in + let data = Slack_j.string_of_update_message_req msg in + let body = `Raw ("application/json", data) in + log#info "data: %s" data; + match%lwt + webhook_channel_request ctx ~channel:msg.channel ~build_error Slack_j.update_message_res_of_string body + with + | Ok (_res : Slack_t.update_message_res) -> Lwt.return_ok () + | Error e -> + log#warn "failed to run webhook call: %s" e; + request_token_auth ~name:"update message in channel" ~body ~ctx `POST "chat.update" read_unit let send_chat_unfurl ~(ctx : Context.t) ~channel ~ts ~unfurls () = let req = Slack_j.{ channel; ts; unfurls } in