Updated the PR template to latest agreed format

[jimleroyer]

Summary | Résumé

Updated the PR template to latest agreed format!

And this very PR uses the latest format itself, this is meta. 🤖

Related Issues

• https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/304

Test instructions | Instructions pour tester la modification

Please proceed below 👇 to check the new boxes in the reviewer's checklist please. 🙏

Release Instructions | Instructions pour le déploiement

None.

Reviewer checklist | Liste de vérification du réviseur

This is a suggested checklist of questions reviewers might ask during their
review | Voici une suggestion de liste de vérification comprenant des questions
que les réviseurs pourraient poser pendant leur examen :

• Check that the current PR does not break existing functionality.
• Check that this does not violate GCNotify's privacy policies.
• Check that this does not raise any security concern.
• Check that this does not significantly alter performance.
• Check if additional documentation needs to be updated as a result of these changes such
  as the README, setup instructions, a related ADR or the technical documentation.

[sastels]

I ask out of ignorance since I missed the meeting yesterday: are there thoughts around what is sufficient around testing? in particular it may be hard to decide the affect on performance of, for example, celery changes, without doing some testing on staging after a PR is merged in. I would suggest in cases like this the reviewer shouldn't check the box but rather comment that we should do some staging tests before release. thoughts?

[jimleroyer]

I ask out of ignorance since I missed the meeting yesterday: are there thoughts around what is sufficient around testing? in particular it may be hard to decide the affect on performance of, for example, celery changes, without doing some testing on staging after a PR is merged in. I would suggest in cases like this the reviewer shouldn't check the box but rather comment that we should do some staging tests before release. thoughts?

Yes, there might be additional required tests that changes do not cover and need to be performed after the PR is merged. There might be a negotiation with the reviewer(s) and author(s). Do you see an additional requirement check or modifying an existing one to fit this use case in? Or we want to keep it this way, with an implicit understanding that this should happen? (and keep it short)

[sastels]

Can we link to the Risk Register doc here 🙏

[jzbahrai]

hmmm... what is the likelihood that a reviewer is going to click and scroll through that document each time? I'd err on not having it in the PR review

[jimleroyer]

I thought of linking it but I am paranoid. I checked access to the document and this is protected, but I fell for security through obscurity on this one as this is a sensitive document. I can put it there if you pay me cider later on.

[jzbahrai]

in response to your other comment jimmy and steve, I would like to keep it this way, with an implicit understanding that this should happen

[sastels]

I ask out of ignorance since I missed the meeting yesterday: are there thoughts around what is sufficient around testing? in particular it may be hard to decide the affect on performance of, for example, celery changes, without doing some testing on staging after a PR is merged in. I would suggest in cases like this the reviewer shouldn't check the box but rather comment that we should do some staging tests before release. thoughts?

Yes, there might be additional required tests that changes do not cover and need to be performed after the PR is merged. There might be a negotiation with the reviewer(s) and author(s). Do you see an additional requirement check or modifying an existing one to fit this use case in? Or we want to keep it this way, with an implicit understanding that this should happen? (and keep it short)

Maybe just a bit after the checkboxes, something like
"If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected."

[sastels]

This is such an improvement! 🎉

Let's followup in a month to see if it needs refinement.

[andrewleith]