openstack
/
contributor-guide
Code Issues Proposed changes

Merge "Add best practices on structuring changes and handle patches"

This commit is contained in:
Zuul 2018-04-19 16:25:40 +00:00 committed by Gerrit Code Review
parent 30f44e912a c1a59b0b90
commit 3cfa44be23
2 changed files with 181 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
doc/source/code-and-documentation/index.rst
View File

@ -15,3 +15,4 @@ Code & Documentation Contributor Guide
/common/task-tracking
/common/i18n
/common/governance
/common/patch-best-practices

180
doc/source/common/patch-best-practices.rst Normal file
View File

@ -0,0 +1,180 @@
###########################
How to Become a Patch Guru?
###########################
When you are working on implementing a new feature or adding documentation to
an already existing one it is easy to get carried away by the work itself and
forget about the unwritten rules of constructing your changes.
This section will guide you through how to create patches that people will want
to review.
The Right Size
==============
Reviewing large patches is very inconvenient and time consuming therefore we
always suggest to break down your changes into smaller blocks.
While there is no magic number try to **keep the size of your changes as small
as possible**, but under a few hunderds of line changes total. Patches that are
test heavy with little code change require as much effort as code heavy
changes.
In rare occasions when there is no good logical breakdown for a change and your
patch can grow to a thousand lines or more. In some cases it is acceptable as
you cannot extract the related test changes to another patch for instance, but
it's highly not highly recommended.
Always try to extract as much as you can into other patches, like documentation
or logical parts of the functionality that do not depend on common functions
in a lower layer.
Longer patches require more time to review; wherever you can, keep the length
reasonable. And where you can't, you can help the reviewers by adding code
comments and writing a detailed commit message to describe the changes you
introduced in your patch.
Patch Chains, Depends-On Tag and Gerrit Topics
==============================================
In the case of complex feature implementation work when you need to introduce
changes to multiple modules of the same project or multiple projects you need
to be very careful with managing the dependencies.
There are multiple options to choose from depending on the structure of your
design. You can either organize the changes in patch chains or you can use the
'Depends-On' tag.
Depends-On Tag
--------------
When you have changes in multiple project repositories you can mark dependent
patches with the 'Depends-On' tag. The tag will appear as a link in the
commit message which helps you and also the reviewers to track and navigate
between the dependencies of your changes.
The 'Depends-On' tag is only a marker on your changes and in case you apply it
a patch cannot be merged until all its dependencies got landed.
The tag can be applied to patches proposed for the same repository as well. In
that case the changes are separate enough to be kept indenependent which means
that if you need to fix changes from review comments you can do it on a per
patch basis. It is also true for rebasing each patch.
.. note::
In case you use the 'Depends-On' tag you need to download all the changes
for a feature implementation or documentation work to test the feature or
build the documentation with all the changes applied. Git will not take care
of handling the dependencies automatically in this case.
Patch Chains
------------
In some cases when you break down the required changes to smaller blocks you
cannot avoid having direct dependencies between them that prevents you from
having independent changes. You need to organize your changes in a chain to
maintain the dependencies which requires some additional care when you work
with these changes.
Even if you have a chain of patches you still need to keep code changes and
related tests in one patch as you cannot guarantee that both land in time for a
release.
When you have a chain Gerrit helps you by listing all the commit titles in the
'Related Changes' column on the upright corner of the Gerrit UI. The titles are
also links which help you navigate between the changes to review them when you
upload a new version.
How to Handle Chains?
---------------------
A patch chain is easy to handle if you keep in mind a few recommendations:
* Always have a local branch for these changes to ensure that you don't mix it
together with changes related to another feature or bug fix.
* Always handle a chain as one block of changes by rebasing the whole chain and
keep it up to date when you modify a patch to fix review comments or add
changes to it.
* To modify a patch within a chain you will need to use interactive rebase:
.. code-block:: console
git rebase -i HEAD^
You need as many '^' as the number of the patch you want to edit first from the
top of the chain. Gerrit also provides you options to edit the patch itself or
only the commit message and a few more for more advanced changes, like
modifying the author.
* To download the full chain you need to download the top patch and Git will
automatically dowloand all the dependent patches in the chain.
* If you only need to modify the top patch in the chain that can be done the
same way as you update individual patches.
* When you upload changes in a chain only the patches that got updated will be
uploaded. This also means that the review scores on lower patches in the
chain will be untouched.
* Always check the changes you made to each patch and be careful that you
applied the changes in the right one as patches still get merged individually
and there is no guarantee that the whole chain gets landed at the same time.
Gerrit Topics
-------------
You have the option to specify a topic for your changes when you upload them
for review. While Gerrit topics will not add dependency to your patches you can
apply a search based on the topic that will show you all the relevant changes
in all the projects where there are patches with the same topic. Gerrit will
also show them to you in the 'Same Topic' column on the upright corner of the
page of a review.
This is a good way to help tracking related changes let that be a feature
implementation, bugfix or documentation work.
How to Structure Your Changes?
==============================
When your work item grows above a particular size and you need to upload
multiple patches it is crucial to structure it well in case of both patch
chains and independent changes.
It is a good practice to group changes by modules in a project, for instance
virt driver changes, compute manager and api changes in case of OpenStack
Compute.
By grouping the changes per module you can also construct the chain or
dependencies by the hierarchy of the components and always keep the API changes
last as that will ebable the new functionality and that change will depend on
everything else you needed to touch for your design.
Beyond this you can also look into the functionality to find smaller building
blocks and make your changes smaller. For instance changes to an object can be
implemented first that you will use later when you implement new API
functionality.
The Right Content
=================
Changes that are not related to any feature implementation or bug report can be
uploaded but are less welcomed by reviewers.
Improvement to either the code or documentation should be part of a larger
effort, like if you would like to fix typos in documentation then you should
do it for a larger block, like a whole guide. It is also preferred to report a
story with tasks for a work item like this that can be tracked later.
It is similar for code improvements. As the community is large and world-wide
we have coding guidelines, but the style of each individuals can still be very
different. We don't enforce a particular coding style, therefore changes
related to fix that are less welcomed and are sources of very opinionated
arguments that should be avoided.
In case of code refactoring work which makes the code more readable and easier
to maintain by restructuring methods and deleting unused code snippets it is
highly encouraged to consult with the project team and report a story in
StoryBoard first and then upload the relevant changes to Gerrit for review.