New rules for Merge Request acceptance (PSC meeting 2018-02-22)

new rules to accept Merge Request : core dev can vote, only two +1 votes, etc.

added 1 commit

• e66617e9 - Update CONTRIBUTING.md after PSC meeting (2018-02-22)

Compare with previous version

There is still some point to precise. I think that these 3 points must be described in the issue, and some times in the merge request if there is no initial issue.

Merge Request can be linked to an issue and should describe :

1. What changes will be made and why they will make a better Orfeo ToolBox
2. When will those changes be available (target release or date)
3. Who will be developing the proposed changes

Agreed, these items come from the former "Request for Comments" and should be placed in the issue section.

added 1 commit

• a4dd4c51 - Simplification : some text related to former "requests for comments" has been deleted.

Compare with previous version

Is there an easy way to get the list of core developers?

I don't know : there are several categories in gitlab : "admin", "external", etc. Maybe we can add a category "core dev". @gpasero : do you know if it's possible ?

Otherwise, it's not so difficult to check that merge request acceptance has been made by the right persons.

In the project orfeotoolbox/otb, go to "Settings->Members" and you can check the list of members that are either "Developer" or "Master". No need to make an additional category I think.

added 1 commit

• 7e38156c - Add definition of core developers

Compare with previous version

added 1 commit

• 5fb9e141 - Add definition of core developers

Compare with previous version

added 1 commit

• 85d3d9b4 - change definition : "master" members include PSC members and other core developers

Compare with previous version

You are right : I slightly changed the definition in PSC.md & Contributing.md

Ok so:

• Developer -> any OTB developer
• Master -> core developers + PSC members

This merge request has 3 votes (from PSC members), if there is nothing else to add, it could be merged.

This is a tiny thing, but with the current voting process, anyone can a MR, even if they're not a core developer. This may or may not make counting votes harder.

An alternative I've seen used is to add a list of the core developers to the MR template, where each can tick/sign off the MR., e.g.

• @gpasero
• @poughov
• the others here, not naming them to avoid triggering notifications

why not, but if anyone can tick the boxes, the problem is the same.

I prefer using the voting process as it is, we don't have a lot of voters anyway so it is easy for now. The checklist option may be re-considered if the number of voters increases.

I like the idea of @lnicola with the list in the template but I agree that if anyone can tick it will not solve the problem. But do we really have a problem here? What I would like to suggest here is to keep things simple and open:

• anyone can vote for a merge request, but there is a clear description of what it means in otb statement (you've understand the MR, review it and you personally validate the contribution and agree to merge it in OTB)
• at the end of the contribution process there is a "core developer" who merge the MR in the develop branch and who somehow is also "responsible" for it

I'm even not sure that we need to keep the notion of "core developers" or "master" in OTB statement especially during the review process. IMO, it does not really makes sense as for most MR any reviews and votes from anybody are welcomed and valuable (and we're not that many to votes anyway for now).

Note also that participate to MR reviews can also be a good start to get involve in the project. For example, @lnicola is officially a committer (or core developer) for only few weeks but his experience in the project make me think that he should have been able to review and vote for MR for years without problems.

I think that if problems arises the project will be able to deal with it (and perhaps adapt the contribution process if necessary).

my 2 cents,

but if anyone can tick the boxes, the problem is the same.

I wasn't necessarily worried about malicious actors. For example, it looks like I can merge MRs, even though I shouldn't be able to. (This might be solved by configuring GitLab permissions, or by making develop a protected branch).

It's just that "liking" doesn't necessary imply voting for acceptance. A random passer-by might a MR.

Who should be able to merge a MR? As you're an official committer I think you should too. The other option as you suggest is to protect the develop branch (perhaps a good option).

You're right about the possible confusion between 'liking' and voting for acceptance. Don't have a good solution instead of what you're proposing with the check box.

I think the current required level to merge a MR is Developer, so we do not need to protect the develop branch. We could restrict this to master only, but then less people will be able to merge ...

Regarding the like button, can we make clear somewhere that it is the way votes should be expressed ? If you hovers the button, you see who voted, so tick boxes are redundant I think.

mentioned in merge request !30 (merged)