-1 I really don't think this needs to be codified. If people are merging red PRs, that is a failing as a responsible developer. Defensive programming is all well and good, but this seems like it's a bit beyond the pale in that regard. I foresee it making the correction of a red main pipeline must more difficult that it needs to be. And while it's much better than it had been, I am (anecdotally) still seeing plenty of flakiness in my PRs, either resulting from infra failures or test classes that need to be refactored or reevaluated.
If someone is merging truly broken code that has failed precheckin, that seems to me to be a discussion to have with that person. <s> If it persists, perhaps a public flogging would be in order. </s> But at the end of the day, the onus is on us to be responsible developers, and no amount of gatekeeping is going to be a substitute for that. On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan <gosulli...@pivotal.io> wrote: > I'm in favor of this change, but only if we have a way to restart failing > PR builds without being the PR submitter. Any committer should be able to > restart the build. The pipeline is still flaky enough and I want to avoid > the situation where a new contributor is asked repeatedly to push empty > commits to trigger a PR build (in between actual PR review) and their PR > gets delayed by days if not weeks. That's a real bad experience for the > people we want to be inviting in. > > On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann <amurm...@pivotal.io> > wrote: > > > What's the general consensus on flakiness of the pipeline for this > purpose? > > If there is consensus that it's still too flaky to disable the merge > button > > on failure, we should probably consider doubling down on that issue > again. > > It's hard to tell from just looking at the dev pipeline because you > cannot > > see easily what failures were legitimate. > > > > On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt <bschucha...@pivotal.io > > > > wrote: > > > > > I'm in favor of this. > > > > > > Several times over the years we've made a big push to get precheckin to > > > reliably only to see rapid degradation due to crappy code being pushed > > > in the face of precheckin failures. We've just invested another half > > > year doing it again. Are we going to do things differently now? > > > Disabling the Merge button on test failure might be a good start. > > > > > > On 11/9/18 12:55 PM, Dan Smith wrote: > > > > > > > Hi all, > > > > > > > > Kirks emails reminded me - I think we are at the point now with our > PR > > > > checks where we should not be merging anything to develop that > doesn't > > > pass > > > > all of the PR checks. > > > > > > > > I propose we disable the merge button unless a PR is passing all of > the > > > > checks. If we are in agreement I'll follow up with infra to see how > to > > > make > > > > that happen. > > > > > > > > This would not completely prevent pushing directly to develop from > the > > > > command line, but since most developers seem to be using the github > > UI, I > > > > hope that it will steer people towards getting the PRs passing > instead > > of > > > > using the command line. > > > > > > > > Thoughts? > > > > -Dan > > > > > > > > > > > > >
