Update pull-request-template.md

[ehanson8]

Purpose and background context

I'm PRing this repo first to refine the template and then I will PR python-lambda-template. Nitpick away, no suggestion is too minor as we refine this 🙂

How can a reviewer manually see the effects of these changes?

View this PR for an example of the updated template.

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO

What are the relevant tickets?

• NA

Developer

• All new ENV is documented in README
• All new ENV has been added to staging and production environments
• All related Jira tickets are linked in commit message(s)
• Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

Reviewer 1	Reviewer 2	Checklist
		The commit message is clear and follows our guidelines (not just this PR message)
		There are appropriate tests covering any new functionality
		The documentation has been updated or is unnecessary
		The changes have been verified
		New dependencies are appropriate or there were no changes

[github-actions]

Pull Request Test Coverage Report for Build 6787697486

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 6512809818:	0.0%
Covered Lines:	36
Relevant Lines:	36

---

💛 - Coveralls

[ehanson8]

Not totally happy about this table but let me know your thoughts

[ghukill]

Feels like a tricky thing to get right, that depends on what we want out of it.

I know I proposed the double checkbox columns, but there feel like lots of edge cases. What if 1 reviewer? or 3? Will it make sense to people outside of DataEng?

That said, feels workable? Maybe some repititions with it will give us a sense if we like it or not? I'd be okay moving forward with this approach.

[ghukill]

I think it looks good. Approving, with some questions and comments, but approved if those changes aren't adopted.

Overall, I think repititions with it will be the best feedback. If and when this PR is approved, I'd propose that we just assume we might be tweaking it further down the road.

[ghukill]

Feels like a tricky thing to get right, that depends on what we want out of it.

I know I proposed the double checkbox columns, but there feel like lots of edge cases. What if 1 reviewer? or 3? Will it make sense to people outside of DataEng?

That said, feels workable? Maybe some repititions with it will give us a sense if we like it or not? I'd be okay moving forward with this approach.

[ehanson8]

Thinking about this some more, do we actually need the changes verified twice? And couldn't disagreements about commit messages or dependencies be hashed out in the comments? Adding a 2nd checkbox would be a change in practice that's potentially confusing to non-DataEng. We could keep it to 1 set of check boxes as a reminder to all reviewers and whoever gets there first can check them. If a subsequent reviewer disagrees, they could uncheck and/or comment.

While I see the utility of reminding every reviewer of these, I'm not sure each item needs to be done multiple times. But I want to hear thoughts from everyone, I'm not firm about this

[ghukill]

Thinking about this some more, do we actually need the changes verified twice? And couldn't disagreements about commit messages or dependencies be hashed out in the comments? Adding a 2nd checkbox would be a change in practice that's potentially confusing to non-DataEng. We could keep it to 1 set of check boxes as a reminder to all reviewers and whoever gets there first can check them. If a subsequent reviewer disagrees, they could uncheck and/or comment.

While I see the utility of reminding every reviewer of these, I'm not sure each item needs to be done multiple times. But I want to hear thoughts from everyone, I'm not firm about this

I agree. I appreciate you putting them into multiple columns to see, but it feels like it would ultimately be more confusing (e.g. "Was I reviewer 1 or 2..."). If one reviewer has confirmed and checked the box, that should be sufficient, for the reasons you stated. The PR and inline comments have been great, and we don't need to recreate all that here. I'd be in favor of keeping just a single column.

Maybe we could do a tiny reword to "Code Reviewer(s)", just enough to indicate it's potentially multiple people contributing to those checking...

[ehanson8]

Pushed some changes based on our discussion

[ghukill]

Pushed some changes based on our discussion

This is what it looks like now:

I think it looks great. Simple, clean, understandable. Thanks for making these updates.

[jonavellecuerdo]

For the 3rd checkbox for "Code Reviewer(s)", can we update the statement to instead read:

• The provided documentation is sufficient for understanding any new functionality introduced.

[ehanson8]

For the 3rd checkbox for "Code Reviewer(s)", can we update the statement to instead read:

• The provided documentation is sufficient for understanding any new functionality introduced.

Great suggestion!

[jonavellecuerdo]

Thank you! Everything looks great. 🎉