I'm in favour of quick reverts as well. Even though a failure might seem easy to fix at a first glance, experience has proven otherwise in many cases, so quickly reverting the offending commit seems the right thing to do. Whom should revert the offending commit?, I'd say the first person that detects the failure... It should, of course, be communicated to the originator of the PR so he/she can work towards solving the problem and submit a new PR afterwards. In my opinion nobody should be offended by a revert, as long as the person reverting the commit communicates empathetically and helps the originator of the PR to understand what was wrong, why it was reverted and, how can be solved (if needed). Maybe it's just me, but I'd feel more offended if my commit is breaking the pipeline and preventing other community members from continue working normally, than if somebody just reverts the bad changes I introduced. Cheers.
On Thu, 30 Apr 2020 at 05:18, Owen Nichols <onich...@pivotal.io> wrote: > Hi Mark, I’ve noticed some voluntarily quick reverts, which is awesome, > but other times CI has stayed broken for days, so it doesn’t feel like > we’re all on the same page. I cannot find anything in the wiki or dev list > archives to suggest this was “settled” previously, but please share a link > if I missed something. Consensus that "quick reverts are good” sounds > nice, but the details of who can notice and initiate the revert make a > difference (do we really expect every contributor to stare at CI for the > next 10 hours after their PR is merged?). > > > On Apr 29, 2020, at 8:12 PM, Mark Hanson <mhan...@pivotal.io> wrote: > > > > As far as I am aware, I think this is already a settled decision. The > decision was quick revert. > > In almost every case the build is more important than the change. > > > > Thanks, > > Mark > > > >> On Apr 29, 2020, at 4:14 PM, Robert Houghton <rhough...@pivotal.io> > wrote: > >> > >> I am very pro-revert for breaking changes. The Benchmark or Windows > tests > >> being a culprit is unfortunate, since the PR pipeline cannot tell us > about > >> those, but thats the cost of our excellence :) > >> > >> On Wed, Apr 29, 2020 at 4:06 PM Owen Nichols <onich...@pivotal.io > <mailto:onich...@pivotal.io>> wrote: > >> > >>> There are many ways a commit can break the Geode CI pipeline[1] despite > >>> our required PR checks, such as: > >>> - merge a PR that failed some non-required PR checks > >>> - merge a PR that last ran checks awhile ago and now interacts > >>> unexpectedly with other recent changes > >>> - merge a PR that fails Windows tests > >>> - merge a PR that fails Benchmarks tests > >>> > >>> When a bad commit gets through for whatever reason, what should happen > >>> next? For example: > >>> - bring it up on the dev list > >>> - someone should revert the change as soon as possible, allowing the > >>> pipeline to remain green while the issue is addressed > >>> - a reasonable amount of time should be allowed for someone to make a > >>> quick fix, otherwise revert. > >>> - the pipeline should remain broken for as long as it takes to fix, as > >>> long as there is clear communication that someone is working on it > >>> - everyone look the other way and hope it fixes itself > >>> > >>> I’m sure there are other ideas as well. A related question is *who* > can > >>> or should revert a bad commit? Only the person that merged the PR? > Only > >>> the original author of the PR? The first person to notice the problem? > >>> > >>> What is a reasonable amount of time for this to happen? 2 hours? 2 > days? > >>> 2 weeks? Does it depend whether the bad commit is also affecting PR > checks > >>> for everyone else vs only depriving downstream consumers of new Geode > >>> -SNAPSHOTs? > >>> > >>> Would you take offense if someone else reverted your commit? Does is > make > >>> a difference if your commit is exposing an existing issue (as opposed > to > >>> introducing a new bug)? > >>> > >>> Is there a perception that reverts create a lot of extra work? (they > >>> shouldn’t--just start your rework PR with a revert of the revert, then > add > >>> additional commits that resolve the issue, so reviewers can easily see > what > >>> was missing the first time) > >>> > >>> This is a discussion thread, not a vote. We trust committers to do > what’s > >>> best. Would embracing a “anyone can revert, no shame” > >>> revert-first-then-fix culture benefit our community, or is our current > >>> easygoing approach working just fine? > >>> > >>> [1] > >>> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fconcourse.apachegeode-ci.info%2Fteams%2Fmain%2Fpipelines%2Fapache-develop-main&data=02%7C01%7Chansonm%40vmware.com%7C631cbe6f65c14bbb030108d7ec931267%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637237988725318710&sdata=2ecuh535cj7G9i8STCT32zyunsnrcupg4c8dowrGwvo%3D&reserved=0 > < > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fconcourse.apachegeode-ci.info%2Fteams%2Fmain%2Fpipelines%2Fapache-develop-main&data=02%7C01%7Chansonm%40vmware.com%7C631cbe6f65c14bbb030108d7ec931267%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637237988725318710&sdata=2ecuh535cj7G9i8STCT32zyunsnrcupg4c8dowrGwvo%3D&reserved=0 > > > > -- Ju@N
