Sorry to raise false hopes. I'm +1 on the proposal. Nick
On Fri, 18 Aug 2017 at 19:05 Joan Touzet <woh...@apache.org> wrote: > Nope, GitHub doesn't allow this - I just tried. > > So my proposal at present remains: > > > > https://help.github.com/assets/images/help/repository/protecting-branch-loose-status.png > > > > Specifically: > > > > Protect master branch (and any release branches like 2.1.x) > > Require status checks to pass before merging > > Require branches be up to date before merging > > since this does not mandate 2 humans to get changes onto master. > > We can ratchet things down farther when our committer base gets more > active in reviewing PRs with the "Require pull request reviews before > merging" setting. > > If there are no objections raised over the weekend, I will file the > INFRA ticket to have these settings changed on our primary repos > where Travis CI is enabled: > > couchdb > couchdb-fauxton > couchdb-documentation > couchdb-nano > couchdb-docker > > I will be explicitly leaving out couchdb-pkg, since CI doesn't (yet) > make sense for it. > > -Joan > > ----- Original Message ----- > From: "Nick North" <nort...@gmail.com> > To: "Joan Touzet" <woh...@apache.org> > Cc: dev@couchdb.apache.org > Sent: Friday, 18 August, 2017 12:42:19 PM > Subject: Re: [DISCUSSION] Disallow all merges of PRs to master that cause > tests to fail > > I'm not sure if you can on GitHub, so that would need checking. We're > using the Microsoft TFS Git system, for historical reasons. > > Nick > > > On 18 Aug 2017, at 16:55, Joan Touzet <woh...@apache.org> wrote: > > > > I didn't realize you could review your own PR. That gives us the "escape > > hatch" that we need. > > > > -Joan > > > > ----- Original Message ----- > > From: "Nick North" <nort...@gmail.com> > > To: dev@couchdb.apache.org, "Joan Touzet" <woh...@apache.org> > > Sent: Friday, 18 August, 2017 9:48:40 AM > > Subject: Re: [DISCUSSION] Disallow all merges of PRs to master that > cause tests to fail > > > > > > This is pretty much the set of restrictions we have on the master branch > in my organisation, and it works well. We also require PR reviews before > merging, but anyone in the team can do the review, including the PR author. > This means the author has to make a conscious decision on whether the > changes are trivial enough to sign off themselves, or whether someone else > should review them, and there's an audit trail of that decision being made. > > > > > > Nick > > > > > > > > On Wed, 16 Aug 2017 at 23:49 Joan Touzet < woh...@apache.org > wrote: > > > > > > Seems there is general consensus. > > > > Now, how do people feel about me asking Infra to make this change to the > > main repos (couchdb, couchdb-fauxton, etc.): > > > > > https://help.github.com/assets/images/help/repository/protecting-branch-loose-status.png > > > > Specifically: > > > > Protect master branch (and any release branches like 2.1.x) > > Require status checks to pass before merging > > Require branches be up to date before merging > > > > We can have an optional secondary discussion around enforcing: > > > > Require pull request reviews before merging > > > > This would enforce our RTC model, but we *need* more active devs if this > > is going to pass. I've had to beg multiple times for many of my PRs in > > the 2.1.0 release cycle to be approved...even trivial documentation > > changes. It was very frustrating. > > > > -Joan > > > > ----- Original Message ----- > > From: "Nick Vatamaniuc" < vatam...@gmail.com > > > To: dev@couchdb.apache.org > > Sent: Wednesday, 16 August, 2017 6:01:34 PM > > Subject: Re: [DISCUSSION] Disallow all merges of PRs to master that > cause tests to fail > > > > +1 > > > >> On Aug 16, 2017 15:50, "Alexander Shorin" < kxe...@gmail.com > wrote: > >> > >> It's strange to say something else than +1 or question the topic in any > >> way. > >> > >> Good call, Joan! > >> -- > >> ,,,^..^,,, > >> > >> > >>> On Wed, Aug 16, 2017 at 6:46 AM, Joan Touzet < woh...@apache.org > > wrote: > >>> Hi committers, > >>> > >>> I'd like to propose a change to our policy on version control, namely > >>> that no check-ins be allowed on the master branch unless CI test runs > >>> against that PR are clean. > >>> > >>> We've worked hard as a group to get runs clean. We need to protect > >>> that achievement and investment in our test suite. That means not > >>> letting rogue check-ins slip by because we are ignoring a red X in > >>> GitHub (GH) from the Travis run. > >>> > >>> Things I see as exceptions: > >>> * Changes to things clearly not related to the test suite, i.e. > >>> documentation, support scripts, rel/overlay/etc/ files, etc. > >>> * Changes already agreed upon in a previous PR/discussion for > >>> administrative tasks > >>> > >>> Interesting situation right now for a discussion: Garren has a PR up[1] > >>> that enables the mango tests to be part of the standard Travis/Jenkins > >>> runs. Unfortunately, it doesn't pass on one of our platforms right now > >>> and that needs investigation. Should we allow the PR to land and fix > >>> the problems in master, or should the PR hold-up until it can land > along > >>> with the fixes for the failing mango tests? I can see both sides of > this > >>> argument. > >>> > >>> It may or may not be possible for our GH setup to actually prevent such > >>> checkins (the Apache GH setup is somewhat restricted, and various > things > >>> like commit hooks and webhooks have to be configured by INFRA, not us). > >>> > >>> I'd like to further discuss whether people feel such a hook would be > >>> acceptable, onerous or otherwise. Personally, I worry that such a setup > >>> might prevent us from checking in some of the exceptions above, but if > >>> there is a way around it, we could proceed down that path. > >>> > >>> What do you think, sirs?[2] > >>> Joan > >>> > >>> > >>> [1]: https://github.com/apache/couchdb/pull/753 > >>> [2]: It's a Mystery Science Theatre 3000 Joel reference. :) > >> >
