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

combine instances of "reviewers as assignees" rule #129

Open
SimonLab opened this issue Jun 22, 2017 · 9 comments
Open

combine instances of "reviewers as assignees" rule #129

SimonLab opened this issue Jun 22, 2017 · 9 comments

Comments

@SimonLab
Copy link
Member

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

Cleop commented Jun 23, 2017

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

ghost commented Jun 23, 2017

Links to #92

@SimonLab
Copy link
Member Author

This has also happened on LDMW

@ghost
Copy link

ghost commented Jun 23, 2017

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

SimonLab commented Jun 26, 2017

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

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

ghost commented Aug 9, 2017

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

@ghost ghost added priority-2 and removed priority-3 labels Aug 9, 2017
@ghost
Copy link

ghost commented Aug 9, 2017

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

@ghost ghost added this to the Sprint 2 milestone Aug 9, 2017
@ghost ghost modified the milestones: Sprint 2, Backlog Aug 9, 2017
@ghost ghost modified the milestones: Sprint 2, Backlog Aug 9, 2017
@ghost ghost added bug and removed enhancement labels Aug 9, 2017
@naazy
Copy link
Member

naazy commented Sep 8, 2017

I think we should wait for #134 (comment) to be merged in before tackling this because they are related

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

No branches or pull requests

4 participants