[WIP] Feat/send notification to slack #6

thatportugueseguy · 2019-07-28T21:53:49Z

Branches off and is blocked by #5.

Added the ability to send notifications to slack.

There is still no error handling on the curl request, as i'm not sure what would be considered enough. I've seen different examples for error handling and would like to know if it's enough just to have a catch all catch or if i should handle any specific cases, like curl exceptions. Some pointers here would be welcome :)

I've opted not to use the curl.lwt module as it didn't seem necessary as i'm not doing any processing while the request is running. Please let me know if i'm not looking at this the right way.

(meant to create a WIP PR, but pressed the create button by mistake and now can't change the type of PR

ygrek

I've opted not to use the curl.lwt module as it didn't seem necessary as i'm not doing any processing while the request is running.

this is not true, server is asynchronous and can handle other requests while current one is doing curl query

ygrek · 2019-07-29T01:35:43Z

configuration.ml

+    and c = Curl.init () in
+    Curl.set_timeout c 1200;
+    Curl.set_sslverifypeer c false;
+    Curl.set_sslverifyhost c Curl.SSLVERIFYHOST_EXISTENCE;


meh, use proper ssl verification (and anyway this option maps to strict name check in modern libcurl)

I'm not exactly sure how to do it. Looking at the curl lib examples and having a look around libcurls' docs, it seems that https://github.com/ygrek/ocurl/blob/master/examples/ossl.ml#L14 could be an option:

Curl.set_sslverifypeer c true; Curl.set_sslverifyhost c Curl.SSLVERIFYHOST_HOSTNAME;

Would this be enough? Seems that the next step in verification would imply having code to validate the actual certificate (iiuc).

this is the way, yes.

no next step is needed, libcurl verifies the certificate against the system ca bundle alright

ygrek · 2019-07-29T01:36:15Z

configuration.ml

+    Curl.set_writefunction c (writer_callback r);
+    Curl.set_tcpnodelay c true;
+    Curl.set_verbose c false;
+    Curl.set_post c false;


I would assume slack webhook should actually be post?

Removing this line, as it's set up on the send_notification function.

ygrek · 2019-07-29T01:42:31Z

notabot.ml

+      let fields = 
+        match List.length labels with 
+        | n when n > 0 -> (
+            let labels_string = List.fold ~init:"" ~f:(fun acc label -> Printf.sprintf "%s %s," acc label.name) labels in 


Suggested change

let labels_string = List.fold ~init:"" ~f:(fun acc label -> Printf.sprintf "%s %s," acc label.name) labels in

let value = String.concat ", " @@ List.map (fun x -> x.name) labels

Thanks! (ended up having to make the more changes than the suggestion, so didn't accept the suggested change, but implemented it.)

ygrek · 2019-07-29T01:44:13Z

notabot.ml

+      }
+
+    let git_short_sha_hash hash =
+      String.sub ~pos:0 ~len:8 (Github_events_notifications_j.string_of_commit_hash hash)


8 is not enough for our repos now, there should be short_hash field in github message which select minimum unambiguous length, no?

Unfortunately, not. I did look for it, though. Looking at the compare link for the push notification, they use 12 length. I used 8 by copying from the github extension which shows only 8.

Actually this is something that will only be visible on slack, it's not going to be part of any link or something like that. When we click on it, it will open the link that will show the proper commits.

I will change to 12, in case we ever want to copy and use it.

ah, right, then no change needed

8 it is, then :)

ygrek · 2019-07-29T01:46:53Z

notabot.ml

+      let commit_branch = 
+        (* take out refs/heads and get the full name of the branch *)
+        String.concat ~sep:"/" @@ Array.to_list @@ Array.subo ~pos:2 ref_tokens in
+      let number_of_commits = List.length commits in


identifier longer than the expression it replaces 😬

good point. 👌

ygrek · 2019-07-29T01:50:19Z

notabot.ml

+        }
+      ] in
+      let attachments = [
+        {


could have { empty_attachment with color = ... }

ygrek · 2019-07-29T01:50:53Z

notabot.ml

+      let fields = [
+        { 
+          title = None;
+          value = String.concat ~sep:"\n" @@ title :: status :: description :: commit_info :: author_info :: [];


better use [title;status;...] here

ygrek · 2019-07-29T01:51:26Z

notabot.ml

+      let open Github_notifications_handler in
+      match request_notification with
+      | Push notification -> Ok (generate_push_notification notification)
+      | Pull_request n when is_review_requested n.action -> Ok (generate_pull_request_notification n)


we probably want all PRs, not only those with review requested

lets have all and then limit later when we see it in action

ygrek · 2019-07-29T01:51:53Z

notabot.ml

+      Curl.set_postfieldsize c (String.length data);
+      Curl.perform c;
+      let rc = Curl.get_responsecode c in
+      Curl.cleanup c;


cleanup is not called in case of exn in perform

Added cleanup in %lwt.finally. I'm not sure if that's exactly how it should be as (iiuc) it will always run regardless of the outcome of the try with block.

I'm still wrapping my head around some bit of the lwt docs, but got the impression this isn't what we want, but it's used like so on the ocurl lwt tests, so i'm not sure i understood it correctly.

as (iiuc) it will always run regardless of the outcome of the try with block

and that's exactly what we want, we need to dispose the resource regardless of the outcome of operation

ok, I had misinterpreted your first commentary as it meaning that cleanup should not be called in case of exn, hence the question.

ygrek · 2019-07-29T01:53:13Z

notabot.ml

+      Curl.set_httpheader c [ "Content-Type: " ^ content_type ];
+      Curl.set_postfields c data;
+      Curl.set_postfieldsize c (String.length data);
+      Curl.perform c;


exn handling is needed ofc - just catch all exns from curl.perform and log in case of error

Handled a few error cases. Please comment if adequate.

ygrek · 2019-07-31T05:25:38Z

notabot.ml

+      let title = 
+        Printf.sprintf "*<%s|%i new commit%s> pushed to `<%s|%s>`*" compare (List.length commits) (match commits with [_] -> "s" | _ -> "") repository.url commit_branch in
+      let commits_text_list = List.map commits ~f:(fun { url; id; message; _ } ->
+          Printf.sprintf "`<%s|%s>` - `%s`" url (git_short_sha_hash id) message) in


but probably should do something with single quotes in the message itself (aka escape)

ygrek · 2019-07-31T05:26:40Z

notabot.ml

+          | code -> 
+            Stdio.print_endline @@ Printf.sprintf "Slack notification request errored: %s" (Curl.strerror code);
+            Lwt.return None
+        with exn -> 


try is not needed, easier to match against exception exn then

thatportugueseguy added 3 commits July 27, 2019 23:26

Add slack formatted notifications

8d798ac

Add send POST request notification to slack

7e9561d

Small cleanup

e0ba1de

thatportugueseguy changed the title ~~Feat/send notification to slack~~ [WIP] Feat/send notification to slack Jul 28, 2019

Remove curl.lwt dependency

b5fc2b4

ygrek suggested changes Jul 29, 2019

View reviewed changes

thatportugueseguy added 2 commits July 29, 2019 18:08

Add changes from PR

6af44c1

Clean up a bit after more PR feedback

b66a7a4

ygrek approved these changes Jul 31, 2019

View reviewed changes

ygrek merged commit d06113d into fix/fix-request-signature-validation Jul 31, 2019

ygrek deleted the feat/send-notification-to-slack branch July 31, 2019 05:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feat/send notification to slack #6

[WIP] Feat/send notification to slack #6

thatportugueseguy commented Jul 28, 2019 •

edited

Loading

ygrek left a comment

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

ygrek Jul 30, 2019

thatportugueseguy Jul 30, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 30, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019 •

edited

Loading

ygrek Jul 29, 2019

thatportugueseguy Jul 30, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 30, 2019

thatportugueseguy Jul 30, 2019

ygrek Jul 29, 2019

thatportugueseguy Jul 29, 2019

ygrek Jul 31, 2019

ygrek Jul 31, 2019

	let labels_string = List.fold ~init:"" ~f:(fun acc label -> Printf.sprintf "%s %s," acc label.name) labels in
	let value = String.concat ", " @@ List.map (fun x -> x.name) labels

[WIP] Feat/send notification to slack #6

[WIP] Feat/send notification to slack #6

Conversation

thatportugueseguy commented Jul 28, 2019 • edited Loading

ygrek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatportugueseguy Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatportugueseguy commented Jul 28, 2019 •

edited

Loading

thatportugueseguy Jul 29, 2019 •

edited

Loading