openstack/cinder
Code Issues Proposed changes

Add review best practices section

Browse Source 
This doc change aims at creating a checklist of points which would
help reviewers provide useful feedback while reviewing patches.

Change-Id: I1af1ea6b1dab2bc2e5420076ffe7977f11a4aa75
This commit is contained in:
whoami-rajat 2022-03-21 15:56:10 +05:30
parent d2b9831d15
commit cf6e53b2ee
1 changed files with 121 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

121
doc/source/contributor/gerrit.rst
View File

@ -177,9 +177,130 @@ failures that are going to take a while to fix, and issuing a recheck will be
futile at that time and we should wait for a couple of hours, or maybe even a
day, before issuing a recheck that can yield the desired result.
Efficient Review Guidelines
---------------------------
This section will guide you through the best practices you can follow to do
quality code reviews:
* **Failing Gate**: You can check for jobs like pep8, py36, py38, functional
etc that are generic to all the patches and look for possible failures in
linting, unit test, functional test etc and provide feedback on fixing it.
Usually it's the author's responsibility to do a local run of tox and ensure
they don't fail upstream but if something is failing on gate and the author
is not be aware about how to fix it then we can provide valuable guidance on
it. There are also jobs specific to particular area of code (for example,
``cinder-plugin-ceph-tempest`` for the RBD volume driver,
``devstack-plugin-nfs-tempest-full`` for the generic NFS driver etc) so look
for issues in the jobs if they are related to the code changes proposed.
There is a past example on why we should check these jobs, the
``devstack-plugin-nfs-tempest-full`` is a non-voting job and was failing on
one of the FS drivers related `patch`_ which got merged and started failing
the ``NetApp CI`` blocking the netapp features during that time.
* **Documentation**: Check whether the patch proposed requires documentation
or not and ensure the proper documentation is added. If the proper
documentation is added then the next step is to check the status of docs job
if it's failing or passing. If it passes, you can check how it looks in HTML
as follows:
Go to ``openstack-tox-docs job`` link -> ``View Log`` -> ``docs`` and go to
the appropriate section for which the documentation is added.
Rendering: We do have a job for checking failures related to document
changes proposed (openstack-tox-docs) but we need to be aware that even if
a document change passes all the syntactical rules, it still might not be
logically correct i.e. after rendering it could be possible that the bullet
points are not under the desired section or the spacing and indentation is
not as desired. It is always good to check the final document after rendering
in the docs job which might yield possible logical errors.
* **Readability**: In a large codebase (like Cinder), Readability is a big
factor as remembering the logic of every code path is not feasible and
contributors change from time to time. We should adapt to writing readable
code which is easy to follow and can be understood by anyone having
knowledge about Python constructs and working of Cinder. Sometimes it
happens that a logic can only be written in a complex way, in that case,
it's always good practice to add a comment describing the functionality.
So, if a logic proposed is not readable, do ask/suggest a more readable
version of it and if that's not feasible then asking for a comment that
would explain it is also a valid review point.
* **Type Annotations**: There has been an ongoing effort to implement type
annotations all across Cinder with the help of mypy tooling. Certain areas
of code already adapt to mypy coding style and it's good practice that new
code merging into Cinder should also adapt to it. We, as reviewers, should
ensure that new code proposed should include mypy constructs.
* **Microversions**: Cinder uses the microversion framework for implementing
new feature that causes a change in the API behavior (request/response)
while maintaining backward compatibility at the same time. There have been
examples in the past where a patch adding a new microversion misses file(s)
where the microversion changes are necessary so it's a good practice for the
author and reviewer to ensure that all files associated with a microversion
change should be updated. You can find the list of files and changes
required in our `Microversion Doc`_.
* **Downvoting reason**: It often happens that the reviewer adds a bunch of
comments some of which they would like to be addressed (blocking) and some
of them are good to have but not a hard requirement (non-blocking). It's a
good practice for the reviewer to mention for which comments is the -1 valid
so to make sure they are always addressed.
* **Testing**: Always check if the patch adds the associated unit, functional
and tempest tests depending on the change.
* **Commit Message**: There are few things that we should make sure the commit
message includes:
1) Make sure the author clearly explains in the commit message why the
code changes are necessary and how exactly the code changes fix the
issue.
2) It should have the appropriate tags (Eg: Closes-Bug, Related-Bug,
Blueprint, Depends-On etc). For detailed information refer to
`external references in commit message`_.
3) It should follow the guidelines of commit message length i.e.
50 characters for the summary line and 72 characters for the description.
More information can be found at `Summary of Git commit message structure`_.
4) Sometimes it happens that the author updates the code but forgets to
update the commit message leaving the commit describing the old changes.
Verify that the commit message is updated as per code changes.
* **Release Notes**: There are different cases where a releasenote is required
like fixing a bug, adding a feature, changing areas affecting upgrade etc.
You can refer to the `Release notes`_ section in our contributor docs for
more information.
* **Ways of reviewing**: There are various ways you can go about reviewing a
patch, following are some of the standard ways you can follow to provide
valuable feedback on the patch:
1) Testing it in local environment: The easiest way to check the correctness
of a code change proposed is to reproduce the issue (steps should be in
launchpad bug) and try the same steps after applying the patch to your
environment and see if the provided code changes fix the issue.
You can also go a little further to think of possible corner cases where an
end user might possibly face issues again and provide the same feedback to
cover those cases in the original change proposed.
2) Optimization: If you're not aware about the code path the patch is fixing,
you can still go ahead and provide valuable feedback about the python code
if that can be optimized to improve maintainability or performance.
3) Perform Dry Run: Sometimes the code changes are on code paths that we
don't have or can't create environment for (like vendor driver changes or
optional service changes like cinder-backup) so we can read through the code
or use some example values to perform a dry run of the code and see if it
fails in that scenario.
.. _Review guidelines: https://docs.openstack.org/doc-contrib-guide/docs-review-guidelines.html
.. _Gerrit: https://review.opendev.org/#/q/project:openstack/cinder+status:open
.. _Quick Reference: https://docs.openstack.org/infra/manual/developers.html#quick-reference
.. _Getting Started: https://docs.openstack.org/infra/manual/developers.html#getting-started
.. _Development Workflow: https://docs.openstack.org/infra/manual/developers.html#development-workflow
.. _patch: https://review.opendev.org/c/openstack/cinder/+/761152
.. _Microversion Doc: https://opendev.org/openstack/cinder/src/branch/master/doc/source/contributor/api_microversion_dev.rst#other-necessary-changes
.. _external references in commit message: https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references
.. _Summary of Git commit message structure: https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure
.. _Release notes: https://docs.openstack.org/cinder/latest/contributor/releasenotes.html