Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated the PR template to latest agreed format #2122

Merged
merged 7 commits into from
Feb 29, 2024
61 changes: 10 additions & 51 deletions pull_request_template.md
Original file line number Diff line number Diff line change
@@ -1,65 +1,24 @@
# Summary | Résumé

> 1-3 sentence description of the changed you're proposing, including a link to
> a GitHub Issue # or Trello card if applicable.
_TODO: 1-3 sentence description of the changed you're proposing._

---
## Related Issues | Cartes liées

> Description en 1 à 3 phrases de la modification proposée, avec un lien vers le
> problème (« issue ») GitHub ou la fiche Trello, le cas échéant.
* https://app.zenhub.com/workspaces/notify-planning-614b3ad91bc2030015ed22f5/issues/gh/cds-snc/notification-planning/1
* https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/1
jimleroyer marked this conversation as resolved.
Show resolved Hide resolved

# Test instructions | Instructions pour tester la modification

> Sequential steps (1., 2., 3., ...) that describe how to test this change. This
> will help a developer test things out without too much detective work. Also,
> include any environmental setup steps that aren't in the normal README steps
> and/or any time-based elements that this requires.

---

> Étapes consécutives (1., 2., 3., …) qui décrivent la façon de tester la
> modification. Elles aideront les développeurs à faire des tests sans avoir à
> jouer au détective. Veuillez aussi inclure toutes les étapes de configuration
> de l’environnement qui ne font pas partie des étapes normales dans le fichier
> README et tout élément temporel requis.
_TODO: Fill in test instructions for the reviewer._

# Release Instructions | Instructions pour le déploiement

None.
jimleroyer marked this conversation as resolved.
Show resolved Hide resolved

> Necessary steps to perform before and after the deployment of these changes.
> For example, emptying the cache on a feature that changes the cache data
> structure in Redis could be mentioned.

---

> Étapes nécessaires à exécuter avant et après le déploiement des changements
> introduits par cette proposition. Par exemple, vider la cache suite à des
> changements modifiant une structure de données de la cache pourrait être
> mentionné.

# 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 :


- [ ] Is the code maintainable? | Est-ce que le code peut être maintenu?
- [ ] Have you tested it? | L’avez-vous testé?
- [ ] Are there automated tests? | Y a-t-il des tests automatisés?
- [ ] Does this cause automated test coverage to drop? | Est-ce que ça entraîne
une baisse de la quantité de code couvert par les tests automatisés?
- [ ] Does this break existing functionality? | Est-ce que ça brise une
fonctionnalité existante?
- [ ] Does this change the privacy policy? | Est-ce que ça entraîne une
modification de la politique de confidentialité?
- [ ] Does this introduce any security concerns? | Est-ce que ça introduit des
préoccupations liées à la sécurité?
- [ ] Does this significantly alter performance? | Est-ce que ça modifie de
façon importante la performance?
- [ ] What is the risk level of using added dependencies? | Quel est le degré de
risque d’utiliser des dépendances ajoutées?
- [ ] Should any documentation be updated as a result of this? (i.e. README
setup, etc.) | Faudra-t-il mettre à jour la documentation à la suite de ce
changement (fichier README, etc.)?
- [ ] This PR does not break existing functionality.
jimleroyer marked this conversation as resolved.
Show resolved Hide resolved
- [ ] This PR does not violate GCNotify's privacy policies.
- [ ] This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we link to the Risk Register doc here 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

- [ ] This PR does not significantly alter performance.
- [ ] Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).
jimleroyer marked this conversation as resolved.
Show resolved Hide resolved
Loading