search

z0al / probot-review-me

Decide when a pull request is ready for review based on its statuses
MIT License
16 stars 0 forks source link

Sets the review label on other event than a push event #4

Open johlju opened 6 years ago

johlju commented 6 years ago

It seems it gets a status event when there is a label change event. Is it possible to get it to only check statuses on a pull request push event? See https://github.com/PowerShell/DscResource.Tests/pull/271. I want to label the PR 'waiting for code fix' since it has merge conflicts. But the bot add back 'needs review' without a push event.

johlju commented 6 years ago

Okay, so it seems that the reason it set the 'needs review' was because I merged a PR and that kicked of a new status event, and since it can't see the merge conflict as a context (issue #3) it relabeled the PR. This is by design.

z0al commented 6 years ago

Hey @johlju , I really appreciate that to you take the time to report those issues :)

Okay, so it seems that the reason it set the 'needs review' was because I merged a PR

I was about to write the same comment.

johlju commented 6 years ago

@z0al No problem - only way to improve something is to report an issue 🙂

It seems there is also a status event update on all PR's when a push is made to any PR. 🤔 Still, this is by design.

johlju commented 6 years ago

@z0al Reopening this issue for a discussion how this can be solved, if possible. This is in regards to that it updates statuses on all PR's on merge or push to any one PR.

Our review workflow.

  1. PR should first pass all checks, Review Me labels the PR as 'needs review' once all statuses are success.
  2. Maintainer reviews and labels the PR either as 'waiting for code fix' or 'waiting for author response', and removes 'needs review' label.
  3. (I was hoping) On a new push to the pull request, Review Me sets the label to 'needs review', and removes 'waiting...'-label (issue #2).

Now, if a push is made to another PR before step 3 happens, then Review Me will label the PR that is 'waiting for a code fix' back as 'needs review'. I would like, if possible, that not to happen. This is because all issues in ~60 repos are tracked on a Waffle board (https://waffle.io/powershell/dscresources), and by labeling an issue 'needs review' means that the issue moves to "Needs review" column, and maintainer need to take action, while it is in the "Waiting for code fix" column then no action is needed.

Not sure if it is possible to solve. Maybe we need to change the review workflow. 🤔

johlju commented 6 years ago

I think issue #4 is resolved only once ReviewMe detects that a change was made on the specific PR.

For example, if there are two PR's; PR1 and PR2 - both has a waiting label 'waiting for code fix'. If I push to PR1, then ReviewMe will check statuses and change label to 'needs review'. But it will also update PR2, even if that didn't get a push (since push, merge, changes status on all PR's), so it will change the label to 'needs review' on PR2 too, even when there was no activity on that PR. It should only check if there was activity on the PR, and only then change labels if so. An exception to this is that is should always detect merge conflicts, if opt-in.

z0al commented 6 years ago

I've pushed new changes to #5 so that ReviewMe will only run checks against the related PR

johlju commented 6 years ago

@z0al Cool! Looking forward trying this out. Thanks for putting in the work on this! 😃