Review process - #433
Conversation
Jenny Hickson (jennyhickson)
added
the
crSet
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Nothing particularly controversial I think
| are marked on the :ref:`Ticket Summary wiki pages <template>` by those signing | ||
| off the approval. | ||
| Every pull request will need to get approval from a group of people. These | ||
| approvals are marked in the pull request description by those signing off the |
There was a problem hiding this comment.
Github Reviewers?
There was a problem hiding this comment.
Hmm, realised this page needs going over more generally. And presumeably config owners will still need to be via the trac.log.
There was a problem hiding this comment.
Not convinced it needs to stay in the trac.log? For UM we'll need a custom action, which can use the code in my open suite_report PR. But for Apps I think we have a normal looking CODEOWNERS file? But sure, we can revisit this section later when things are a touch clearer
| .. tip:: | ||
|
|
||
| From vn13.2 the :ref:`trac.log <traclog>` summary of your testing will also | ||
| The :ref:`trac.log <traclog>` summary of your testing will also |
There was a problem hiding this comment.
Github Reviewers?
|
|
||
| * Review suggestions and comments are formed into conversations. The `Resolve | ||
| Conversation` button is used by the reviewer when they are satisfied to | ||
| close that part of the review. The developer should not resolve |
There was a problem hiding this comment.
Maybe make the not bold here?
There was a problem hiding this comment.
done
| suggestions for other suitable reviewers or you can approach anyone who would | ||
| have good insight into the changes made. | ||
|
|
||
| Changes that have a linked LFRic Core ticket should find a SciTech reviewer from |
There was a problem hiding this comment.
I'm not sure this line is needed - they don't have to be specifically SR for a linked Apps ticket? There are Apps-Core science tickets that would benefit from an actual science review?
There was a problem hiding this comment.
I think this came about (the line was pre-existing in the old reviewers file) because they felt that Core tickets needed a CCD reviewer for either science or code review. But perhaps thats just one for us to keep an eye on as assigners
There was a problem hiding this comment.
Yeah I think so - I'm in the habit of doing so
There was a problem hiding this comment.
done
Co-authored-by: James Bruten <109733895+james-bruten-mo@users.noreply.github.com>
Co-authored-by: James Bruten <109733895+james-bruten-mo@users.noreply.github.com>
Co-authored-by: James Bruten <109733895+james-bruten-mo@users.noreply.github.com>
Co-authored-by: James Bruten <109733895+james-bruten-mo@users.noreply.github.com>
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Just one small suggestion
Co-authored-by: James Bruten <109733895+james-bruten-mo@users.noreply.github.com>
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Ta
9872f94
into
MetOffice:github_wps
2 participants
A refresh of the review process documentation including updating the pull request page with details of review states and reviewers, and updating the code review and scitech review guidance to remove references to tickets and templates.