Error log in notification #176
Conversation
|
I fixed the test suite by having an explicit |
There was a problem hiding this comment.
you need to pull the latest changes in master to your branch, a lot has changed.
We need to get logs only for the jobs we are effectively notifying, not for all failed jobs.
These logs should be in a snippet so they are collapsible. These should be present in the tests too.
a few files files need newlines at the end
lib/github.ml
Outdated
| let get_logs ~ctx ~(api : (module Api.Buildkite)) (n : status_notification) = | ||
| let module Buildkite = (val api) in | ||
| let ( let* ) = Lwt_result.bind in | ||
| match n.state with | ||
| | Success | Pending -> Lwt.return_ok None | ||
| | Failure | Error -> | ||
| let* build = Buildkite.get_build ~ctx n in | ||
| let failed_jobs = | ||
| List.filter | ||
| (fun (job : Buildkite_t.job) -> | ||
| match job.state with | ||
| | Some Failed -> true | ||
| | _ -> false) | ||
| build.jobs | ||
| in | ||
| (match failed_jobs with | ||
| | [] -> Lwt.return_ok None | ||
| | _ :: _ -> | ||
| let* log = Buildkite.get_job_log ~ctx (List.hd failed_jobs) in | ||
| Lwt.return_ok (Some log.content)) |
There was a problem hiding this comment.
we don't wan to get the logs for every failed job. We only want to get the logs for the failed jobs we will notify to slack. This should be done in Action.generate_notifications before calling Slack.generate_status_notifications and then you can pass in the logs to that function.
I think making the failed_steps arg a tuple of failed_step * log would be the most correct way
lib/slack.ml
Outdated
| let followups = | ||
| match job_log with | ||
| | Some log -> | ||
| let text = sprintf "Log : ```\n%s\n```" (Text_cleanup.cleanup log) in |
There was a problem hiding this comment.
| let text = sprintf "Log : ```\n%s\n```" (Text_cleanup.cleanup log) in | |
| let text = Text_cleanup.cleanup in |
we already know these are logs. It should be on the snippet title.
lib/slack.ml
Outdated
| let make_followup ?text ?attachments ?blocks ?(reply_broadcast = false) () = | ||
| { text; attachments; blocks; reply_broadcast } |
There was a problem hiding this comment.
logs should go in snippets, otherwise it will make for giant messages, no? (and in any case we wouldn't want to spam the channel with logs)
| let make_followup ?text ?attachments ?blocks ?(reply_broadcast = false) () = | |
| { text; attachments; blocks; reply_broadcast } | |
| let make_followup ?text ?attachments ?blocks () = | |
| { text; attachments; blocks; reply_broadcast = false } |
There was a problem hiding this comment.
I did not know about snippet, I checked them out, I agree they should be used.
test/slack_payloads.expected
Outdated
There was a problem hiding this comment.
these changes seem unintended?
EmileTrotignon
force-pushed
the
error-log-in-notification
branch
from
cd28762 to
7c69e6b
Compare
thatportugueseguy
force-pushed
the
master
branch
from
2f4a80c to
468d914
Compare
EmileTrotignon
force-pushed
the
error-log-in-notification
branch
from
20b02eb to
619808d
Compare
EmileTrotignon
force-pushed
the
error-log-in-notification
branch
from
619808d to
c99d4ba
Compare
This is add error logs for failed builds.
For know the test are broken, and I don't really understand how to work with them, help is appreciated.