Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create GenServer process to merge errors together #134

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

SimonLab
Copy link
Member

@SimonLab SimonLab commented Jun 28, 2017

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.

@SimonLab SimonLab self-assigned this Jun 28, 2017
@codecov
Copy link

codecov bot commented Jun 28, 2017

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% <ø> (ø) ⬆️
web/controllers/rules/status/travis_failure.ex 100% <ø> (ø) ⬆️
web/controllers/rules/pr/no_description.ex 100% <ø> (ø) ⬆️
web/controllers/rules/issue/no_description.ex 100% <ø> (ø) ⬆️
web/controllers/rules/issue/time_estimation.ex 100% <ø> (ø) ⬆️
web/controllers/rules/pr/awaiting_review.ex 100% <ø> (ø) ⬆️
web/controllers/rules/issue/noassignees.ex 100% <ø> (ø) ⬆️
web/controllers/rules/pr/merge_conflict.ex 100% <ø> (ø) ⬆️
web/controllers/rules/issue/inprogress.ex 100% <ø> (ø) ⬆️
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.

@moduledoc """
join rules together when necesssary:
- in-progress label added: combine no_assignee with no_estimation
- multiple reviewers added: combine add assignees rule
Copy link
Member Author

@SimonLab SimonLab Jul 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll implement this (combine reviewers errors) on another PR, this one is full enough I think

@SimonLab SimonLab changed the title WIP - create GenServer process to merge errors together create GenServer process to merge errors together Jul 1, 2017
@SimonLab SimonLab requested a review from samhstn July 1, 2017 11:56
@SimonLab SimonLab requested a review from nelsonic July 1, 2017 11:56
@SimonLab SimonLab assigned nelsonic and unassigned SimonLab Jul 1, 2017
Copy link
Member

@samhstn samhstn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great @SimonLab

nil
iex>get_error_by_type([%{error_type: "type_1"}], "type_1")
%{error_type: "type_1"}
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^^^ I like this. Very helpful 👍

iex>get_id(%{"repository" => %{"full_name" => "repo"}, "issue" => %{"number" => 42}})
"repo/42"
"""
def get_id(payload) when is_map(payload)do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of guard 👍

@samhstn
Copy link
Member

samhstn commented Jul 1, 2017

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
Copy link
Member Author

SimonLab commented Jul 1, 2017

@Shouston3 your question remind me that I need to update the doc for this 👍
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 👍

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonLab already looks great! assign when you think it's ready. 👍

@SimonLab
Copy link
Member Author

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:

def handle_cast({:report_errors, errors}, errors_state) do
joined_errors = Join.join(errors)
joined_errors
|> Enum.each(fn(err) ->
@github_api.report_error(err.token, err)
end)
{:noreply, errors_state}
end

To resolve this we need to add another check on the merge function to check the origin of the errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants