In that case, why don't we make the rule that the owner of the PR is not
allowed to merge it.
Another committer must then accept and merge it.
--Udo
On 11/12/18 14:03, Dan Smith wrote:
To be clear, I don't think we have an issue of untrustworthy committers
pushing code they know is broken to develop.
The problem is that it is all to easy to look at a PR with some failures
and *assume* your code didn't cause the failures and merge it anyway. I
think we should all be at least rerunning the tests and not merging the PR
until everything passes. If the merge button is greyed out, that might help
communicate that standard to everyone.
Looking at the OpenJDK 8 metrics, it looks to me like most of the issues
are recently introduced (builds 81-84 and the EvictionDUnitTest), not old
flaky tests. So I think we were a little more disciplined always listening
to what the checks are telling us we would have less noise in the long run.
https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-metrics/jobs/GeodeDistributedTestOpenJDK8Metrics/builds/23
-Dan
On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer <u...@apache.org> wrote:
0
Patrick does make a point. The committers on the project have been voted
in as "responsible, trustworthy and best of breed" and rights and
privileges according to those beliefs have been bestowed.
We should live those words and trust our committers. In the end.. If
there is a "rotten" apple in the mix this should be addressed, maybe not
as public flogging, but with stern communications.
On the other side, I've also seen the model where the submitter of PR is
not allowed to merge + commit their own PR's. That befalls to another
committer to complete this task, avoiding the whole, "I'll just quickly
commit without due diligence".
--Udo
On 11/12/18 10:23, Patrick Rhomberg wrote:
-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
