General updates to PR template #2
Conversation
homostellaris
force-pushed
the
dm/feat/update-pr-template
branch
from
00c509c to
ccba735
Compare
Some suggestions based on using it for a while.
homostellaris
force-pushed
the
dm/feat/update-pr-template
branch
from
ccba735 to
91d04ec
Compare
| @@ -1,51 +1,40 @@ | |||
| ## What does this change? | |||
There was a problem hiding this comment.
Removed this section as I very rarely use it now that the Notion integration exists and if general notes were needed I think it would be obvious for them to go at the top anyway.
| - [ ] I wrote new unit tests for this change | ||
| - [ ] I wrote new integration/e2e tests for this change | ||
| - [ ] I updated existing tests for this change | ||
| - [ ] I ran test commands to test this change (list below) |
There was a problem hiding this comment.
I personally didn't find these useful as I think adding tests is too basic to warrant a specific reminder and tests should always be run as part of CI anyway so no need to specify which ones have been run (they're all going to be run).
There was a problem hiding this comment.
adding tests is too basic to warrant a specific reminder
Sometimes they still get missed though! I think we could merge the unit/e2e to "added tests if required" but would rather keep a little reminder in
tests should always be run as part of CI anyway
That prompt is a reminder to run locally before opening the PR. I often find it a good reminder (saves our credits)
| - [ ] I have tested this change against all known scenarios and considered new ones. | ||
| - E.g. net negative import scenario for solution summary graph. | ||
| - [ ] I have tested this change against real data |
There was a problem hiding this comment.
Replaced with some reminders that I find a bit more useful encouraging me to think more deeply about different scenarios and take the time to get a variety of real data samples.
|
|
||
| ## Monitoring | ||
|
|
||
| - [ ] This change can be deployed to prod without triggering any alarms | ||
| - [ ] This change will be covered by our existing monitoring | ||
| (no new /metrics/dashboards/alarms are required) | ||
| - No new CloudWatch alarm or Sentry logging is required |
There was a problem hiding this comment.
Trying to give more specific examples.
|
|
||
| ## Rollout | ||
|
|
||
| - [ ] This change can be merged immediately into the pipeline upon approval (no upstream/downstream/config updates required first) | ||
| - [ ] This change can be merged immediately into the pipeline upon approval | ||
| - No upstream/downstream/config updates required first |
There was a problem hiding this comment.
Could we add a specific example of this?
|
|
||
| - [ ] I added images demonstrating the changes in desktop and mobile views | ||
| - [ ] I've checked the changes in Firefox and Safari |
There was a problem hiding this comment.
Not trying to say this is mandatory but I think we should consider it depending on the change as we currently have no automated capability for this and have missed browser-specific things quite a few times.
|
|
||
| - [ ] I requested review from jacklsmith |
There was a problem hiding this comment.
Removed this as I think its evident from the reviewers section of the PR if this has been done
|
|
||
| ### Design review | ||
| ## Rollback |
There was a problem hiding this comment.
Encouraged to write down specific rollback steps and moved into own section to create more room for this.
There was a problem hiding this comment.
rollback steps
I think this is pretty repo specific
Some suggestions based on using it for a while. Probably quite subjective so please do push back / discuss, I've highlighted my reasoning for particular bits in the diff.