From 8c7fdb588e93a8960742c60b4a9f0864abab95d7 Mon Sep 17 00:00:00 2001 From: KATO Tomoyuki Date: Thu, 7 Jul 2016 10:27:01 +0900 Subject: [PATCH] [contributor] reorganize reviewing documentation chapter Also, migrated the review guidelines from https://wiki.openstack.org/wiki/Documentation/ReviewGuidelines Change-Id: I1d6d97b0826869834e4252cb2458f5bd03233c73 Implements: blueprint contributor-guide-reorg --- .../source/docs-review-guidelines.rst | 209 +++++++++++++++ doc/contributor-guide/source/docs-review.rst | 240 ++++++++++-------- 2 files changed, 338 insertions(+), 111 deletions(-) create mode 100644 doc/contributor-guide/source/docs-review-guidelines.rst diff --git a/doc/contributor-guide/source/docs-review-guidelines.rst b/doc/contributor-guide/source/docs-review-guidelines.rst new file mode 100644 index 0000000000..594aa87095 --- /dev/null +++ b/doc/contributor-guide/source/docs-review-guidelines.rst @@ -0,0 +1,209 @@ +================= +Review guidelines +================= + +This section provides guidelines to improve the quality and speed of +the documentation review process. + +Critique categories +~~~~~~~~~~~~~~~~~~~ + +Below is a list of the categories you will critique as the reviewer. + +Objective +--------- + +* `Commit message `_ + + * Content + * Conventions + * Bug number or blueprint + +* Patch + + * Content + * Grammar + * :doc:`Style, phrasing, wording, and capitalization ` + * Spelling + * :doc:`RST formatting ` + * Accuracy of the selected location + +Subjective +---------- + +* Commit message + + * Grammar + * Spelling + * Style, phrasing, and wording + +* Patch + + * Grammar + * Style, phrasing, and wording + * Technical accuracy + * Other suggestions + +Scope +~~~~~ + +Try to keep reviews limited to the contents of the bug, contents of +the commit message, and changes made by the patch. +In other words, if the patch solves the stated problem, but there are +other improvements that could result from the patch, approve the patch +and file a subsequent bug, rather than -1'ing the patch with a comment +about improvements. This is why it is a good idea to write commit +messages that clearly define the scope of the patch. + +Additional comments within the objective and subjective aims of a patch, +that would result in a -1, are appropriate. +Remember to consider if the change is related to the scope. + +Consistency +~~~~~~~~~~~ + +* Mark all instances of an issue if one appears in a patch. + + * If the author uploads a patch correcting your objective issue and + you find another instance that you didn't mark, comment on it and + score with a -1. Preferably, upload a patch to fix it. + + * If the author uploads a patch correcting your subjective issue and + you find another instance that you didn't mark, comment on it and + score with a 0. + + * If the author uploads a patch correcting your objective and/or + subjective issue and you find another objective issue, comment on + it and score with a -1. Preferably, upload a patch to fix it. + + * If the author uploads a patch correcting your objective and/or + subjective issue and you find another subjective issue, comment on + it and score with a 0. + +* If an issue appears that could affect other portions of a book, + provide appropriate comments, score the patch with a -1, and consider + mentioning your issue on the mailing list or in a meeting. + + Example: A new service uses "key = value" in the configuration file + and all other services use "key=value" in their configuration files. + Both methods work, but the book should maintain consistency. + +* If a patch has already received a -1, do not be discouraged from + checking on it, and add additional comments. This way, there will + be fewer patch sets and comments building up under one review. + +Tagging additional reviewers +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In some cases, you should tag one or more people with interest in or +experience with the content of your patch to review it. + +Q: How long should an author wait for reviews by these people? + +A: For extremely busy projects with backlogs of over 400 patches, +wait two weeks at least. If you have difficulty getting a reviewer +for a particular project, use the `Cross Project Documentation Liaisons +`_ +page to contact the documentation liaison for the project to get a reviewer. +The Documentation PTL can also assist in getting reviewers attention. + +The waiting game +~~~~~~~~~~~~~~~~ + +After the first review with a -1 or 0 score, the author should update +the patch. Authors do not need to wait for a lengthy period of time. +Expect to leave some time for reviewers to check on a patch, however. +Consider that some reviewers are located in different timezones. + +Core reviewers will in general review a patch within a few days. +If no review happens, feel free to ask on the #openstack-doc IRC channel. + +Review scoring and approvals +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +* Scores available to contributors: -1, 0, +1 +* Scores available to core reviewers: -2, -1, 0, +1, +2 +* Approvals + +A core reviewer can +2 score a patch with a +2 score from another core +reviewer and approve it. +If the change contains important or critical information, +the core reviewer should not approve it immediately, but provide a few days +for wider community audience to express their opinion, and suggest posting +to the documentation mailing list. + +.. note:: + + If you find an issue with a patch that already has a +2 score from + another core reviewer, consider commenting on the issue and scoring the + patch with a 0 rather than scoring it with a -1. + +Setting WorkInProgress (WIP) during review +------------------------------------------ + +The WIP tag tells potential reviewers to expect additional updates to +a patch before reviewing. Both the change's owner and any core reviewer +can set the WIP status: + +* A change owner and core reviewers can set this tag on their own review + to mark that additional changes are still being made, and to avoid + unnecessary reviews while that happens. + +This can be a great convenience to fellow reviewers. It allows the core +reviewer to politely send the message that the change needs additional +work while simultaneously removing it from the list of ready changes +until that happens. + +To add the WIP tag: + +#. Select a patch set. +#. Click the :guilabel:`Reply...` button. +#. Choose :guilabel:`-1 (Work In Progress)` from the :guilabel:`Workflow` + options. +#. Optional: enter comments. +#. Click :guilabel:`Post`. + +This sets a ``-1`` and informs everyone that the patch is Work In Progress. + +Abandoning patches +------------------ + +Core reviewers may abandon patches that receive a -2 review or +lack activity for at least four weeks to freshen the patch review queue. +The owner of a patch can also abandon it. +The owner and core reviewers can restore it again. + +Patches by OpenStack Proposal Bot +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +There are a few proposal jobs set up that are run regularly: + +* ``Imported Translations from Zanata``: Import of translated files from + the translation infrastructure. + This is run once a day (06:00 UTC) for each repository. +* ``Updated from openstack-manuals``: Import of glossary files from + openstack-manuals. This job is triggered whenever openstack-manuals has + merged and will propose a change if something has changed. +* ``Updated from global requirements``: Import of requirements.txt and + test-requirements.txt from the global requirements repository. + +For all types of patches, any core can approve a patch if all the tests pass. +If the tests do not pass, vote ``-1`` on the patch, fix the problem and +wait for the next proposal run. +The proposal job will update the patch with the next run. +If you cannot fix the problem, ask for help on the mailing lists: + +* openstack-i18n: translation failures +* openstack-doc: requirements failures, glossary sync failures, + and common content sync failures. + +Considerations for documentation aligned with release cycles +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Beginning with milestone releases, shift focus to objective issues, +especially with new services and existing services with significant changes. +Only patches with significant subjective issues should receive a -1 score. +Otherwise, comment on subjective issues and score with a 0. +Beginning with release candidates, focus almost entirely on content issues. +Only comment on subjective issues if the patch should receive a -1 score +for objective issues. diff --git a/doc/contributor-guide/source/docs-review.rst b/doc/contributor-guide/source/docs-review.rst index 18b8a17547..a259048e1d 100644 --- a/doc/contributor-guide/source/docs-review.rst +++ b/doc/contributor-guide/source/docs-review.rst @@ -4,16 +4,25 @@ Reviewing documentation ======================= +.. toctree:: + :maxdepth: 1 + + docs-review-guidelines.rst + +OpenStack documentation is treated like code. +We follow the standard code review process. To see what documentation changes are ready for review, use the -`Documentation Program Dashboard`_. It is organized in groupings based on -the audience for the documentation. To see current proposed changes, make -sure you register and log into https://review.openstack.org. For more -details on the review process, see `Code Review`_. +`Documentation Program Dashboard `_. +It is organized in groupings based on the audience for the documentation. +To see current proposed changes, make sure you register and +log into https://review.openstack.org. +For more details on the review process, see `Code Review +`_. Repositories and core team ~~~~~~~~~~~~~~~~~~~~~~~~~~ -The OpenStack Docs team is core for api-site, openstack-manuals, +The OpenStack Documentation team is core for api-site, openstack-manuals, openstackdocstheme, and openstack-doc-tools projects. For the following repositories that are part of the Documentation program, @@ -30,17 +39,115 @@ special rules apply: The current list of docs cores for openstack-manuals can be found at https://review.openstack.org/#/admin/groups/30,members. +Reviewing a documentation patch +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Before you proceed with reviewing patches, make sure to read carefully +the :doc:`Review Guidelines ` for documentation +and `Code Review Guidelines +`_. +Once done, follow the steps below to submit a patch review. + +#. Go to the `Documentation Program Dashboard + `_. +#. Select a patch set. +#. Click a file that was uploaded to view the changes side by side. +#. If you see some inconsistencies or have questions to the patch owner, + you can also highlight the line or word in question, and press 'c' + on your keyboard, which enables commenting directly on that line or word. + Click :guilabel:`Save` button once you write a draft of your comment. +#. In the :guilabel:`Jenkins check` section, click the Jenkins ``checkbuild`` + gate link (for the openstack-manuals, it is called + ``gate-openstack-manuals-tox-doc-publish-checkbuild``) and review the + built manuals to see how the change will look on the web page. For a new + patch, it takes some time before Jenkins checks appear on the Gerrit + page. You can also `build the patch locally`_ if necessary. +#. Click :guilabel:`Reply` to vote and enter any comments about your review, + then click :guilabel:`Post`. + +.. note:: + + A patch with WorkInProgress (WIP) status needs additional work + before review and possible approval. Therefore, you may skip such a patch + and review once it is ready. For more information, see `Work In Progress + `_. + +.. seealso:: + + `Peer Review + `_ + +.. _`build the patch locally`: + +Building an existing patch locally +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Before proceeding, make sure you have all the necessary +:ref:`tools ` installed and +set up for contribution. + +To build a patch locally: + +#. Change to the directory containing the appropriate repository. + + .. code-block:: console + + $ cd openstack-manuals + +#. Create a local branch that contains the particular patch. + + .. code-block:: console + + $ git review -d PATCH_ID + + Where the value of PATCH_ID is a Gerrit commit number. + You can find this number on the patch link, + ``https://review.openstack.org/#/c/PATCH_ID``. + +#. Build all the books that are affected by changes in the patch set: + + .. code-block:: console + + $ tox -e checkbuild + +#. Find the build result in ``publish-docs/index.html``. + +#. Review the source and the output. You can edit and update the patch: + + #. Ensure that your edits adhere to the + :ref:`Writing Style ` for OpenStack documentation + and use standard U.S. English. + #. Once the build and new output are good to commit, run: + + .. code-block:: console + + $ git commit -a --amend + + #. When the editor opens, update the commit message if necessary. But do + not add information on what your specific patch set changes. A reviewer + can use the Gerrit interface to see the difference between patches. + #. Save the changes if any, and exit the editor. + #. Send your patch to the existing review: + + .. code-block:: console + + $ git review + + #. Leave a comment in Gerrit explaining the reason for the additional + changes. + Achieving a core reviewer status ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Core reviewers are able to +2 and merge content into the projects they have -the core status in. Core status is granted to those who have not only done a -lot of reviews, but who also have shown care and wisdom in those reviews. -Becoming a core reviewer also carries with it a responsibility: you are now -the *guardian of the gate*, and it is up to the core team to ensure that -nothing untoward gets through, without discouraging contributions. The core -reviewer's role is complex, and having a great core team is crucial to the -success of any OpenStack project. +Core reviewers are able to +2 and merge content into the projects they +have the core status in. Core status is granted to **those who have not +only done a sufficient quantity of reviews, but who also have shown care +and wisdom in those reviews**. Becoming a core reviewer also carries with +it a responsibility: you are now the **guardian of the gate**, and +it is up to the core team to ensure that nothing unfavorable gets through, +without discouraging contributions. +The core reviewer's role is complex, and having a great core team is +crucial to the success of any OpenStack project. With great power comes great responsibility. @@ -58,108 +165,19 @@ nomination-based approach. This means a couple of things: The process is: -- Every month (usually on the 1st), the documentation PTL draws the top 12 +* Every month (usually on the 1st), the documentation PTL draws the top 12 names using these reports: - - http://russellbryant.net/openstack-stats/docs-reviewers-30.txt - - http://russellbryant.net/openstack-stats/docs-reviewers-90.txt - - http://stackalytics.com/?module=openstack-manuals&metric=commits + * http://russellbryant.net/openstack-stats/docs-reviewers-30.txt + * http://russellbryant.net/openstack-stats/docs-reviewers-90.txt + * http://stackalytics.com/?module=openstack-manuals&metric=commits -- The PTL then consults the existing core team with a list of names to be - removed and added from/to the core list. Once an agreement is reached, the - changes are made and advertised to the main documentation mailing list. +* The PTL then consults the existing core team with a list of names to be + removed from and added to the core list. Once an agreement is reached, + the changes are made and advertised to the main documentation mailing list. Cores who are being removed will be contacted personally before removal. -- Existing core team members can nominate a new core member at any time, + +* Existing core team members can nominate a new core member at any time, with a justification sent to the existing core team: openstack-doc-core@lists.launchpad.net. Three +1 votes from other existing core team members must be achieved for approval. - -How to review a documentation patch -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -Before you proceed with reviewing patches, make sure to read carefully the -`Review Guidelines`_ for documentation and `Code Review Guidelines`_. Once -done, follow the steps below to submit a patch review. - -#. Go to the `Documentation Program Dashboard`_. -#. Click a patch set. -#. Click a file that was uploaded to view the changes side by side. -#. If you see some inconsistencies or have questions to the patch owner, - double click the line in question for a *Comment* field to appear. - Click *Save* button once you write a draft of your comment. -#. In the *Jenkins check* section, click the Jenkins *checkbuild* gate - link (for the openstack-manuals, it is called - *gate-openstack-manuals-tox-doc-publish-checkbuild*) and review the - built manuals to see how the change will look on the web page. For a new - patch, it takes some time before Jenkins checks appear on the Gerrit - page. You can also `build the patch locally`_ if necessary. -#. Click *Reply* to vote and enter any comments about your review, - then click *Post*. - -.. note:: A patch with WorkInProgress (WIP) status needs additional work - before it gets merged. Therefore, you may skip such a patch and - review once it is ready. For more information, see - `Work In Progress`_. - -.. seealso:: `Peer Review`_ - -.. _`build the patch locally`: - -How to build an existing patch locally --------------------------------------- - -Before proceeding, make sure you have all the necessary -:ref:`tools ` installed and -set up for contribution. - -To build a patch locally: - -#. In terminal, switch to a necessary directory. For example:: - - cd openstack-manuals - -#. Run the below command to create a local branch with the patch in question:: - - git review -d - - where the value of is a Gerrit commit number. For example, 226632 - is the commit number of the patch https://review.openstack.org/#/c/226632. - -#. Build all the books that are affected by changes in the patch set:: - - sudo tox -e checkbuild - -#. Find the build result in :file:`openstack-manuals/publish-docs/index.html`. - -#. Review the source and the output. You are also welcomed to edit and update - the patch: - - #. Ensure that your edits adhere to the - :ref:`Writing Style ` for OpenStack documentation - and uses standard US English. - #. Once the build and new output are good to commit, run:: - - git commit -a --amend - - #. When the editor opens, update the commit message if necessary. But do - not add information on what your specific patch set changes. A reviewer - can use the Gerrit interface to see the difference between patches. - #. Save the changes if any and exit the editor. If your editor is vi, - use the :command:`:wq` command to save the file and exit vi. - #. Send your patch to the existing review:: - - git review - - #. Leave a comment in Gerrit explaining the reason for your patch set. - -.. TODO: make a seealso and add a links to the below pages once converted to - .RST: - - https://wiki.openstack.org/wiki/Documentation/HowTo#Building_Output_Locally - - https://wiki.openstack.org/wiki/Documentation/HowTo#Using_Tox_to_check_builds - -.. _`Documentation Program Dashboard`: http://is.gd/openstackdocsreview -.. _`Code Review`: http://docs.openstack.org/infra/manual/developers.html#code-review -.. _`Review Guidelines`: https://wiki.openstack.org/wiki/Documentation/ReviewGuidelines -.. _`Code Review Guidelines`: http://docs.openstack.org/infra/manual/developers.html#code-review -.. _`Peer Review`: http://docs.openstack.org/infra/manual/developers.html#peer-review -.. _`Work In Progress`: http://docs.openstack.org/infra/manual/core.html#work-in-progress