diff --git a/source/Development/planning_your_change.rst b/source/Development/planning_your_change.rst index 38c3f3e2..efa80453 100644 --- a/source/Development/planning_your_change.rst +++ b/source/Development/planning_your_change.rst @@ -67,9 +67,9 @@ aware of these. `__ * `PEP 8 (Python) `__ -**Who will SciTech review the change?** This is a useful consideration as not +**Who will SciTech Review the change?** This is a useful consideration as not everyone who uses the repository has the knowledge or experience to review -every change that is being developed. Get in touch with your SciTech reviewer +every change that is being developed. Get in touch with your SciTech Reviewer early in the process as they will have valuable insights that can help to shape your change. @@ -103,7 +103,7 @@ upgrade macro to include the switch into the upgraded configuration. vital to make sure your code changes work when switched **on** and give the same answer when the code is run over different processor configurations. Producing a quick plot or plots to show the impact of your code and including -them on your pull request can aid your SciTech reviewer in showing that your code +them on your pull request can aid your SciTech Reviewer in showing that your code works properly. **Does the change need any new diagnostics to make sense of the code?** Many diff --git a/source/Development/temp_logicals.rst b/source/Development/temp_logicals.rst index 1006dde9..bc82deac 100644 --- a/source/Development/temp_logicals.rst +++ b/source/Development/temp_logicals.rst @@ -29,7 +29,7 @@ for two reasons: Neither of these statements suggest that the fix shouldn't be included --- in fact the opposite is true. The decision as to whether to include a temporary logical normally rests with the configuration owner, but with guidance from -the CodeSys reviewer and the Simulation Systems and Deployment team. In such +the Code Reviewer and the Simulation Systems and Deployment team. In such cases, the following guidance is followed: * Essential bug fixes (e.g. something which would on occasions cause the model diff --git a/source/FurtherDetails/coding_style.rst b/source/FurtherDetails/coding_style.rst index 03df563a..4bb248ac 100644 --- a/source/FurtherDetails/coding_style.rst +++ b/source/FurtherDetails/coding_style.rst @@ -1912,7 +1912,7 @@ rules are imposed. .. _`sec:reviews`: -Code reviews +Code Reviews ============ In order to ensure that these standards are adhered to and are having the @@ -1927,12 +1927,11 @@ performed in the order below: `__ page on the UM homepage. -#. A code/system review is performed to analyse the change for its impact, +#. A Code Review is performed to analyse the change for its impact, ensure that it meets this coding standard and to ensure that all concerned parties are made aware of changes that are required. Guidance for reviewers - is outlined in `Code/System Review Guidance - `__ - page on the UM homepage. + is outlined in `Code Review Guidance page + `__. .. _Appendix A: diff --git a/source/FurtherDetails/glossary.rst b/source/FurtherDetails/glossary.rst index c9a79663..31313051 100644 --- a/source/FurtherDetails/glossary.rst +++ b/source/FurtherDetails/glossary.rst @@ -19,7 +19,7 @@ Code Review Deadline: The date by which all pull requests aiming to be included in a release have been moved into code review. -CodeSys Review: +Code Review: A technical review of the changes involved in the pull request, including checks that code standards have been upheld and that the working practices have been followed. These reviews are generally completed by a member of the diff --git a/source/Reviewers/codereview.rst b/source/Reviewers/codereview.rst index 68bcac2c..6a8d89dd 100644 --- a/source/Reviewers/codereview.rst +++ b/source/Reviewers/codereview.rst @@ -6,8 +6,8 @@ .. _code_review: -Code and System Review -====================== +Code Review +=========== .. tip:: @@ -18,7 +18,7 @@ Code and System Review Purpose of the review --------------------- -The purpose of the code/system review is to analyse a change for its impact and +The purpose of the Code Review is to analyse a change for its impact and to ensure that all concerned parties are made aware of changes that are required. @@ -38,7 +38,7 @@ contains a Code Review section with questions to help you think through all the areas of concern. This Code Review section should be completed once you are finished. -Work through the code review template considering each question in turn. These +Work through the review template considering each question in turn. These will include areas such as: .. dropdown:: Is the pull request and testing complete? @@ -126,7 +126,7 @@ Final decision points and actions --------------------------------- The pull request will likely iterate between the reviewer and the developer -during the review process while retaining it's code review status. The Code +during the review process while retaining it's Code Review status. The Code Reviewer can `Resolve Conversations` when they feel each query has been satisfactorily answered. diff --git a/source/Reviewers/howtocommit.rst b/source/Reviewers/howtocommit.rst index 9eb92a91..eff0871b 100644 --- a/source/Reviewers/howtocommit.rst +++ b/source/Reviewers/howtocommit.rst @@ -112,17 +112,17 @@ To update the test suite for an upgrade macro, please run: --um=vnXX.Y_tZZZZ \ [--jules-path=/path/to/jules/clone] - where ``-\-um=vnXX.Y_tZZZZ`` is the ``AFTER_TAG`` of the latest + where ``--um=vnXX.Y_tZZZZ`` is the ``AFTER_TAG`` of the latest upgrade macro. If there is a macro for fcm_make or createbc then check that the makes ``version*_*.py`` has the correct BEFORE and AFTER tags and - append ``-\-makeum=vnXX.Y_tZZZZ`` and/or - ``-\-createbc=vnXX.Y_tZZZZ`` to the above command. + append ``--makeum=vnXX.Y_tZZZZ`` and/or + ``--createbc=vnXX.Y_tZZZZ`` to the above command. .. note:: - The ``-\-jules-path`` option is only required if there are + The ``--jules-path`` option is only required if there are linked changes to the ``rose-meta/jules-shared`` directory in the Jules repository. @@ -448,8 +448,8 @@ KGO differently* .. tab-item:: LFRic Apps + LFRic Core KGO Checksums are stored in the repository alongside the code and can - be updated using a script. This can be done by either the code - reviewer or by the developer (before submitting their changes for + be updated using a script. This can be done by either the Code + Reviewer or by the developer (before submitting their changes for review). In the latter case, the update will need redoing by the reviewer before commit if there are merge conflicts in the checksum files. diff --git a/source/Reviewers/index.rst b/source/Reviewers/index.rst index 1dcdb737..dfca49b8 100644 --- a/source/Reviewers/index.rst +++ b/source/Reviewers/index.rst @@ -12,15 +12,15 @@ Guides for Reviewers PRs will pass through a two-stage review. Linked PRs should work through this process together. -The first, scitech, review is carried out by someone with a good understanding +The first, SciTech, review is carried out by someone with a good understanding of the area being developed. Once satisfied they then pass the PR to a -code reviewer who takes a bigger picture and system view of the change. -Trivial PRs can bypass the scitech review. +Code Reviewer who takes a bigger picture and system view of the change. +Trivial PRs can bypass the SciTech Review. The change then passes to the Code Reviewer and in this stage the developer is encouraged to merge ``main`` into their branch and resolve any conflicts. -Once the reviews are complete the code reviewer is then responsible for the +Once the reviews are complete the Code Reviewer is then responsible for the final merge and commit of the change to ``main``. diff --git a/source/Reviewers/scitechreview.rst b/source/Reviewers/scitechreview.rst index d1b3a773..aab85782 100644 --- a/source/Reviewers/scitechreview.rst +++ b/source/Reviewers/scitechreview.rst @@ -32,7 +32,7 @@ Reviewer responsibilities and checkpoints The pull request template that populates the pull request description box contains a SciTech Review section with questions to help you think through all -the areas of concern. This SciTech review section should be completed once you are finished. +the areas of concern. This SciTech Review section should be completed once you are finished. The Science / Technical reviewer should @@ -61,18 +61,25 @@ The Science / Technical reviewer should Final decision points and actions --------------------------------- -The science/technical reviewer must demand that non-compliance is corrected +The SciTech reviewer must demand that non-compliance is corrected before a change is passed onto the next level of review. The pull request will likely iterate between the reviewer and the developer during -the review process while retaining it's sci/tech review status. +the review process while retaining it's SciTech Review status. -Once you are happy that the change is appropriate and correct, and the sci-tech -review parts of the pull request template have been completed then change -the review state to Code Review and leave an approving review. +Once you are happy that the change is appropriate and correct: + +#. Ensure the the SciTech parts of the pull request template have been completed. +#. Leave an approving review +#. Change the review state to Code Review +#. Request the review of the nominated Code Reviewer (or leave them a + message with ``@username`` if their review has already been requested) + +These steps are covered in more detail as part of the :ref:`review_process` .. image:: ../WorkingPractices/images/gh_screenshots/project_scitech_light.png :class: only-light border .. image:: ../WorkingPractices/images/gh_screenshots/project_scitech_dark.png :class: only-dark border. + diff --git a/source/WorkingPractices/final_steps.rst b/source/WorkingPractices/final_steps.rst index e889a30b..cd5555d2 100644 --- a/source/WorkingPractices/final_steps.rst +++ b/source/WorkingPractices/final_steps.rst @@ -16,7 +16,7 @@ there may be several commits to different trunks involved. Sometimes there can be a delay between a code change being approved and the commit to ``main``. This can be for a number of reasons and rarely will be - due to your change. If you have any concerns, please contact your CodeSys + due to your change. If you have any concerns, please contact your Code Reviewer in the first instance. Overnight and Weekly Testing diff --git a/source/WorkingPractices/images/gh_screenshots/project_scitech_dark.png b/source/WorkingPractices/images/gh_screenshots/project_scitech_dark.png index e94a431e..46c6b981 100644 Binary files a/source/WorkingPractices/images/gh_screenshots/project_scitech_dark.png and b/source/WorkingPractices/images/gh_screenshots/project_scitech_dark.png differ diff --git a/source/WorkingPractices/images/gh_screenshots/project_scitech_light.png b/source/WorkingPractices/images/gh_screenshots/project_scitech_light.png index 0094182b..bff68cef 100644 Binary files a/source/WorkingPractices/images/gh_screenshots/project_scitech_light.png and b/source/WorkingPractices/images/gh_screenshots/project_scitech_light.png differ diff --git a/source/WorkingPractices/images/gh_screenshots/rerequest_dark.png b/source/WorkingPractices/images/gh_screenshots/rerequest_dark.png new file mode 100644 index 00000000..64f6929d Binary files /dev/null and b/source/WorkingPractices/images/gh_screenshots/rerequest_dark.png differ diff --git a/source/WorkingPractices/images/gh_screenshots/rerequest_light.png b/source/WorkingPractices/images/gh_screenshots/rerequest_light.png new file mode 100644 index 00000000..84fe07ad Binary files /dev/null and b/source/WorkingPractices/images/gh_screenshots/rerequest_light.png differ diff --git a/source/WorkingPractices/pull_requests.rst b/source/WorkingPractices/pull_requests.rst index 6882d403..a892ec40 100644 --- a/source/WorkingPractices/pull_requests.rst +++ b/source/WorkingPractices/pull_requests.rst @@ -179,104 +179,3 @@ details see :ref:`updating a branch `. development should be done on a branch from ``stable`` without merging in changes from ``main``. Only when the development has been completed and the pull request is almost ready for commit should you merge in ``main``. - -Selecting Reviewers -------------------- - -There is a space in the pull request template to list the GitHub user ID of the -scitech and code reviewers. Once these are filled in a GitHub Action will add -this user as an ``assignee`` to the pull request. - -.. tip:: - - Github allows reviewers to directly make suggestions to the code. This is - very useful for easily suggesting changes. However, the developer should - always check carefully that the change is sensible and doesn't contain any - errors or bugs. - -SciTech Review -^^^^^^^^^^^^^^ - -First refusal for completing the SciTech review should go to the main code -owner(s) for the area affected. If they don't want to then they may have -suggestions for other suitable reviewers or you can approach anyone who would -have good insight into the changes made. - -Once you have found a reviewer add their GitHub user ID to the pull request -description and request their review by clicking the cog on the `Reviewers` -pane on the right of the pull request and selecting their name or GitHub user -ID. - -.. image:: images/gh_screenshots/review_cog_light.png - :class: only-light border - -.. image:: images/gh_screenshots/review_cog_dark.png - :class: only-dark border - -Guidance for the SciTech reviewer can be found on the -:ref:`SciTech review page `. - -Code Review -^^^^^^^^^^^ - -Code reviewers are assigned by the Simulation Systems and Deployment Team from -a pool of repository maintainers. New ``ready for review`` pull requests will be -assigned a reviewer on a regular basis. If you need your pull request looking at -more urgently than that, or think your pull request has been overlooked, then -leave a comment for ``@ssdteam`` on the pull request. - -The assigned person will be listed in the pull request description and -selected as a reviewer. A label will also be added to the pull request to help -track which pull requests are waiting for a code reviewer to be assigned. - -Guidance for the Code reviewer can be found on the -:ref:`Code review page `. - -.. _reviewer_edits: - -Code Reviewer Edits -^^^^^^^^^^^^^^^^^^^ - -As part of the process to commit certain tickets, code reviewers will sometimes -need to commit changes to the branch of a developer. Common reasons for doing -this include, - -* Updating KGO's -* Applying upgrade macros -* Updating commit hashes for linked tickets - -The ability to commit back to another users fork is only available to those with -``maintainer`` access or above and they can only do so for branches with an open -pull request and the ``Allow edits by maintainers`` option selected. - - -Tracking Review Status ----------------------- - -All open pull requests will be added to a GitHub Project called -``Review Tracker``. This is used to give pull requests a status that -distinguishes between the different review states. Some states are achieved -automatically, some require changing manually: - -* When the developer feels a PR is ready for the SciTech or Code Reviewer to - look at the state should be **manually** changed to ``SciTech Review`` or - ``Code Review`` as appropriate. - -* When the SciTech Review has been completed the state should be **manually** - changed to Code Review. - -.. image:: images/gh_screenshots/project_scitech_light.png - :class: only-light border - -.. image:: images/gh_screenshots/project_scitech_dark.png - :class: only-dark border - -Automatic changes include: - -* When changes are requested by a reviewer the state becomes ``Changes Requested`` -* When the pull request has been approved the state becomes ``Approved`` -* When the pull request has been merged, or otherwise closed, the state becomes - ``Done`` - - - diff --git a/source/WorkingPractices/reviews.rst b/source/WorkingPractices/reviews.rst new file mode 100644 index 00000000..71a76214 --- /dev/null +++ b/source/WorkingPractices/reviews.rst @@ -0,0 +1,142 @@ +.. _review_process: + +Review Process +============== + +.. tip:: + + Github allows reviewers to directly make suggestions to the code. This is + very useful for easily suggesting changes. However, the developer should + always check carefully that the change is sensible and doesn't contain any + errors or bugs. + +Selecting Reviewers +------------------- + +There is a space in the pull request template to list the GitHub user ID of the +scitech and code reviewers. These can also be filled into the appropriate +:ref:`project spaces `. + +SciTech Review +^^^^^^^^^^^^^^ + +First refusal for completing the SciTech review should go to the main code +owner(s) for the area affected. If they don't want to then they may have +suggestions for other suitable reviewers or you can approach anyone who would +have good insight into the changes made. + +Once you have found a reviewer add their GitHub user ID to the pull request +description and request their review. + +Guidance for the SciTech reviewer can be found on the +:ref:`SciTech review page `. + +Code Review +^^^^^^^^^^^ + +Code reviewers are assigned by the Simulation Systems and Deployment Team from +a pool of repository maintainers. New ``ready for review`` pull requests will be +assigned a reviewer on a regular basis. If you need your pull request looking at +more urgently than that, or think your pull request has been overlooked, then +leave a comment for ``@ssdteam`` on the pull request. + +The assigned person will be listed in the pull request description and +:ref:`Review Tracking project `. Once the SciTech review has +been completed either the developer or SciTech reviewer should request the +review of the assigned Code Reviewer. + +Guidance for the Code reviewer can be found on the +:ref:`Code review page `. + +.. admonition:: Requesting a Review + + Review requests are handled in the ``Reviewers`` pane on the right hand + side of a pull request. + + Select the cog, and then search for the person you wish to review by + either name or GitHub user ID. + + .. image:: images/gh_screenshots/review_cog_light.png + :class: only-light border + + .. image:: images/gh_screenshots/review_cog_dark.png + :class: only-dark border + + Code owners in some repositories will automatically be added to this + reviewers section based on the files being changed. + + +.. _review_project: + +Simulation Systems Review Tracker +--------------------------------- + +All open pull requests will be added to a GitHub Project called +``Simulation Systems Review Tracker``, which causes this box to appear in the +sidebar of the pull request: + +.. image:: images/gh_screenshots/project_scitech_light.png + :class: only-light border + +.. image:: images/gh_screenshots/project_scitech_dark.png + :class: only-dark border + +The project is used to give pull requests a status that distinguishes between +the different review states, and to monitor who is doing the reviews. + +Some states are achieved automatically, some require changing manually: + +* When the developer feels a PR is ready for the SciTech or Code Reviewer to + look at (either initially, or after changes have been made) the state should + be **manually** changed to ``SciTech Review`` or ``Code Review`` as + appropriate. + +* When the SciTech Review has been completed the state should be **manually** + changed to Code Review. + +* When the Code Review has been completed the state should be **manually** + changed to Approved. + +Automatic changes include: + +* When changes are requested by a reviewer the state becomes ``Changes Requested`` +* When the pull request has been merged, or otherwise closed, the state becomes + ``Done`` + +.. important:: + Changing the project status **does not** notify the reviewer. To do this: + + * When the SciTech Review has been completed you should add the assigned Code + Reviewer to the list of reviewers. This will notify them that their review is + required. + + * If a reviewer has requested changes then you can alert them that you are + ready for another review by using GitHubs ``rerequest review`` option; + selecting the circling arrows to the right of the reviewers name. + + .. image:: images/gh_screenshots/rerequest_light.png + :class: only-light border + + .. image:: images/gh_screenshots/rerequest_dark.png + :class: only-dark border + + * You can ``@username`` in any comment to draw that persons attention to the + pull request. + + +.. _reviewer_edits: + +Code Reviewer Edits +------------------- + +As part of the process to commit certain tickets, code reviewers will sometimes +need to commit changes to the branch of a developer. Common reasons for doing +this include, + +* Updating KGO's +* Applying upgrade macros +* Updating commit hashes for linked tickets + +The ability to commit back to another users fork is only available to those with +``maintainer`` access or above and they can only do so for branches with an open +pull request and the ``Allow edits by maintainers`` option selected. diff --git a/source/WorkingPractices/working_practices.rst b/source/WorkingPractices/working_practices.rst index 6980020e..f8731456 100644 --- a/source/WorkingPractices/working_practices.rst +++ b/source/WorkingPractices/working_practices.rst @@ -92,7 +92,7 @@ Remote Upstream The primary or parent repository, owned by the MetOffice GitHub - organisation. Only code reviewers are able to directly interact with this + organisation. Only Code Reviewers are able to directly interact with this repository, rather than being required to use forks. @@ -162,13 +162,13 @@ The development cycle can be seen below. and you have included proof of local testing. #. Pass the pull request for review. Usually this will involve first a - :ref:`Sci Tech review ` and then a :ref:`Code review + :ref:`SciTech review ` and then a :ref:`Code Review `. If any changes are required ensure the testing still passes. - * Once in code review, you will likely need to update your branch to the + * Once in Code Review, you will likely need to update your branch to the :ref:`head of main `. This may involve solving merge conflicts. -#. Once the pull request has been approved, the code reviewer will merge it to +#. Once the pull request has been approved, the Code Reviewer will merge it to `main`. For detailed explanation of these steps, see the subsequent pages, particularly @@ -244,5 +244,6 @@ helpful for documenting and monitoring progress of your work. multi_repository approvals pull_requests + reviews final_steps branch_migration