|
| 1 | +Pull request review workflow |
| 2 | +============================ |
| 3 | + |
| 4 | +Code changes can be sent as pull requests (PR) in the Github web UI. Some |
| 5 | +integration tests are run and then the PR waits for a review, approval and |
| 6 | +merge. |
| 7 | + |
| 8 | +Open the pull requests against branch **devel** (against *master* branch it's |
| 9 | +also possible but this may miss other development changes). |
| 10 | + |
| 11 | +The merge strategy is to *rebase and merge*. This means that the changes are |
| 12 | +applied on top of the current development branch which is **devel**, although |
| 13 | +they could have been originally based on a different commit in your local |
| 14 | +repository. |
| 15 | + |
| 16 | +Reviewer |
| 17 | +-------- |
| 18 | + |
| 19 | +In the pull request tab *File changes* go through the diff. If you want to leave |
| 20 | +a comment for a particular line, click the plus in a blue box (``[+]``) . Write |
| 21 | +text about the problem, what needs to be fixed or change. Clarification |
| 22 | +requests are also ok (not necessarily leading to a change). This will add a |
| 23 | +single review comment/item. |
| 24 | + |
| 25 | +Adding such comments will add them to the PR code but it's not visible to the |
| 26 | +submitter until you click a *Review changes* at the top of the file diffs. Once |
| 27 | +all comments are written for this round, they need to be submitted by writing |
| 28 | +the summary review comment. |
| 29 | + |
| 30 | +Approved |
| 31 | +^^^^^^^^ |
| 32 | + |
| 33 | +If everything is OK and no review comments need to be resolved, write a comment |
| 34 | +and approve the whole PR. This will be noted in the *Conversation* as a comment |
| 35 | +and visible in the PR list with *Approved* text. |
| 36 | + |
| 37 | +Changes requested |
| 38 | +^^^^^^^^^^^^^^^^^ |
| 39 | + |
| 40 | +Assuming there are review comments, this will mark the whole PR, a comment is |
| 41 | +added to the *Conversation* (publishing the comments). |
| 42 | + |
| 43 | +Submitter |
| 44 | +--------- |
| 45 | + |
| 46 | +If you have email notifications on, you'll get notified about new review |
| 47 | +comments or about PR status changes (like that it got merged). |
| 48 | + |
| 49 | +Please check the review comments and either explain why things need to be done |
| 50 | +in such and such way or fix it in your code and mark the comment as *Resolved*. |
| 51 | +Any changes in the code need to be done locally and then pushed to your |
| 52 | +repository, the updates will be logged in the overview. |
| 53 | + |
| 54 | +Review comments on lines that did not change will be probably visible in the |
| 55 | +new branch updates, resolved on changed lines will disappear (but will be still |
| 56 | +listed in the previous review summary). |
| 57 | + |
| 58 | +Checklist |
| 59 | +--------- |
| 60 | + |
| 61 | +* set the desired target *Milestone* before closing |
| 62 | +* check the result in git branch after merge for potential clashing changes |
| 63 | + that were not discovered in the meantime |
| 64 | +* you can still fix up code after merge and push, but do this carefully so it |
| 65 | + does not overwrite other changes |
| 66 | +* review comments can be *Unresolved*, use that cautiously so it does not cause |
| 67 | + confusion |
| 68 | + |
| 69 | +References |
| 70 | +---------- |
| 71 | + |
| 72 | +* https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews |
| 73 | +* https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request |
0 commit comments