On 21/07/15 04:21, Dmitry Borodaenko wrote:
I -2 for this reason: this patch already had a +2 so it was closed to be
merged by someone else, while the patch was *breaking backward
compatiblity* in puppet-horizon, because Vasyl was changing the default
cache driver to force users to use Memcached by default.
We *never* break backward compatibility (this is a general rule in
OpenStack) and *never* change OpenStack default configuration (general
rule in Configuration management), but rather provide the interface
(called Puppet parameters) to change it.
That's fair enough, and on July 8 we pushed patch set 10 that has fixed
this problem, and set the default to LocMemCache. AFACT at this point
you've lost track of this commit because your gerrit dashboard excludes
all commits that have a -2 vote on them, so it got stuck in limbo for
two weeks.
I still disagree with your use of -2 vote here, a -1 from a core
reviewer should be enough to prevent other cores from merging a patch
set, and should tell them what to watch out for in subsequent patch sets.
This is actually a wider problem across OpenStack - core reviewers in
different projects have different ideas about what -2 means.
For me, it's a signal to the submitter that there is no way the patch
could ever be merged in its current form and that they should not bother
to invest any further effort in it unless there is a radical change in
approach. That's certainly the way it is used in heat-core. (There's
also a procedural version, where it's just that the patch certainly
won't be merged before an imminent feature freeze, but the -2 will be
removed after that.) It's also the case that most tooling is set up for
(e.g. -2'd patches drop off everyone's review radar).
At the other end of the spectrum, I've heard that core reviewers in some
projects (*cough*nova*cough*) will routinely -2 a patch without having
even read it just to be sure that it cannot get merged without their
personal approval. Propriety prevents me from sharing what I think of
this practice here.
The case in question lies somewhere in the middle, but IMHO a -1 would
have been perfectly adequate, and I find it extremely improbable that
another core reviewer would have merged the patch over that objection.
If you're really worried though, the appropriate thing is to set the
Approved- (WIP) flag. This sends a clear signal that the patch is not
ready to merge (this may even be enforced by Gerrit, not 100% sure), but
is also not sticky so it will drop off on the next patchset. This avoids
all of the problems with the patch going into review purgatory and being
blocked by the reviewer being on vacation.
cheers,
Zane.
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
