Merging pull requests

[swhite2401]

Hello all,

Recently we have had issues with merged PR that had not been thoroughly checked by reviewers. While reviewing is time consuming and it is understandable that it is slow because we all have other priorities, I think it is absolutely essential and should be enforced.

I would therefore like to propose to restrict a bit more the master branch with the following rules: -A PR cannot be merged without at least one approval -this approval has to come from another person than the one who did the last push

The main drawback is that it will slow down the integration of new features, but should in theory prevent conflicting situations.

This is very easily done in the github settings (2 check boxes). So let me know if you agree and I do it.

Cheers, Simon

[MJGaughran]

It is very difficult to manually validate new features, especially if you are unfamiliar with the code that has been modified. I do not believe most people who review code will actually test it themselves.

I agree to the above changes, but suggest that we look at e.g. improving our use of automated testing for the future. For example, the recently merged bug fixes could have been accompanied by appropriate unit tests. Over time, this will give us greater confidence in our code.

[MJGaughran]

It is not entirely clear what issues you are referring to, so my comment may be slightly off-topic. I assume it comes from the discussion in #583 .

[simoneliuzzo]

Dear @MJGaughran, I do. Every time I am asked to review code I write a few tests to try it and make sure it works. This takes a lot of time and it is not fun. In my opinion it would be better done by those who write the code. The reviewers would then run the code and propose modifications for the code and the related test. All the work to write tests for every pull request makes me often either: postpone reviews or remove myself from reviews.

[MJGaughran]

Yes, @simoneliuzzo , the developer should be the one to write the unit tests. We could have requirements for both bug fixes as well as new features as to the % coverage expected etc.

We should be wary of introducing too many restrictions that only end up halting development. Requiring tests for small bug fixes is an easy first hurdle, but I think we can go further than that.

[lfarv]

There could be some mitigation on these new rules:

• there is a need for a timeout for approvals, otherwise, depending on the reviewers activities, it may block development
• there must be a possibility of emergency access for critical bug fixes

[lfarv]

-this approval has to come from another person than the one who did the last push

This makes sense when several people work simultaneously on the same branch, but usually in AT only the initiator of the pull request pushes commits, so the reviewer is always someone else. But it does not harm

[swhite2401]

@MJGaughran I fully support more test, and I admit that I need to improve on this. Since there is no strong opposition I propose to activate it for the moment. If we think that this is limiting in any way we can reconsider, as I said it is just a couple check boxes.

[swhite2401]

Master branch protection rules have bee modified