dwyl / dwylbot

:robot: Automating our GitHub Workflow to improve team communication/collaboration and reduce tedious repetition!
28 stars 7 forks source link

create GenServer process to merge errors together #134

Open SimonLab opened 7 years ago

SimonLab commented 7 years ago

ref: #92 (how to combine multiple dwylbot comments) image

this PR also provide the structure for #129 (combine dwylbot comment for reviewers rule)

This PR use GenServer to create a small server which will receive all the errors that need to be send to Github (all the errors of every repo where dwylbot is installed). Then every 5s the server will combine the errors together if needed and will comment/assign/label on Github depending of the dwylbot actions.

The only small issue with A GenServer which start a process every 5s is that we might need to have an Heroku dyno live 24h (which means that we might need to pay). But maybe Heroku will automatically stop the GenServer and restart it when needed, to test.

codecov[bot] commented 7 years ago

Codecov Report

Merging #134 into master will increase coverage by 0.31%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   98.32%   98.64%   +0.31%     
==========================================
  Files          23       26       +3     
  Lines         179      221      +42     
==========================================
+ Hits          176      218      +42     
  Misses          3        3
Impacted Files Coverage Δ
web/controllers/rules/pr/no_assignee.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/status/travis_failure.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/pr/no_description.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/issue/no_description.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/issue/time_estimation.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/pr/awaiting_review.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/issue/noassignees.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/pr/merge_conflict.ex 100% <ø> (ø) :arrow_up:
web/controllers/rules/issue/inprogress.ex 100% <ø> (ø) :arrow_up:
web/controllers/processes/merge_errors.ex 100% <100%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 79e802e...d26e9f7. Read the comment docs.

samhstn commented 7 years ago

I should have asked this earlier @SimonLab, better late than never.

How should we be going about testing the code to make sure it works (besides the tests)? What is the way you would recommend that we go about manually testing these prs?

SimonLab commented 7 years ago

@shouston3 your question remind me that I need to update the doc for this :+1: I've created my own Github App where I can change the configuration of the webhook url to send any data from github to my localhost server. For that my server needs to be public, so I'm using ngrok to create a connection specific url for my server. This url is then added to the config on my app: image

I'll update the doc soon like this you should be able to do some manual testing if you want. Thanks for the review :+1:

SimonLab commented 7 years ago

This PR needs a bit more work. At the moment the all the errors are merged together every 5s but the code doesn't check the origin of the error. So it can happen that errors from different issues and repos are merged together: https://github.com/dwyl/dwylbot/blob/d26e9f78aad3b67afdd72d87f55d20ee04dc4d7a/web/controllers/processes/merge_errors.ex#L33-L41 To resolve this we need to add another check on the merge function to check the origin of the errors.