diff --git a/doc/source/code-and-documentation/index.rst b/doc/source/code-and-documentation/index.rst index 6bcc3cd..b291b14 100644 --- a/doc/source/code-and-documentation/index.rst +++ b/doc/source/code-and-documentation/index.rst @@ -15,3 +15,4 @@ Code & Documentation Contributor Guide /common/task-tracking /common/i18n /common/governance + /common/patch-best-practices diff --git a/doc/source/common/patch-best-practices.rst b/doc/source/common/patch-best-practices.rst new file mode 100644 index 0000000..94c129f --- /dev/null +++ b/doc/source/common/patch-best-practices.rst @@ -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.