openstack team mailing list archive

[Dan Prince <dprince@xxxxxxxxxx>]

----- Original Message -----
> From: "Russell Bryant" <rbryant@xxxxxxxxxx>
> To: andrewbogott@xxxxxxxxx
> Cc: "Andrew Bogott" <abogott@xxxxxxxxxxxxx>, openstack@xxxxxxxxxxxxxxxxxxx
> Sent: Monday, July 2, 2012 3:26:56 PM
> Subject: Re: [Openstack] best practices for merging common into specific	projects
> 
> On 07/02/2012 03:16 PM, Andrew Bogott wrote:
> > Background:
> > 
> >     The openstack-common project is subject to a standard
> >     code-review
> > process (and, soon, will also have Jenkins testing gates.)  Sadly,
> > patches that are merged into openstack-common are essentially
> > orphans.
> > Bringing those changes into actual use requires yet another step, a
> > 'merge from common' patch where the code changes in common are
> > copied
> > into a specific project (e.g. nova.)
> >     Merge-from-common patches are generated via an automated
> >     process.
> > Specific projects express dependencies on specific common
> > components via
> > a config file, e.g. 'nova/openstack-common.conf'.  The actual file
> > copy
> > is performed by 'openstack-common/update.py,' and its behavior is
> > governed by the appropriate openstack-common.conf file.
> 
> More background:
> 
> http://wiki.openstack.org/CommonLibrary
> 
> > Questions:
> > 
> >     When should changes from common be merged into other projects?
> >     What should a 'merge-from-common' patch look like?
> >     What code-review standards should core committers observe when
> > reviewing merge-from-common patches?
> > 
> > Proposals:
> > 
> > I.      As soon as a patch drops into common, the patch author
> > should
> > submit merge-from-common patches to all affected projects.
> >     A.  (This should really be done by a bot, but that's not going
> >     to
> > happen overnight)
> 
> All of the APIs in openstack-common right now are considered to be in
> incubation, meaning that breaking changes could be made.  I don't
> think
> automated merges are good for anything in incubation.
> 
> Automation would be suitable for stable APIs.  Once an API is no
> longer
> in incubation, we should be looking at how to make releases and treat
> it
> as a proper library.  The copy/paste madness should be limited to
> APIs
> still in incubation.
> 
> There are multiple APIs close or at the point where I think we should
> be
> able to commit to them.  I'll leave the specifics for a separate
> discussion, but I think moving on this front is key to reducing the
> pain
> we are seeing with code copying.
> 
> > II.     In the event that I. is not observed, merge-from-common
> > patches
> > will contain bits from multiple precursor patches.  That is not
> > only OK,
> > but encouraged.
> >     A.  Keeping projects in sync with common is important!
> >     B.  Asking producers of merge-from-common patches to understand
> >     the
> > full diff will discourage the generation of such merges.
> 
> I don't see this as much different as any other patches to nova (or
> whatever project is using common).  It should be a proper patch
> series.
>  If the person pulling it in doesn't understand the merge well enough
>  to
> produce the patch series with proper commit messages, then they are
> the
> wrong person to be doing the merge in the first place.

I went on a bit of a rant about this on IRC yesterday. While I agree a patch series is appropriate for many new features and bug fixes I don't think it should be required for keeping openstack-common in sync. Especially since we don't merge tests from openstack-common which would help verify that the person doing the merges doesn't mess up the order of the patchset. If we were to include the tests from openstack-common in each project this could change my mind.

If someone wants to split openstack-common changes into patchsets that might be Okay in small numbers. If you are merging say 5-10 changes from openstack common into all the various openstack projects that could translate into a rather large number of reviews (25+) for things that have been already reviewed once.  For me using patchsets to keep openstack-common in sync just causes thrashing of Jenkins, SmokeStack, etc. for things that have already been gated. Seems like an awful waste of review/CI time. In my opinion patchsets are the way to go with getting things into openstack-common... but not when syncing to projects.

Hopefully this situation is short lived however and we start using a proper library sooner rather than later.


> 
> > III.    Merge-from-common patches should be the product of a single
> > unedited run of update.py.
> 
> Disagree, see above.
> 
> >     A.  If a merge-from-common patch breaks pep8 or a test in nova,
> > don't fix the patch; fix the code in common.
> 
> Agreed.
> 
> > IV.    Merge-from-common patches are 'presumed justified'.  That
> > means:
> >     A. Reviewers of merge-from-common patches should consider test
> > failures and pep8 breakages, and obvious functional problems.
> >     B. Reviewers of merge-from-common patches should not consider
> >     the
> > usefulness, design, etc. of merge-from-common patches.  Such
> > concerns
> > need to be handled within the common review process.
> >     C. Merges from common should get reviewed early and approved
> >     readily
> > in order to avoid project divergence
> 
> I think core reviewers of projects using openstack-common are
> justified
> in doing critical review of new code being used by their project.  A
> core reviewer of nova for examp[le may have a very good reason why
> the
> code is not suitable for consumption in nova that openstack-common
> reviewers didn't think of.
> 
> I don't think blind approvals are ever a good idea.
> 
> > V.     Changes to openstack-common.conf are a special case.
> >     A. Patches with openstack-common.conf changes should include
> >     the
> > relevant merge-from-common changes.
> >     B. Such patches should /not/ include any other
> >     merge-from-common
> > changes.
> >     C. Such patches should not only include the common merges, but
> > should also include whatever code changes make use of the new
> > merge.
> >     D. Such patches require the same justification and scrutiny as
> >     any
> > other standard project patch.
> 
> This all seems fine to me.
> 
> > Please discuss!  If we're able to reach general agreement about
> > this
> > process, I will document the process in the openstack-common
> > HACKING
> > guidelines.
> 
> 
> --
> Russell Bryant
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to     : openstack@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openstack
> More help   : https://help.launchpad.net/ListHelp
>