search

dwyl / dwylbot

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

combine instances of "reviewers as assignees" rule #129

Open SimonLab opened 7 years ago

SimonLab commented 7 years ago

The rule define in #62 (adding reviewers as assignee) is trigger multiple time if reviewers are defined at the same time. We still want to add each reviewers as assignees however we want to only add one dwylbot comment and only add the awaiting-reiview label once.

Cleop commented 7 years ago

This is what I just experienced:

It's also worth mentioning that I had actually assigned these people at the same time as making them reviewers (I did it when I pressed create PR) and the awaiting-review label but dwylbot did it again, perhaps this needs a small delay on it?

ghost commented 7 years ago

Links to https://github.com/dwyl/dwylbot/issues/92

SimonLab commented 7 years ago

This has also happened on LDMW

ghost commented 7 years ago

@SimonLab Can we please update the comment from:

@username, a reviewer has been added to the pull request. The pull request looks ready for review (no "in-progress" label). So the reviewer has been added as an assignee and the "awaiting-reivew" label as been added to.

to:

@username, the in-progress label has been removed from this pull request and a Reviewer has been added. It appears that this pull request is ready for review, so the Reviewer has been added as an Assignee and the awaiting-review label has been applied automatically.

SimonLab commented 7 years ago

A first idea on how to combine errors:

Instead of reporting the errors directly on Github we need to collect them during a specific short time and combine the errors. For that we can create a new process and send the errors to this process. The process will then catch any errors linked to the same org/repo/issue and combine them. Then the new combine errors will be sent to our Github wrapper. Elixir let us name process, this will allow us to know where to send the errors to. This is the basic logic and needs a bit more details but I think it should resolve our multiple error messages issue.

SimonLab commented 7 years ago

I manage to start a process which will collect the errors for a give org/repo/issue then combine the errors. I also manage to give a name to the process with Process.register function. The idea is for each github event check if a process already exists for the issue where the event occurs by looking the process by name with Process.whereis function. However this is not working, the github events are send almost at the same time, the program can't find the name and try to start and register multiple processes with the same name. Maybe GenServer provide a solution for this kind of error concurrency but I think we can simplify the logic first.

"What if two processes try to register the same name, for example?...The general rule is to register your process names when your application starts."

So instead of having multiple processes (one per each org/repo/issue) we can start only one process that we will name when the application start. This process will check every after 1mn for example if some rules can be combined. At the moment this can work but if dwylbot is installed on a important number of repo and if it receives a lot of error this solution might not scale very well. At this stage it's ok to have only one process.

ghost commented 7 years ago

@naazy I'm bumping up the priority on this one as it seems to be affecting a number of people

ghost commented 7 years ago

@SimonLab I noticed the in-progress label is still on - how far away from completion is this solution?

naazy commented 6 years ago

I think we should wait for https://github.com/dwyl/dwylbot/pull/134#issue-239239313 to be merged in before tackling this because they are related