BWA-21042: Add new pull request templates#5
Conversation
|
TBD: Do we want include useful links in templates, like: Create and upgrade account, TP admin |
| ### How ⁉️ | ||
| _How did you implement it? Mention key decisions or trade-offs, e.g.:_ | ||
| - _New or changed Lambda handlers — function name, trigger (API GW, SNS, SQS, EventBridge, scheduled)_ | ||
| - _Salesforce integration — which object(s) are created/updated, which fields map to which params_ | ||
| - _SNS/SQS — new topics or queues, message schema changes, DLQ behaviour_ | ||
| - _DynamoDB — new tables, GSI changes, or access pattern changes_ | ||
| - _Secrets / config — new SSM parameters or environment variables required_ | ||
| - _IAM — any new permissions added to execution roles_ |
There was a problem hiding this comment.
I'm never a huge fan of "how" as to me it just repeats the diff.
I prefer in the "How to review and test" if the diff is large or complex it is highlighted there and there's maybe some notes to guide a reviewer.
| ### External dependencies | ||
| _List all external systems this change touches:_ | ||
| - [ ] Salesforce — _object(s) and fields affected_ | ||
| - [ ] SNS / SQS — _topic / queue name(s)_ | ||
| - [ ] DynamoDB — _table name(s)_ | ||
| - [ ] Third-party API — _name and endpoint_ | ||
| - [ ] None |
| - [ ] None | ||
|
|
||
| ### How to review and test 📱 | ||
| _Steps to verify the feature in staging or locally:_ |
There was a problem hiding this comment.
I think it would be good to list helpful resources here for generating the correct users to save folk hunting in Confluence.
| _Steps to verify the feature in staging or locally:_ | ||
|
|
||
| **If the trigger is an HTTP endpoint:** | ||
| 1. Deploy to staging or run locally (`sam local start-api` / `serverless offline`). |
There was a problem hiding this comment.
| 1. Deploy to staging or run locally (`sam local start-api` / `serverless offline`). | |
| 1. Run locally (`assume trustpilot-classic && npm start`). |
maybe more helpful?
|
|
||
| **If the trigger is an HTTP endpoint:** | ||
| 1. Deploy to staging or run locally (`sam local start-api` / `serverless offline`). | ||
| 2. Call the endpoint: `curl -X POST https://<base-url>/<path> -d '<payload>'` |
There was a problem hiding this comment.
Maybe extend this to include the standard private/internal headers and request style?
| **If the trigger is SNS / SQS:** | ||
| 1. Publish a test message to the topic/queue: `aws sns publish --topic-arn <arn> --message '<payload>'` | ||
| 2. Check CloudWatch Logs for the Lambda execution: `aws logs tail /aws/lambda/<function-name> --follow` | ||
| 3. Describe the expected side-effect (e.g. Salesforce record created, DynamoDB item written). |
There was a problem hiding this comment.
I think we should have a separate guide for microservices as the testing is often different. (i.e usually using a runLocal.ts script
| **If the integration is Salesforce:** | ||
| 1. Trigger the flow (HTTP call or SNS message as above). | ||
| 2. Log in to Salesforce staging org and navigate to the relevant object (Account / Lead / etc.). | ||
| 3. Verify the field values match the payload. | ||
| 4. If the mapping is wrong, check `<config file or env var>` for the field mapping. |
There was a problem hiding this comment.
I think this is too specific as a lot of services don't touch salesforce
| **DLQ / error path:** | ||
| - Send a malformed payload and confirm the message lands in the DLQ rather than causing a silent failure. |
There was a problem hiding this comment.
again not too sure this happens in practise as when run locally the DLQ isn't connected.
|
|
||
| ### Checklist ✅ | ||
| - [ ] My changes generate no new warnings | ||
| - [ ] I have added tests that prove the feature works (unit + integration where applicable) |
There was a problem hiding this comment.
| - [ ] I have added tests that prove the feature works (unit + integration where applicable) | |
| - [ ] I have added tests that prove the feature works (unit + integration + e2e where applicable) |
| - [ ] External dependencies documented in the section above | ||
| - [ ] Salesforce field mapping verified against staging org (if applicable) | ||
| - [ ] DLQ / error path tested | ||
| - [ ] New SSM parameters / secrets documented in the ticket | ||
| - [ ] IAM permission changes are least-privilege | ||
| - [ ] Rollback plan is documented above | ||
| - [ ] Feature flag / gradual rollout considered if applicable |
There was a problem hiding this comment.
| - [ ] External dependencies documented in the section above | |
| - [ ] Salesforce field mapping verified against staging org (if applicable) | |
| - [ ] DLQ / error path tested | |
| - [ ] New SSM parameters / secrets documented in the ticket | |
| - [ ] IAM permission changes are least-privilege | |
| - [ ] Rollback plan is documented above | |
| - [ ] Feature flag / gradual rollout considered if applicable | |
| - [ ] error path tested | |
| - [ ] Rollback plan is documented above | |
| - [ ] Feature flag / gradual rollout considered if applicable |
| ### Checklist ✅ | ||
| - [ ] My changes generate no new warnings | ||
| - [ ] I have added tests that prove the feature works (unit + integration where applicable) | ||
| - [ ] New and existing tests pass locally |
There was a problem hiding this comment.
| - [ ] New and existing tests pass locally | |
| - [ ] New and existing unit/integration tests pass locally | |
| - [ ] Local E2E tests pass against your local branch (where applicable) | |
| ### How ⁉️ | ||
| _How did you implement it? Mention key decisions or trade-offs, e.g.:_ | ||
| - _Component hierarchy changes — which components were added, removed, or renamed_ | ||
| - _State management — Redux action/reducer changes, or new local state_ | ||
| - _Form logic — react-hook-form / Formik changes (validators, default values, submission)_ | ||
| - _LaunchDarkly — new or modified feature flags, and which component reads them_ | ||
| - _Tracking — new Segment events or changes to existing ones_ | ||
| - _Localisation — new or modified translation keys; which locales are affected_ |
There was a problem hiding this comment.
again, would prefer not to have this section (see previous comment)
| - _Tracking — new Segment events or changes to existing ones_ | ||
| - _Localisation — new or modified translation keys; which locales are affected_ | ||
|
|
||
| ### How to review and test 📱 |
There was a problem hiding this comment.
We should explicitly call our testing with keyboard and VoiceOver
| 5. If a LaunchDarkly flag controls the feature, describe how to toggle it in the local dev config or staging environment. | ||
| 6. Link to the CI run once tests pass. | ||
|
|
||
| _For responsive changes, test at mobile (375px), tablet (768px), and desktop (1280px) breakpoints._ |
There was a problem hiding this comment.
| _For responsive changes, test at mobile (375px), tablet (768px), and desktop (1280px) breakpoints._ | |
| 7. Open the browser DevTools and check responsiveness at all reasonable width and heights of screen (from small phone to horizontal tablet to 4k monitor) |
| ### Checklist ✅ | ||
| - [ ] My changes generate no new warnings | ||
| - [ ] I have added tests that prove the feature works | ||
| - [ ] New and existing tests pass locally | ||
| - [ ] Screenshots / GIFs included for all visual changes | ||
| - [ ] Translation keys added for all user-visible strings | ||
| - [ ] Segment tracking events verified (added / updated where needed) | ||
| - [ ] LaunchDarkly flag usage documented (if applicable) | ||
| - [ ] Rollback plan is documented above | ||
| - [ ] Feature flag / gradual rollout considered if applicable |
There was a problem hiding this comment.
| ### Checklist ✅ | |
| - [ ] My changes generate no new warnings | |
| - [ ] I have added tests that prove the feature works | |
| - [ ] New and existing tests pass locally | |
| - [ ] Screenshots / GIFs included for all visual changes | |
| - [ ] Translation keys added for all user-visible strings | |
| - [ ] Segment tracking events verified (added / updated where needed) | |
| - [ ] LaunchDarkly flag usage documented (if applicable) | |
| - [ ] Rollback plan is documented above | |
| - [ ] Feature flag / gradual rollout considered if applicable | |
| ### Checklist ✅ | |
| - [ ] My changes generate no new warnings | |
| - [ ] I have added tests (unit, integration and/or E2E where relevant) that prove the feature works | |
| - [ ] Local E2E tests have been run and are passing against my branch | |
| - [ ] New and existing tests pass locally | |
| - [ ] Screenshots / GIFs included for all visual changes | |
| - [ ] Translation keys added for all strings (including those for screen readers) | |
| - [ ] Segment tracking events verified (added / updated where needed) | |
| - [ ] LaunchDarkly flag usage documented (if applicable) | |
| - [ ] Rollback plan is documented above | |
| - [ ] Feature flag / gradual rollout considered if applicable |
There was a problem hiding this comment.
Personally I would prefer a gm-microservice.md over this.
There was a problem hiding this comment.
💭 Thoughts (non-blocking)
I think we can drop the -pr in the file name (for backend and frontend here)
| - [ ] New SSM parameters / secrets provisioned in all target environments (values never committed) | ||
| - [ ] IAM permission changes are least-privilege | ||
| - [ ] CloudFormation change set reviewed before deploy | ||
| - [ ] No secrets or credentials are committed |
There was a problem hiding this comment.
💭 Thoughts (non-blocking)
| - [ ] No secrets or credentials are committed |
since this is already mentioned above
There was a problem hiding this comment.
Also a bit late if you've raised a PR that contains secrets 😅
There was a problem hiding this comment.
Yeah I thought the same ahah 😅
That's not directly related to this PR, but maybe worth some day enabling the "Secret scanning" feature of GitHub (https://github.com/trustpilot/businessupgrades-businessapp/security/secret-scanning)
There was a problem hiding this comment.
we already have stuff scanning for secrets at the org level, if you commit something that looks like a secret, you'll know 😉
There was a problem hiding this comment.
Ah ok great!
Is it the "Secret scanning" feature of GitHub but at the org level? Or something else?
| 1. Publish a test message to the topic/queue: `aws sns publish --topic-arn <arn> --message '<payload>'` | ||
| 2. Check CloudWatch Logs for the Lambda execution: `aws logs tail /aws/lambda/<function-name> --follow` |
There was a problem hiding this comment.
that doesn't help to test the changes introduced in the PR, this only triggers the deployed version.
Instead, instruct to use the run-local script if there is one in the repo
| - [ ] New SSM parameters / secrets provisioned in all target environments (values never committed) | ||
| - [ ] IAM permission changes are least-privilege | ||
| - [ ] CloudFormation change set reviewed before deploy | ||
| - [ ] No secrets or credentials are committed |
There was a problem hiding this comment.
we already have stuff scanning for secrets at the org level, if you commit something that looks like a secret, you'll know 😉
There was a problem hiding this comment.
I am not convinced we need a specific template for infrastructure, front-end/back-end might be enough
4 participants
Ticket 🎟
BWA-21042
What⁉️
Added new GM team PR templates per service.
Why⁉️
The org-wide template at trustpilot/.github/PULL_REQUEST_TEMPLATE.md is intentionally generic and owned at the org level — we don't want to change it. But for our repos the generic prompts lead to inconsistent PRs and missing context (rollback, risk, test evidence, ticket link).
We want to build on / layer over the global template for just our repos, without forking or replacing it.