Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions source/Development/planning_your_change.rst
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ aware of these.
<https://metoffice.github.io/lfric_core/how_to_contribute/index.html#how-to-contribute-index>`__
* `PEP 8 (Python) <https://legacy.python.org/dev/peps/pep-0008/>`__

**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.

Expand Down Expand Up @@ -97,7 +97,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
Expand Down
2 changes: 1 addition & 1 deletion source/Development/temp_logicals.rst
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,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
Expand Down
9 changes: 4 additions & 5 deletions source/FurtherDetails/coding_style.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1906,7 +1906,7 @@ rules are imposed.

.. _`sec:reviews`:

Code reviews
Code Reviews
============

In order to ensure that these standards are adhered to and are having the
Expand All @@ -1921,12 +1921,11 @@ performed in the order below:
<https://metoffice.github.io/simulation-systems/Reviewers/scitechreview.html>`__
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
<https://metoffice.github.io/simulation-systems/Reviewers/codereview.html>`__
page on the UM homepage.
is outlined in `Code Review Guidance page
<https://metoffice.github.io/simulation-systems/Reviewers/codereview.html>`__.


.. _Appendix A:
Expand Down
2 changes: 1 addition & 1 deletion source/FurtherDetails/glossary.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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
Expand Down
10 changes: 5 additions & 5 deletions source/Reviewers/codereview.rst
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.. _code_review:

Code and System Review
======================
Code Review
===========

.. tip::

Expand All @@ -12,7 +12,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.

Expand All @@ -32,7 +32,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?
Expand Down Expand Up @@ -120,7 +120,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.

Expand Down
12 changes: 6 additions & 6 deletions source/Reviewers/howtocommit.rst
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,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.

Expand Down Expand Up @@ -442,8 +442,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.
Expand Down
8 changes: 4 additions & 4 deletions source/Reviewers/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,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``.


Expand Down
8 changes: 4 additions & 4 deletions source/Reviewers/scitechreview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,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

Expand Down Expand Up @@ -59,10 +59,10 @@ The science/technical 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
Once you are happy that the change is appropriate and correct, and the SciTech
Review parts of the pull request template have been completed then change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a box here saying to either add the CR as a reviewer or tag them in a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turned things the scitech should do into a list, but not in a box. Is that enough?

the review state to Code Review and leave an approving review.

.. image:: ../WorkingPractices/images/gh_screenshots/project_scitech_light.png
Expand Down
2 changes: 1 addition & 1 deletion source/WorkingPractices/final_steps.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,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
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
101 changes: 0 additions & 101 deletions source/WorkingPractices/pull_requests.rst
Original file line number Diff line number Diff line change
Expand Up @@ -173,104 +173,3 @@ details see :ref:`updating a branch <updating_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 <scitech_review>`.

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 <code_review>`.

.. _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``



Loading