add PR template #25949

pdpinch · 2020-12-28T13:21:42Z

Description

This PR adds a pull request template in Github. The contents of the template will be used as the default content of a pull request description, and PR authors will edit it before submitting a pull request.

This PR will only affect developers.

Screenshot of pull request template as the PR author will see it:

Screenshot of pull request template as reviewer will see it:

Supporting information

Fixes openedx/open-edx-proposals#171

Testing instructions

As far as I know, the only way I know to test PR templates is to merge them and create a PR

If you don't already have a suitable fork, fork edx-platform
Merge this PR into master on your fork
Open a PR on your fork
The PR description should be pre-populated with the contents of pull_request_template.md

Deadline

None

Other information

The template includes a link to an OEP PR. Once that OEP has been merged, the template should be updated.

openedx-webhooks · 2020-12-28T13:21:47Z

Thanks for the pull request, @pdpinch! I've created OSPR-5315 to keep track of it in JIRA.

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

nasthagiri · 2020-12-28T21:18:34Z

@pdpinch Thanks for putting this in a PR. Can you add the "meta" (desired outcomes of each element) as markdown comments in the template? This will allow us to co-locate the rationale for what we choose to include and exclude - so we can reassess overtime.

natabene · 2020-12-29T01:28:18Z

@pdpinch Thank you for your contribution. @nedbat You will want to have a look at this.
My personal wish, it is possible to do - include information on which role the change will have impact. E.g. learner, course author, developer, etc. That would help me quite a bit. I have to ensure all PR is with UI or behavior changes, visible to learners, course author, or course admins go through a product review first. Indication of role impacted would help me streamline this process. But if it adds to much burden on PR author, it is fine.

nedbat · 2020-12-29T18:36:27Z

I love the idea of including reasoning for the information we are requesting. But markdown comments are not a good way to do it unless you want people editing their pull requests to see them. (BTW, that link about markdown comments is very odd: most of the syntax doesn't work as comments on GitHub at all, and three dashes are unnecessary).

Maybe an ADR (or OEP since this will likely be rolled out more broadly) for the rationale?

nasthagiri · 2020-12-29T20:04:18Z

An ADR is fine as well. I mentioned MD comments since (1) we don't want to display the rationale in the PR template itself (the rationale is needed for long-term maintenance of the template, not for completing the template), and (2) it's the closest location to the template elements. I'd like to be able to read the template content, annotated with why it's there.

If Github allows the template to be in RST format, we can use RST comments instead.

nasthagiri · 2020-12-29T20:09:10Z

@natabene I like the idea of prompting OSPR contributors to complete a short-survey that will help us route the PR faster. You mention providing information on whether the PR impacts UI and which user persona (author/learner/etc). That's a great idea. Can we put this survey in the OSPR comment that is added after the BOT determines that it's an OSPR - rather than in the default PR Template for all PRs? (I'm trying to ensure the PR template is relevant to the vast majority of our PRs - so it doesn't get ignored over time.)

nedbat · 2020-12-30T00:28:42Z

It doesn't matter what markup we use (rst or md): the author of the PR will be presented with the full text including comments, when creating a pull request. If we have text like rationales that we don't want to distract the author of the pull request, then it has to go somewhere other than the pull request template.

About UI impact and personas affected: this seems like useful information for all pull requests, especially as we are trying to tighten up how we communicate changes to the code.

pdpinch · 2020-12-30T15:44:56Z

Thinking about where to put rationales led me to take a look at CONTRIBUTING.rst which lead me eventually to https://edx.readthedocs.io/projects/edx-developer-guide/en/latest/process/cover-letter.html -- which I probably should have read before I got started.

Clearly the developer's guide needs an update. Would that be a good place to link to for rationales?

pdpinch · 2020-12-30T15:56:21Z

@natabene I added a section for "user impact" but I'm a little torn because I feel like this should naturally be part of the description. On the other hand, it would be valuable for me as a reviewer to see at glance which users are impacted.

nedbat · 2021-01-04T12:32:41Z

It feels like we have a few different approaches to the same problem in flight. I took the liberty of trying to collect them together: Draft OEP: Change Transparency. This could be the place to put the rationale for the pull request template.

TBH, it was hard to come up with reasons for why it's good to provide this information, since it seems kind of self-evident. I'm happy to collaborate on them.

nasthagiri · 2021-01-04T15:17:10Z

it was hard to come up with reasons for why it's good to provide this information, since it seems kind of self-evident.

@nedbat Can you expand further on what you mean by this statement? Is this in regards to the rationale for the requested information in the PR template?

nedbat · 2021-01-04T15:22:28Z

it was hard to come up with reasons for why it's good to provide this information, since it seems kind of self-evident.

@nedbat Can you expand further on what you mean by this statement? Is this in regards to the rationale for the requested information in the PR template?

Yes, sorry, this is about the rationales. As I was trying to express why we wanted the information, I was having a hard time coming up with detailed reasons because it seems obvious to me why this information is useful. So I could use some help.

nasthagiri · 2021-01-04T17:15:35Z

it seems obvious to me why this information is useful

Yes, I can see that. For those immersed in the project, the reasons may seem obvious - hence even more so important to externalize them for others, IMO. Also, when we externalize the outcomes, we may find duplicative efforts that we can optimize.

So I could use some help.

I'm a bit confused about what and where we want to document at this point. This particular PR and its parent GitHub Issue are specific to PR templates (and also specific to edx-platform right now). I think a PR template is a good starting point and we have concrete ideas for it right now - so we can move forward on it.

nedbat · 2021-01-04T19:19:38Z

Absolutely we can move forward with the PR template, I didn't mean to suggest otherwise. I liked your idea of capturing the motivation for collecting information. Since we can't do it in the template, I started another document.

nedbat · 2021-01-19T22:14:14Z

I tightened up the text, and reduced the number of sections, so that it didn't feel like a multi-part form with required sections. What do you think? (the link in the comment at the top would be changed to a real doc about the template.)

<!-- 
Use this template as a guide. Omit sections that don't apply.
More details about the template are at https://github.com/edx/open-edx-proposals/pull/180 
-->

## Description

Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions should be documented in the repo (docstring / README / ADR) and linked here.

Useful information:
- Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
"Developer" and "Admin".
- Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
- Configuration changes needed.

## Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions. 
Be sure to check they are publicly readable, or if not, repeat the information here.

## Deadline

"None" if there's no rush, "ASAP" if it's critical, or provide a specific date or event if there is one.

## Other information

Include anything that will help reviewers and consumers of the change.
- Does this change depend on other changes elsewhere?
- How can it be tested?
- Any special concerns or limitations? Migrations? A11y?

pdpinch · 2021-01-20T03:31:47Z

Thanks @nedbat. My only hesitation is the diminished importance of testing instructions. I consider those the most useful part of the PR, aside from the description. What do you think of bumping "How can it be tested" up to the 1st or 2nd bullet under "useful information" ?

nasthagiri

Thanks for pulling this together. I like the overall structure. I have a few comments for considersation, which hopefully strengthen the proposal and its adoption.

nasthagiri · 2021-01-20T13:07:52Z

.github/pull_request_template.md

@@ -0,0 +1,31 @@
+<!--
+Use this template as a guide. Omit sections that don't apply.
+More details about the template are at https://github.com/edx/open-edx-proposals/pull/180


Can we have the rationale and expected outcomes of the template documented somewhere in the repo itself? Perhaps in a top-level ADR folder.

Relatedly, if one were to measure the impact of having this template, what would the hypotheses be? I want to ensure we are aligned on the value of this effort (rollout, adopt, future maintenance).

This is a template in edx-platform, but if it succeeds, it will be spread to other repos or perhaps even the organization. I'd rather not write an ADR in this specific repo about it.

Idea: You can start with an ADR in edx-platform for now, since that's where we're rolling this out - and we need a place document this decision. Each repo that chooses to adopt this template, can have their own ADR for the adoption and link to the one in edx-platform. The repo may have their own modifications/additions, which they can capture in their repo-specific ADR.

nasthagiri · 2021-01-20T13:16:22Z

.github/pull_request_template.md

+
+Useful information:
+- Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
+"Developer" and "Admin".


What does "Admin" mean in this context? "Course Admin" and "Django Admin" are very different personas. Maybe include "Operator"?

Also, do we want this to be a structured field that we can parse for and use in automation? Or is this a reminder for folks to include in their description?

Finally, in order to "sell" and "maintain" this info, it would be great to know why we believe this info is necessary to ask people to specify.

I've changed it to "Operator".

As I've said, I could use some help expressing the "why" for each piece of information. I would rather not put all the whys into the PR template: they will make it bulkier and harder to navigate. We have a draft of a document for the reasons and backstory for the information we need from developers: openedx/open-edx-proposals#180 Can we add to that document?

Indeed. Scratch my earlier comment about putting the rationale in an ADR.

If we can expand the doc you linked with more info/rationale about each field.

nasthagiri · 2021-01-20T13:16:58Z

.github/pull_request_template.md

+Useful information:
+- Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
+"Developer" and "Admin".
+- How can it be tested?


"How can it be tested" seems out of place in the "Description" section.

As I noted earlier, I feel that "how can it be tested" is one of the most useful parts of the PR description. I fear that if it is relegated to "Other Information" in a middling bullet it will be omitted regularly.

If it doesn't belong here, can we elevate it to the ## level?

@pdpinch Can you clarify your position with a reference example of what information you expect from the "how can it be tested" section? Also, what outcome are you expecting? Faster review cycle? Is the test instructions intended to be short-term just for the review cycle or longer-term for later reference?

I wrote a "Testing instructions" section in the PR description for this PR.

Outcome: "Testing instructions" are primarily intended to speed up the review cycle, avoiding the reviewer wasting time trying to infer the steps to test from the description. Instructions can also be useful for later reference if a future developer is researching regression.

I love testing instructions in PRs. It shows the author has thought carefully about the change and gives me a place to start.

I do wonder if How can it be tested is the best wording. My suggestion might be, Please provide step-by-step test instructions (to be explicit that we want this, and what we are looking for)

I've moved testing instructions to a top-level section.

nasthagiri · 2021-01-20T13:18:40Z

.github/pull_request_template.md

+"Developer" and "Admin".
+- How can it be tested?
+- Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
+- Configuration changes needed.


Instead of having people include config info in the PR description, what if we remind them to annotate their configs and link to the config annotations? Robert may have a suggestion for this.

"Provide links to the description of corresponding configuration changes. Remember to correctly annotate these changes."

.github/pull_request_template.md

nasthagiri · 2021-01-20T13:28:35Z

.github/pull_request_template.md

+
+Include anything that will help reviewers and consumers of the change.
+- Does this change depend on other changes elsewhere?
+- Any special concerns or limitations? Migrations? A11y?


Seems like info about testing could probably go in this section.

Should we spell out the word a11y for developers who may be new to the concept of accessibility?

@nasthagiri I'm not sure I agree that Testing should be nested under Other Information. I think testing is very important to have thought through and spell out that having it as a higher-up section makes sense to me.

I'm supportive of us including "Testing" in its own section, assuming we agree on its value. "Testing" didn't seem to belong in the "Description" section. @pdpinch has taken a stab at proposing its value in this comment.

nasthagiri · 2021-01-20T13:30:24Z

.github/pull_request_template.md

+
+## Deadline
+
+"None" if there's no rush, "ASAP" if it's critical, or provide a specific date or event if there is one.


@natabene Can you take a look at this "Deadline" section? Are there other field values for "deadline" that would be useful for OSPR triage?

Also, "ASAP" seems too general. Does that mean minutes/hours/days?

I've removed ASAP.

nasthagiri · 2021-01-20T13:32:07Z

.github/pull_request_template.md

@@ -0,0 +1,31 @@
+<!--


Should any of these be checklists instead of bullet points?

I didn't see any of them as simple check-offs, since the instructions are asking for information. Perhaps, "[ ] I have written a docstring/readme/adr" could be a check mark, but I'd rather have a link to it.

.github/pull_request_template.md

nedbat · 2021-01-20T18:28:04Z

BTW, some other possible things to cover, reading over Change Transparency: deprecations, reverts, and chores.

.github/pull_request_template.md

nedbat · 2021-01-21T21:48:18Z

@pdpinch I've made one last tweak using Nimisha's words. Are we good to go?

felipemontoya · 2021-01-25T21:57:21Z

fwiw I left my general approval as a core commiter here. I don't have specific feedback, I very much liked what I saw on the template

nasthagiri

This looks good to me overall! Thanks for all the effort.

Let's include the final touches before merging.

nasthagiri · 2021-01-26T00:18:54Z

.github/pull_request_template.md

+## Description
+
+Describe what this pull request changes, and why. Include implications for people using this change.
+Design decisions and their rationales should be be documented in the repo (docstring / ADR), per [OEP-19](https://open-edx-proposals.readthedocs.io/en/latest/oep-0019-bp-developer-documentation.html), and can be linked here.


Nit: be be -> be

.github/pull_request_template.md

Co-authored-by: Ned Batchelder <[email protected]>

pdpinch · 2021-01-26T13:21:00Z

@nasthagiri @nedbat I made the requested changes, and updated the PR description to model the template.

I also updated the commit message, but I'm new to conventional commits and this is an unusual one. Let me know what you think.

edx-status-bot · 2021-01-26T13:41:16Z

Your PR has finished running tests. There were no failures.

nedbat · 2021-01-26T18:46:34Z

Conventional commits are one of those categorization exercises that can be tricky on outliers like this commit. I might have used "chore", but I think "docs" is better: these words are explaining something to devs, so docs is a good fit.

nedbat · 2021-01-27T17:39:30Z

Thanks @pdpinch and @nasthagiri :)

openedx-webhooks · 2021-01-27T17:39:43Z

@ormsbee, @nasthagiri: thought you might like to know that pdpinch merged this pull request.

openedx-webhooks · 2021-01-27T17:39:54Z

@pdpinch 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2021-01-27T18:39:48Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-01-27T19:34:33Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added core committer open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Dec 28, 2020

pdpinch mentioned this pull request Dec 28, 2020

**PR Template.** As a PR reviewer, I would like contributors to use a PR template, in order to establish standardization and base expectations. openedx/open-edx-proposals#171

Closed

3 tasks

nedbat mentioned this pull request Jan 5, 2021

A draft of Change Transparency openedx/open-edx-proposals#180

Closed

pdpinch force-pushed the pdpinch/pr-template branch from 2b353fd to 5211e5f Compare January 20, 2021 03:47

nasthagiri reviewed Jan 20, 2021

View reviewed changes

nedbat reviewed Jan 21, 2021

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

felipemontoya approved these changes Jan 25, 2021

View reviewed changes

nasthagiri reviewed Jan 26, 2021

View reviewed changes

docs: add PR template

bfeafd4

Co-authored-by: Ned Batchelder <[email protected]>

pdpinch force-pushed the pdpinch/pr-template branch from 7add40c to bfeafd4 Compare January 26, 2021 13:19

nedbat approved these changes Jan 26, 2021

View reviewed changes

nasthagiri approved these changes Jan 26, 2021

View reviewed changes

nedbat merged commit 7dae0b9 into openedx:master Jan 27, 2021

openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 27, 2021


		## Deadline

		"None" if there's no rush, "ASAP" if it's critical, or provide a specific date or event if there is one.

add PR template #25949

add PR template #25949

Conversation

pdpinch commented Dec 28, 2020 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Dec 28, 2020

nasthagiri commented Dec 28, 2020

natabene commented Dec 29, 2020

nedbat commented Dec 29, 2020

nasthagiri commented Dec 29, 2020

nasthagiri commented Dec 29, 2020

nedbat commented Dec 30, 2020

pdpinch commented Dec 30, 2020

pdpinch commented Dec 30, 2020

nedbat commented Jan 4, 2021 • edited Loading

nasthagiri commented Jan 4, 2021

nedbat commented Jan 4, 2021 • edited Loading

nasthagiri commented Jan 4, 2021 • edited Loading

nedbat commented Jan 4, 2021

nedbat commented Jan 19, 2021 • edited Loading

pdpinch commented Jan 20, 2021

nasthagiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasthagiri Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedbat commented Jan 20, 2021

nedbat commented Jan 21, 2021

felipemontoya commented Jan 25, 2021

nasthagiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdpinch commented Jan 26, 2021

edx-status-bot commented Jan 26, 2021

nedbat commented Jan 26, 2021

nedbat commented Jan 27, 2021

openedx-webhooks commented Jan 27, 2021

openedx-webhooks commented Jan 27, 2021

edx-pipeline-bot commented Jan 27, 2021

edx-pipeline-bot commented Jan 27, 2021

pdpinch commented Dec 28, 2020 •

edited

Loading

nedbat commented Jan 4, 2021 •

edited

Loading

nedbat commented Jan 4, 2021 •

edited

Loading

nasthagiri commented Jan 4, 2021 •

edited

Loading

nedbat commented Jan 19, 2021 •

edited

Loading

nasthagiri Jan 20, 2021 •

edited

Loading