All committers have to right to commit and all committers have the right
to revert.
It is ALL for the good of the project. Now, of course, I imagine we
won't apply scorched earth policies, but every committers should feel
empowered to be able to revert a bad merge, if it hindering the progress
and health of the project.
Of course, contacting the original committer to revert the merge, is
preferable, but sometimes things take too long and a kindly worded email
to the committer, letting them know the merge was reverted and for what
reason is all that is really required in the end.
--Udo
On 10/21/19 10:41 AM, Owen Nichols wrote:
One perspective to consider is, should anyone be able to revert a bad commit,
or only the original committer? If we feel individual ownership of commits, we
might hesitate to revert someone else’s commit, even if it broke the build.
However if we feel a strong sense of community, we might be ok with an
anyone-can-revert policy. A revert does not have to imply shaming...
On Oct 21, 2019, at 10:30 AM, Robert Houghton <rhough...@pivotal.io> wrote:
@Udo Kohlmeyer <ukohlme...@pivotal.io> What, if anything, do you propose we
do to help keep our project building and running cleanly that does not
force punitive or coercive behavior on our developers? "Naming names" or
"shaming" are not popular choices, and everyone on the comitters list
*should* follow procedure, but doesn't.
What would you do?
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>
