@tsteur opened this Issue on March 8th 2021 Member

Hi everyone, currently it's not always clear which pull requests needs another review. Especially when there are several iterations of changes during a review.

Currently, once a pull request is ready for a review we assign the label Needs Review. Then someone reviews it and the issue stays in Needs Review until merged. This makes it hard to know when the issue needs yet another review. You often have to click into the same issue several times to check if there's an update and if it actually does need another review or if we are waiting for the author to make changes.

I propose that once an issue has been reviewed, we remove the Needs Review label, and maybe add some other label like WIP work in progress or Changes Requested or change it to draft or simply don't add another label. Then, once the PR author made the required changes the label Needs Review would be added again.

I think this would make it more clear which reviews actually need our attention currently since reviews have pretty much highest priority for us.

Any thoughts or ideas @diosmosis @Findus23 @flamisz @sgiehl @mattab ?

@mattab commented on March 8th 2021 Member

btw maybe the github review feature could be leveraged? eg. https://docs.github.com/en/github/managing-your-work-on-github/filtering-pull-requests-by-review-status (i'm not sure if it meets the needs but maybe it could)

@tsteur commented on March 8th 2021 Member

@mattab it doesn't really help unfortunately

@diosmosis commented on March 8th 2021 Member

:+1: could help. would make things clearer for me at least (though it's not a big time sink for me atm to just look through all the PRs in the current milestone and see what can be merged/what needs reviews)

@sgiehl commented on March 8th 2021 Member

Sounds fine I guess.
we could maybe also use the review states more actively. Once the required changes have been done by the author it might maybe also be helpful to dismiss reviews that should have been solved with new commits. If multiple peoples are doing reivew we might otherwise end up with having multiple states from different reviewes within one PR.

@flamisz commented on March 9th 2021 Contributor

@diosmosis comment about the milestone is very helpful too:

for me atm to just look through all the PRs in the current milestone and see what can be merged/what needs reviews

It means if I work on something which has a milestone in the future but finished earlier, I should set the milestone to the very next one?

I like the idea to set/unset labels. As the team grows, more and more active PRs will have, this could really help quickly go through.

@diosmosis commented on March 9th 2021 Member

@flamisz no, but it would help to set the milestone to the same one as the issue. What goes into the next milestone has to do with the value a change might bring & the potential cost of introducing it into live systems & making it stable, rather than what is coded and ready to go at any given moment. And @mattab & @tsteur are the ones in charge w/ prioritization at this level, so we wouldn't change that unless we knew it was a valuable enough or a small enough change.

@tsteur commented on March 12th 2021 Member

trying to summarise the thoughts so far from previous comments:

  • The PR author adds "Needs Review" label and assigns the PR to a milestone (usually the same milestone as the referenced issue) once the PR is ready for a review
  • The reviewer selects "Approve / Reject changes / Comment" when finishing a review so it's clear if a
    PR is accepted or more changes are needed
  • The PR author marks an individual review conversion/comment as resolved when the feedback was applied or notices/acknowledged (not always do need changes to be made for each comment as sometimes they are suggestions etc)
    • If further feedback is needed for the conversation/comment then PR author will ask the reviewer to confirm the conversation is resolved
  • The reviewer removes the label "needs review" when changes are required. No new label is added for now for simplicity (but could also add on)
  • The PR author adds the label "needs review" again once it's ready for another review
  • When a PR is approved and no further review required then we aim to directly merge the PR if the tests pass
    • If a PR is approved overall but tests are failing then we might need another round of review (meaning reject the PR and remove needs review label)
    • unless it is only needed to update the expected test results then maybe the PR author could later fix the build and merge directly without yet another review (or maybe only if the PR reviewer mentions that only test )... could clarify this further in a team meeting some day

Does this overall sound in the right direction? Any thoughts / objections? Also of course we can change/improve things any time .

Overall the goal again be to easily be able to see which issues currently need a review etc and trying to get approved PRs merged as quick as possible (for example so less potential merge conflicts happen etc)

@flamisz commented on March 12th 2021 Contributor

I like it, it's a very clean and simple workflow.

@tsteur commented on March 15th 2021 Member

Great, I've put this into the docs and I'll close this for now and we can change it any time. Any feedback any time be greatly appreciated. :)

This Issue was closed on March 15th 2021
Powered by GitHub Issue Mirror