I'd still like to see the PR rerun tool or an equivalent available to
everyone. Sure you can push an empty commit but that reruns everything,
but the PR tool lets you rerun only the checks that failed.
On 10/21/19 3:04 PM, Ernest Burghardt wrote:
+1
@Naba thanks for seeing this through!
On Mon, Oct 21, 2019 at 1:51 PM Nabarun Nag <n...@apache.org> wrote:
Thank you all for all the valuable feedback. Robert was kind enough to
check the technical feasibility of the integration of Github Branch
Protection Rules and its compatibility with our concourse CI checks and we
are satisfied with the settings provided and the initial tests that Robert
did with a demo geode branch. Please find attached the screenshot of the
settings that will be sent to INFRA for enabling it on the Apache Geode
repository.
Regards
Nabarun
On Mon, Oct 21, 2019 at 12:21 PM Aaron Lindsey <alind...@pivotal.io>
wrote:
+1
- Aaron
On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <u...@apache.com> wrote:
BIG +1 (Yes, I'm changing my -1)
@Naba, thank you for the offline chat. It seems that the proposal of
Github enforcing our good practices is a great option.
2 merged PR's without a full green pipeline, since 18 Oct 2019, shocking
and disgusting!!
I would just like to reiterate to ALL committers, you have a
responsibility towards the project to commit/merge only the best code
possible. If things break, fix them. Don't merge and hope that it goes
away and someone else fixes it.
I really don't want to support a mechanism where the project has become
so prescriptive and restrictive, all because we have a few committers
who will not follow the established and agreed processes. BUT, once
again, the masses are impacted due to a few.
Thank you Naba for following this through.
--Udo
On 10/21/19 11:05 AM, Nabarun Nag wrote:
@Karen
Thank you so much for the feedback. I understand that committers must
trust
each other and I agree entirely with that. This proposal is just
preventing
us from making mistakes. Its just guard rails. As you can see in the
email
chain that this mistake was made multiple times and this has let to a
lot
of engineers give up their time and detecting and fixing this issue.
We
still trust our committers, we are just enabling some features to
avoid
time being wasted on avoidable mistakes and use that time in
improving Geode. I hope I can persuade you to change your opinion.
@Owen
Thank you for accepting the baby step proposal. Yes, let's see in some
time
if this is successful. We can always undo this if needed.
@Rest
- Blaming people etc. will be very detrimental to the community, I do
not
propose that.
- This proposal was not just my idea but from feedback from lot of
developers who contribute to Geode on a frequent basis.
- It is very* unfair *to the engineers to go and investigate and find
out
what caused the failure and then send emails etc, their time can be
used
in
doing something more valuable.
- This is not something new, we have had this issue for over 3 years
now,
and maintaining this as the status quo is not healthy. Let us try this
solution, the committers, and developers who contribute on a regular
basis
will immediately see if it works or does not work and can revert it if
this
does not.
- There is a problem and this is an attempt at the solution. If it
does
not
work, we can revert it. For the sake of all the developers who are in
pain
and helping them spend the time on more productive tasks, let us just
give
this proposal a chance. If there are any issues we can revert it.
*Reiterating the proposal:*
Github branch protection rule for :
- at least one review
- Passing build, unit and stress test.
In our opinion, no committer would want to check-in code with failing
any
of the above.
Regards
Nabarun
On Mon, Oct 21, 2019 at 10:28 AM Alexander Murmann <
amurm...@apache.org>
wrote:
Udo, I think you are making a great point! I am fully bought into
trusting
our committers to know how many reviews are needed for their PRs. We
should
be able to have the same trust into knowing when it's OK to merge.
If a
committer wants to they can after all, always merge and push without
a
PR.
That's because we trust them.
If we are seeing that our committers merge without sufficient review
(via
human or automated means), is this an education problem we can solve?
On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <u...@apache.com>
wrote:
I must agree with @Karen here..
All committers are trusted to do the right thing. One of those
things
is
to commit (or not commit) PR's.
Now we are discussing disabling the button when tests are failing.
Why
stop there? Why not, that the submitter of the said commit does not
get
to merge their own PR?
Now, that of course is taking it to the extreme, that we don't want
(at
least I don't want to be THAT over prescriptive).. So why do we
want to
now limit when I can merge? It remains the committers
responsibility to
merge commits into the project, that are of the expected quality.
IF it
so happens that one, by accident, has merged a PR before it was
green,
revert it. All committers have the power to do so.
So from my perspective, a -1 on disabling the Merge button, just
because
someone was not careful in merging and without following our
protocol
of
waiting for an "All green".
--Udo
On 10/21/19 10:11 AM, Ernest Burghardt wrote:
+1 to enacting this immediately... just this weekend a PR was
merged
with
failures on all of the following:
* concourse-ci/DistributedTestOpenJDK11 * Concourse CI build
failure
* concourse-ci/UnitTestOpenJDK11 * Concourse CI build failure
* concourse-ci/UnitTestOpenJDK8 * Concourse CI build failure
* concourse-ci/UpgradeTestOpenJDK11 * Concourse CI build failure
Thanks,
EB
On Mon, Oct 21, 2019 at 10:01 AM Karen Miller <kmil...@pivotal.io>
wrote:
I have (more than once) committed docs changes for typo fixes
without
review. I generally label the commits
with a bold "Commit then Review" message. But, I am bringing
this up
as I
have purposely not followed what
looks to be a positively-received proposed policy, since I have
not
gotten
reviews. If all feel that we need a
rule for everyone to follow (instead of a guideline that PRs shall
have
at
least 1 review), I will follow the rule,
but I'm a -0 on the process. I get it, and I understand its
purpose
and
intent, but I personally prefer to trust that each
comitter takes personal responsibility for the code they commit
WRT
waiting
for tests and/or obtaining reviews.
Karen
On Mon, Oct 21, 2019 at 6:24 AM Joris Melchior <
jmelch...@pivotal.io
wrote:
+1 to the revised approach. I think requiring at least one
review is
important. More eyes make for better code.
Cheers, Joris.
On Mon, Oct 21, 2019 at 8:11 AM Ju@N <jujora...@gmail.com>
wrote:
+10 to Naba's proposal, it seems the right thing to do and will
help
us
to
prevent accidentally breaking *develop* while keeping focus on
people
instead of processes.
I'd add, however, that the *Merge Pull Request* button should
remain
disabled until *all CIs have finished*, and only enable it once
the
*Build,
Unit, Stress Tests and LGTM are green *(that is, force the
committer
to
wait at least until all CIs are done)*. *I also agree in that
that
we
should require *at least one* official approval.
Cheers.
--
*Joris Melchior *
CF Engineering
Pivotal Toronto
416 877 5427
“Programs must be written for people to read, and only
incidentally
for
machines to execute.” – *Hal Abelson*
<https://en.wikipedia.org/wiki/Hal_Abelson>
