while we’re using GitHub pull requests more and more to review code, this is not always the case and also not the original development model of XWiki. Instead, Committers can directly commit to master. At the same time, we also ask Committers to review all changes in the repository. If there is anything to remark, usually comments are added on the commit on GitHub. Unfortunately, we have currently no way to a) track if somebody has already reviewed a commit and b) if feedback on a commit has been addressed. This has several disadvantages:
As a committer, I have no idea which commits have already been reviewed. I can pick some at random and review them, but there is a high chance that many commits aren’t reviewed at all and some are reviewed many times. This makes it hard to ensure any kind of quality control.
It is easy to miss feedback and there is no easy way for a reviewer to see and verify that their feedback has been addressed.
I recently stumbled upon this article about Non-Blocking, Continuous Code Reviews that basically describes this kind of review process where code is committed without any prior review and reviewed afterward. The article also mentions tooling for this process and in particular mentions ex-remit, a tool developed for this purpose by the dev team behind Auctionet. This tool seems to implement exactly the two features I mentioned on top of GitHub commit comments - marking comments as resolved, marking committs as reviewed, and showing feedback you received or participated in. Unfortunately, there is not much documentation around it and the only deployment instructions are for Heroku. It seems that there is a Docker image, though. It’s also using a tech stack we don’t have any experience with afaik (Elixir/Phoenix).
Before we put any more work into this I would like to ask you for your feedback - do you agree that such tooling would be helpful, and if yes, do you agree regarding the features or do you see different needs?
EDIT: I misunderstood the application scope of this tool /EDIT
I’ll give my opinion as someone opening regularly PRs for reviews
I have notifications on all of my PRs to email me as soon as there’s feedback or activity on my PRs. It’s not that difficult to see feedback.
We could make it a practice for the PR creator to ping the reviewer when there’s feedback. I do not do it as of today mostly for the sake of avoiding notification overload for the reviewer.
In order to make it easier for reviewers to check that their feedback have been addressed, I try to systematically link the commit with the fix in an answer to the comment where the feedback happened.
I had a course on Elixir a couple years ago, never thought it could end up being useful
From what I remember, Elixir is quite difficult to get a grasp on. Very powerful but contains many complex abstractions.
Such tooling would be definitely useful for point 1.
IMO point 2. could also be pretty much solved with a couple practices we agree to follow when reviewing/fixing PRs.
Ex-remit could also be useful to check tests. Most PRs are tested. However, sometimes it’s a lot of tests and some fixes are small so I don’t systematically rerun all the tests on all commits. Ex-remit would help me track the last commit I ran each test on, and make sure to run the tests regularly. Too many times tests pass with the initial PR, I rework things and forget to check the appropriate tests again. E.g. XWIKI-21452: Macros info, success, warning and error are only distinguished by colors by Sereza7 · Pull Request #3023 · xwiki/xwiki-platform · GitHub had 2hours worth of tests, I checked them initially but didn’t reconsider the whole list of tests after any other commit. I’m not saying it would solve everything, but it would make it easier to track tests without posting a giant non-collapsable comment in between each commit.
Some PRs can have a pretty long comment history and that would also help keep things nice and tidy (having to scroll through two month worth of discussion to get to the comment you’re looking for is not very efficient).
Just to avoid further confusion, this thread is not about our use of pull requests and tooling around pull requests. This thread is only about comments on commits that are already on the master branch like this example on xwiki-commons. There are email notifications for these kinds of comments but if you don’t systematically keep track of such comments, it’s easy to lose track and to forget them.
I’m a bit mixed. I’ve the feeling that we started to move using more pull requests than before, which tend to decrease a bit the need for such review by commit. Now we still perform direct commits, and for those I agree with you about the problems you mentioned. But right now I’ve no idea if maintaining another tool worth it. And it’s difficult to answer that question when looking at remit, since their doc is a bit vague: I’m not sure to see yet how the tool would work within a team to be honest.
So I guess we can give it a try in a hackathon or something like that, just to see how it looks / if it’s easy to use / maintain, and if it helps us.
<history>
I have used Cenqua Crucible in the past (was bought by Atlassian) and it was quite nice. AFAIR we even had Atlassian set up a fisheye+crucible instance for XWiki at http://fisheye2.atlassian.com/browse/xwiki/ (no longer exists). I don’t remember why it stopped though. Maybe when we decided to move to GitHub. </history>
Yes I agree that such tools would be nice. Now I’m not sure how much time to spend on them vs other stuff. If someone is willing to spend the time and set it up, I see no problem. Maybe research the topic a bit more first.
We could also list commit comments using REST API endpoints for commit comments - GitHub Docs and if there’s no response send a notification email or something like this. We would need to define where to put it (in the release plan app, on xwiki.org in a page, in the ci pipeline, etc).
