Repo Rules

[scoopxyz]

Hello,

We've talked about it before, but I'd just like to formalize the discussion and the changes aren't controlled by a Pull Request for review, so...

I'm looking to add two rules to the OCIO repo for PRs.

1. 2 Approvals required for PRs
2. Require Travis and Appveyor CI build tests to pass

Yay / Nay ?

[scoopxyz]

@michdolan @Shootfast @hodoulp @lgritz

[Shootfast]

Yay

[dbr]

The 2 approvals presumably excludes the PR author; but does it exclude the merge-author? Minor point, but difference between needing 3 people in-favour versus 4

Either way, yay

[dbr]

Also these points can be written in a CONTRIBUTING document and Github will flag them to people when they create pull requests, https://help.github.com/articles/setting-guidelines-for-repository-contributors/

[scoopxyz]

It would be two people exclusive of the author (who presumably approves of his/her own contributions)

There is also the ability to add a CODEOWNERS file which would auto-add different reviewers for different sections of the codebase. A bit complicated for what we have right now, but might be nice to add @hodoulp and @BernardLefebvre to potentially ASDK critical code paths for example.

[hodoulp]

We already discussed about having rules to validate a pull request, and that’s a great idea to define the rules and document them. Here at Autodesk, we have rules to govern reviews and merges. These rules are mainly like the proposed one. However, all employees being full time, there is also a time constraint. The implicit agreement is that a first pass review must be done with 1-2 days (followed by any number of passes to reach a final approval). The goal is to avoid stalling reviews and/or merges when others or author next jobs could need the work.

The challenge with OpenColorIO project seems to have review comments and/or approval (or not) within a reasonable time. For example, there are some ‘orphan’ pull requests with no comment and/or no clear status (i.e. approved or rejected) for a long time. On the other hand, some pull requests have comments and/or questions without any follow-up from the reviewer (i.e. when the change is done, or the answer is documented). So, I think that the implementation of the principle is challenging and needs some thinking.

My point is that we should document the rules, and at the same time, explain the process to implement these rules.

The merge part of the process is already under control as only few people have the appropriate rights. The review part is open to everyone which implies having some time to let more eyes reviewing the work and at the same, is currently the challenging part. We could perhaps propose that some of us (i.e. those with merge capability?) regularly (once a week or two weeks?) scan the existing pull requests to ensure a feedback and/or follow-up when needed.

[hodoulp]

At Autodesk, the github settings include a rule to disable the merge if the CI builds failed. That's could be interesting for OCIO too.

[michdolan]

Yay. Definitely. I think as long as a PR has two non-author approvals, and all CI builds have passed, it doesn't really matter who does the merge.

I do agree with @hodoulp about timing though, especially since we are fortunate enough to have full time engineers on the project. In the near term, if @hodoulp or @BernardLefebvre are blocked from continuing work because of stalled PRs, we might want to have a special tag that flags the PR (e.g. "blocking") to communicate that (and of course a nudge from the author).

One other consideration about timing is the spread of time zones represented within the community. We may want to have a policy of a minimum time period a PR has to be open prior to allowing a merge (even with 2 approvals). 48 hours would give enough time for all time zones to have reasonable lead time, for example.

[lgritz]

My (totally non-binding) $0.02:

I think number of approvals is less important than seniority of approvers (to a point), and that the size/risk of the change should be factored in, and also the length of time that the PR has been open to discussion.

For example, a more nuanced set of rules might be:

• Core design decisions, large new features, or anything that might be perceived as changing the overall direction of the project should be discussed at length in the mail list before any PR is submitted, in order to (a) solicit feedback, (b) try to get as much consensus as possible, and (c) alert all the stakeholders to be on the lookout for the eventual PR when it appears.

• Small changes (bug fixes, docs, tests, cleanups) can be okayed and merged by a single senior committer.

• Big changes that can alter behavior, add major features, or present a high degree of risk should be signed off by TWO senior (commit-privileged) people, ideally one of whom should be the "owner" for that section of the codebase (if there is such a thing). If the person submitting the PR is him/herself the "owner" of that section of the codebase, then only one additional senior approval is sufficient. But in either case, I think a 48 hour minimum is helpful to give everybody a chance to see it, unless it's a critical emergency fix (which would probably put it in the previous "small fix" category anyway, not a "big feature").

• Escape valve: Big changes can nonetheless be merged by a single senior (commit-privileged) developer if the PR has been open for over two weeks without any unaddressed objections from senior developers. At some point, we have to assume that the people who know and care are monitoring the PRs and that an extended period without objections is really assent.

And absolutely, CI pass should be mandatory. (In fact, I think it's even fine for everybody to avoid spending time reviewing a PR that doesn't pass CI as a minimum requirement.)

Also, there should be a clear list of who owns which sections of the codebase (if there are such things), and who the senior committers are.

[michdolan]

Great points @lgritz

[hodoulp]

I like that @lgritz, and the blocking label idea.

[KelSolaar]

Excellent points Larry!

CI pass should be mandatory. (In fact, I think it's even fine for everybody to avoid spending time reviewing a PR that doesn't pass CI as a minimum requirement.)

Are there any plan to add coverage tests in the CI process? Getting 100% coverage is daunting task and I merge very often PR that don't have 100% coverage.

[lgritz]

Maybe certain core classes are designated as needing very close to full code coverage in the tests? But true 100% coverage across the whole codebase is really, really hard. (Example: How much effort should be put into making a CI test that guarantees coverage of the code that prints the error message for a file write that can only fail when the disk volume is full?) Some people are zealots for getting 100% coverage, but I think some balance is necessary -- you can easily either tie yourself in knots to do it, as well as take away time from other fixes and development. It's a risk/reward tradeoff.

But I do think that if someone adds a new feature, the reviewers should be considering whether a test of some kind should be part of the review, and if somebody fixes a tricky bug that escaped the test suite the first time, ideally the fix ought to come along with a regression test. Again, some judgment is necessary.

[KelSolaar]

But true 100% coverage across the whole codebase is really, really hard.

Yes exactly, but I could see how easily changes introduced could also lower the coverage and thus if coverage is a metric for successful CI (as we do for Colour), then the PR by virtue of CI pass should be mandatory should not be merged.

[hodoulp]

If the community agrees with the Larry's proposal, one could document it in a CONTRIBUTING document as proposed by @dbr

[scoopxyz]

Cool, thanks for the discussion everyone, it seems like we're on board. So I'll consider it approved, and will just work out the details of execution.

To address some of the notes from above:

• @dbr - Yes, will do CONTRIBUTING doc
• @hodoulp - Yes, we'll ensure CI passes before a merge
• We already have "Defect: Critical" lables, can change them to generic ["Critial", "Major", "Minor"] for uniformity ( I don't like millions of tags)
• @lgritz I will definitely codify your logic in the contirbution doc, the orignally question was mostly around GitHub built-in functionality where the website literally prevents merging unless there are X number of approvals
• @lgritz The CODEOWNERS mechanism I mentioned above allows you to tag subdirectories as having different owners, and auto-adds them as reviewers if any PR touches that corner of the code. I obviously have never used it in practice, so we'd have to try it out and see
• @KelSolaar coverage is definitely a good thing to aim for, I think we should address tracking it (coveralls, etc) with the future goals of static analysis checks, common code formatting, et. al.

[scoopxyz]

Updated.

[scoopxyz]

Implemented:

• Require 2 Reviewers (administrators can bypass this requirement)
• Require both TravisCI and Appveyor to pass

Due to the administrator-bypass-ability, should we also cull the list of admins :smile_cat:

[hodoulp]

My understanding of the Larry's comment that we agreed on, was to have some flexibility depending of the change. The github change always imposes 2 reviewers. The recent history clearly demonstrates that it could be challenging to have.

[scoopxyz]

Yeah, that's more of a soft rule, mutually agreed behavior. The hard rule of 2 reviewers that I just added can currently be ignored by everyone that has merge rights right now, so in practice, it doesn't change anything... but promotes that behavior and allows the tiers of project ownership to grow over time.

Something akin to:

• Owner (head honcho(s) that could theoretically delete the whole repo)
• Admin (can change repo rules/structure like I just did, and can bypass review rules)
• Collaborator (has merge rights if merge rules satisfied)
• Contributor (all our lovely community helping hands)

[scoopxyz]

edit: all the current "Admin" can bypass rules, which includes @Shootfast @michdolan @scoopxyz @lgritz

[hodoulp]

The point is only to highlight that recent history demonstrates the challenge to have two reviewers who approved a pull request so it leads to stalled pull requests. Looking at the last few week history, the most active reviewers were @michdolan and @hodoulp and it would be inelegant for me to approve Autodesk branches.

[scoopxyz]

I understand, and think we should aim for better, that's the point of this Issue.

We definitely have a losing battle with time though, two/three full time developers vs. three weekend warriors. Its my hope however that by GitHub now saying "At least 2 approving reviews are required to merge this pull request." and suggestions for reviewers we can at least A be aware that there is PR at all B spend a minute to at least see whats included in the changes and say "Yay". You can also label the PRs {'Critical', 'Major', 'Minor'} to focus attention.

I don't think there is any cause for alarm by these changes. Any single Admin user can still merge stuff without any reviews. So unless you mentally subscribe to my merge philosophy, nothing changes.

[hodoulp]

We definitely have a losing battle with time though, two/three full time developers vs. three weekend warriors.

1. You perfectly describe the challenge we currently have. Anyway, let's give a try :+1:

2. {'Critical', 'Major', 'Minor'} -> {'Blocking', 'Critical', 'Major', 'Minor'} as blocking is a much more appropriate keyword to describe the PR #520 versus all other PR's