Owners/Collaborators merging in their own changes to master

[Scott Jones]

In the startup where I'm now consulting, we've actually disallowed that... any PR needs to have at least 2 reviews, and been approved by the reviewers, before someone (not the author of the change) can push the button to merge it into master.

I've seen some changes go in without any review, merged in by their author, only to have issues show up later.

I wonder if it would be a good idea for JuliaLang to adopt a more formal review process, like we've done?

(This is NOT meant as a criticism of anybody... nobody is perfect [not even me! :-) ], and it's just a measure to help improve

stability of the code [and make sure other pieces like documentation, regression testing, and performance regression testing

get done])

[Randy Zwitch]

Mine is sort of a tangent to this, but it does feel weird that 'master' and the tagged version in METADATA are usually different, with "try master" often being a suggestion to fix problems. I've been using Julia long enough to know this trick, but it feels like a poor experience for newer users.

Why do people push changes to master and not tag a new version? Is there something I'm missing with this behavior?

[Stefan Karpinski]

Not every changes needs two reviewers – some are obvious fixes, e.g. documentation, typos in code, etc. I do think, however, that there have been cases where people have merged risky changes without going through a pull request (i.e. pushing directly to master), which has broken things on various platforms. This is quite bad and there's generally been push back when this has happened, so hopefully that will stop happening. I can't think of any cases where a pull request has actually been made, passed all CI, and been merged without sufficient review in a problematic way.

[Stefan Karpinski]

I'm not sure why this happens, but tagging new version is fairly easy, which is the only way I can think of to improve this. I could imagine if we had a better CI infrastructure that the CI infrastructure could, once a day per package, check master and see if it passes all tests and works well with other packages and ping the owner / collaborators to suggest tagging a pre-approved new version.

[Scott Jones]

I've found two (minor) issues just this week... one, an incorrect error message (in that case, I think it had only one reviewer, I found the mistake after it had already been merged), and the other case, the documentation hadn't been changed to reflect a breaking change in an API.  Come to think of it, that change probably needs something in NEWS.md as well...

What I've seen, is that even for fixing typos in code, or documentation, etc., that one pair of eyes reviewing just isn't enough... of course, maybe I'm paranoid, because I'm used to developing software that *has* to be pretty much 100% reliable (for medical software).

Also, this isn't meant as a criticism of anybody... I always want two or more reviewers on any of *my* code before it goes in as well, for Julia or for work.

[Keno Fischer]

I think a good improvement would be a system where anybody can suggest tagging a version and the owner of the repository can approve it. A lot of the time the reason I fall behind in tagging a version is that I don't have it locally installed anymore and the reason there's new stuff is that somebody made a pull request. This almost works by making a METADATA pull request, but the problem is that you also need to create tags in the actual repository, which only collaborators can do. If we had a bot that automatically tagged repositories if the METADATA pull request is merged, that would be very helpful.

[John Myles White]

+1 to Keno's suggestion

Tagging new versions is actually a lot of work because of various CI false positives, so I can't tell if a person's bug fixes are safe to both merge and release to the public. I've been encouraging people to submit updates to METADATA for my packages, but I think we need to make this easier and have clearer cultural guidelines for what tagging involves.

 -- John