Skip to content
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.

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

Update pull request template #13947

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

colebow
Copy link
Member

@colebow colebow commented Aug 31, 2022

Description

  • Getting rid of the various descriptive questions and consolidating them into a "non-technical explanation" header.
  • Moving the comments with instructions to above the headings and adding sufficient whitespace between them, so that it's much clearer and more obvious as to where a PR author needs to fill in the template.
  • Getting rid of the docs question. This was often a no-signal question, as many contributors weren't clear on what constitutes documentation, and they would mark "sufficient docs included" thinking that commented code counted as docs. It was pretty misleading, and I'd rather get rid of it just to get rid of any possible confusion. I think it'll be easier and less error-prone to check the descriptions, conclude whether or not there should be docs, and check the code changes to see if we have any .rst changes, then follow-up as needed from there.
  • Adding reasons why release notes may not be necessary, as well as adding a box to ask for the maintainer/release manager to handle the RN for the change. I think this'll encourage people to think a little more critically about release notes, and it's trying to discourage saying "no release notes required" as a means of avoiding having to think about them. Open to feedback on this, I think it's where this change is the most spicy.

If we're changing the template to this, we also need to be sure that it is being filled out by everyone, always. There are three touchpoints, the first one is the most important as the broad description that helps with a review, and the other two can be very brief, especially for technical changes that aren't user-facing and shouldn't require much effort. "Just tests" or "refactor" and a single (x) will handle test changes/refactors for those last two questions, and it'd be nice to see that done for every PR with no exceptions. This change shortens it from 1-2 minutes of scrolling and trying to find the right places to type to an extremely quick and trivial process, and it should be low-friction enough for it to be used by everyone.

Alternate approach to #13916.

I would also want to accompany this change by adding #13916 (comment) to the Trino project.

Non-technical explanation

Updating the PR template on GitHub! I'm even using the new template here.

Release notes

(x) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Aug 31, 2022
@colebow colebow marked this pull request as ready for review August 31, 2022 19:32
@colebow colebow requested a review from losipiuk August 31, 2022 19:32
@colebow colebow force-pushed the colebow/github-pr-template branch 2 times, most recently from eaa4663 to 0566c91 Compare August 31, 2022 20:23
Comment on lines 7 to 8
<!--Provide a user-friendly explanation, keep it brief if it isn't user-visible.-->
## Non-technical explanation
Copy link
Member

Choose a reason for hiding this comment

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

Can you share an example of what you think you would like to see here? I can't see the utility of this over the description above.

Copy link
Member Author

@colebow colebow Sep 1, 2022

Choose a reason for hiding this comment

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

I think for a lot of things, this is going to simply be "Refactor" or "Update tests" or "Upgrade dependency version" or something along those lines, or otherwise a simple restatement of a description which is easily understood from a non-technical perspective.

As an arbitrary example where it would be helpful, on #13736, it would be useful to say something like "make sure Trino stops running if it can't secure sufficient resources" in this section, because a less-technical user won't understand what jvmkill is or why terminating the jvm matters.

The section admittedly won't provide a ton of value most of the time, but when it does, it'll help a lot with making sure release notes make sense in situations where the PR author wouldn't otherwise provide a ton of detail as to why users should care about a change.

@colebow colebow force-pushed the colebow/github-pr-template branch from 0566c91 to bfbab99 Compare September 2, 2022 08:37
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

Definitely a step in the right direction.

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:
( ) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
Copy link
Member

Choose a reason for hiding this comment

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

is it intentionally not using checkboxes (* [ ])?

checkboxes would allow the option to be marked with a click.

Copy link
Member Author

@colebow colebow Sep 2, 2022

Choose a reason for hiding this comment

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

I think @mosabua told me at one point that that something in Trino's Github automation expects all of them to be ticked before a PR is merged? I've kind of just gone off of that, and I assumed it was previously made to be parentheses for a reason.

Copy link
Member

Choose a reason for hiding this comment

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

Correct.. if you use [] it counts as a build action..

Copy link
Member

@findepi findepi Sep 5, 2022

Choose a reason for hiding this comment

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

I edited description of #1394 #13941 so it includes checkboxes.
@colebow @mosabua where can i see the consequences of the fact these checkboxes are not checked yet?

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@colebow colebow force-pushed the colebow/github-pr-template branch from bfbab99 to 639d99b Compare September 2, 2022 16:36
@findepi findepi merged commit 8d41253 into trinodb:master Sep 6, 2022
@findepi
Copy link
Member

findepi commented Sep 6, 2022

@colebow can you please address #13947 (comment) in a followup?

@findepi
Copy link
Member

findepi commented Sep 6, 2022

btw many thanks for simplifying this!

@github-actions github-actions bot added this to the 395 milestone Sep 6, 2022
@colebow colebow deleted the colebow/github-pr-template branch September 8, 2022 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants