I have requested INFRA take the steps as outlined in this ticket: https://issues.apache.org/jira/browse/INFRA-14991
-Joan ----- Original Message ----- From: "Nick North" <nort...@gmail.com> To: dev@couchdb.apache.org, "Joan Touzet" <woh...@apache.org> Sent: Friday, 18 August, 2017 4:48:48 PM Subject: Re: [PROPOSAL] Disallow all merges of PRs to master that cause tests to fail 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. :) >>
