Open Stack

> On 17 Oct 2015, at 00:26, Armando M. <armamig at gmail.com> wrote:
> 
> I am pretty confident that the experience you went through was an exception rather than the rule. We're pretty flexible and allow a patch to go in (master) with a promise of a follow up, if it's critical backport. The important thing is to identify that...and this is what Ihar is talking about in this thread.
> 
> It is noteworthy that 'trail blazing' as you say makes the patch difficult to backport, so any advice given in this direction was clearly pointing you the wrong way.
> 

If we talk about nitpicking for backports, I am not sure it’s indeed anything but an exception and a reviewer mistake that should be pointed out and ignored instead of being handled. (I seldom need to request a revert to the original ‘bad’ version of a patch when I see such ‘enhancements' before I allow a patch in.)

It’s herd wisdom in openstack that backports must not differ from their original master counterparts, unless there is actual need for it (like gate failure, test framework differences, etc.) Apart from it, any nitpicking about tests, typos, import order, incorrect copyright year, etc. is out of place in stable branches. If you get a suggestion to ‘enhance’ your backport in some way that is of no user visible influence, kindly suggest the reviewer to fix it in master and backport to stable branches, if they care that much. It’s not your job to fix it. The only exception is if by chance backport review reveals an actual bug in master code that would introduce a regression in stable branches, if backported. In that case, the backport should be blocked until master is fixed and the second backport is proposed; in that case, both backports should be up for review and ready to merge before the first one is pushed into gate (that’s to ensure we reduce chance of breaking someone who catches up with the branch in the middle).

I thought it’s documented and/or self-obvious, but since probably it’s not, I updated the stable branch guidelines as per above:

https://wiki.openstack.org/wiki/StableBranch#Proposing_Fixes

Now, that was only for backports. But as for master, I agree with other folks that expressed the need for decent test coverage before a patch is considered for master merge, even for major issues. It’s better to spend some time on polishing tests and enhancing test coverage (and becoming assured that the fix is correct and does not break anything), then to push a patch in and hope for the best, waiting until our users hit a regression that is caused by insufficient test coverage.

Now, that does not mean that we should postpone critical patches due to incomplete test framework that would require months to get into shape; that said, for non-critical patches, it may be reasonable to push committers a bit harder to get to the point when we have testing framework deficiencies closed, and where we feel more safe to touch the code that (apart from the bug in question) seems to work.

Again, if we plan to backport a fix into stable branches, it’s ok to have enhanced testing coverage made as a follow-up, when otherwise we would have issues with backporting the tests to stable branches. Actually, thanks to Armando’s polishing our policies, we already have that caution documented:

http://docs.openstack.org/developer/neutron/devref/effective_neutron.html#scoping-your-patch-appropriately

Quoting: "If a fix or feature requires code refactoring, submit the refactoring as a separate patch than the one that changes the logic. Otherwise it’s difficult for a reviewer to tell the difference between mistakes in the refactor and changes required for the fix/feature. If it’s a bug fix, try to implement the fix before the refactor to avoid making cherry-picks to stable branches difficult.”

Feel free to use the quote when pushing for a high priority bug fix in master. (Note that while it allows a follow up, it does not explicitly suggest that follow up can be up for review after the actual bug fix is merged; but the logic and common sense suggest that in some critical cases it can be the case, assuming there is at least some test coverage for the code touched, even if not ideal.)

Ihar
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20151019/ae7917c1/attachment.pgp>