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

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

Open
johlju opened this issue Aug 7, 2018 · 7 comments
Open

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

johlju opened this issue Aug 7, 2018 · 7 comments

Comments

@johlju
Copy link

johlju commented Aug 7, 2018

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 PowerShell/DscResource.Tests#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 johlju changed the title Sets the label when I change the label Sets the review label on other event than a push event Aug 7, 2018
@johlju
Copy link
Author

johlju commented Aug 7, 2018

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.

@johlju johlju closed this as completed Aug 7, 2018
@z0al
Copy link
Owner

z0al commented Aug 7, 2018

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

johlju commented Aug 7, 2018

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

johlju commented Aug 7, 2018

@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 Support optionally removing existing labels when setting #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
Copy link
Author

johlju commented Aug 30, 2018

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

z0al commented Aug 31, 2018

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

@johlju
Copy link
Author

johlju commented Sep 4, 2018

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

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

Successfully merging a pull request may close this issue.

2 participants