Skip to content

LG-7648: Create PR template #7015

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.

Sign up for GitHub

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

Merged
eileen-nava merged 3 commits into from
Sep 23, 2022
Merged

LG-7648: Create PR template #7015

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d53b9e8
create pr template
eileen-nava Sep 22, 2022
91033c4
changelog: Internal, In-person proofing, create template for PRs
eileen-nava Sep 22, 2022
6c7826e
drop the reference to jira
eileen-nava Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
## 🎫 Ticket

Link to the relevant ticket.

## 🛠 Summary of changes

Write a brief description of what you changed.

## 📜 Testing Plan

Provide a checklist of steps to confirm the changes.

- [ ] Step 1
- [ ] Step 2
- [ ] Step 3

## 👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

<details>
<summary>Before:</summary>

</details>

<details>
<summary>After:</summary>

</details>

## 🚀 Notes for Deployment
Copy link
Copy Markdown
Contributor

@aduth aduth Sep 26, 2022

Choose a reason for hiding this comment

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

Who is the intended audience of these notes? The deployer?

Copy link
Copy Markdown
Contributor Author

@eileen-nava eileen-nava Sep 27, 2022

Choose a reason for hiding this comment

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

Yes, I was thinking of scenarios where we might need to ensure that two PRs are deployed at the same time, or that one PR is deployed first. I saw the first scenario on a previous project. I was imagining it as a possible scenario on this project, too, but let me know if you think this section isn't relevant.

Copy link
Copy Markdown
Contributor

@aduth aduth Sep 27, 2022

Choose a reason for hiding this comment

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

I'd wonder how the deployer is to be made aware of these notes. Since there's nothing in the deployment guide to set the expectation that a deployer is to be reviewing each pull request for these notes, I don't think we can expect that they would.

For the scenarios you mentioned, those seem to me as being the responsibility of the code author to ensure any dependencies are merged when appropriate based on the regular deployment cadence, since those sorts of selective deployments would not be possible with the current process.

That being said, we could always revisit the steps of the deployment process itself, but I'd lean toward avoiding extra work for the deployer if we can delegate that to the individual contributors.

No need to act on it now if we'd want to reserve this kind of feedback for the planned follow-on discussion. 👍

Copy link
Copy Markdown
Contributor

@mitchellhenke mitchellhenke Sep 27, 2022

Choose a reason for hiding this comment

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

I agree, I'd guess the vast majority of PRs won't have unique notes for deployment and we could potentially drop this section.

Copy link
Copy Markdown
Contributor Author

@eileen-nava eileen-nava Sep 27, 2022

Choose a reason for hiding this comment

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

Yeah, I do agree with avoiding extra work for the deployer. Asking the deployer to review each PR’s “Notes for deployment” section would obviously make their job harder. Also, in the example I gave, the code author could use stacked PRs.

The follow-up discussion is October 26 in engineering huddle. Are you both fine with leaving the PR template as is until then?

Copy link
Copy Markdown
Contributor

@aduth aduth Sep 27, 2022

Choose a reason for hiding this comment

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

The follow-up discussion is October 26 in engineering huddle. Are you both fine with leaving the PR template as is until then?

Totally fine with me 👍


Include any special instructions for deployment.