PR-Template: Updated the PR template with new checkboxes

[minosgalanakis]

Description

Updates the development branch's PR checkboxes as discussed in PR #9008. Will need to apply for 3.6 and 2.28 braches as well before merging.

[gilles-peskine-arm]

You can see the template live at minosgalanakis/mbedtls@feature/crypto_client_testing_base...minosgalanakis:mbedtls:dev_fix_checkboxes

Will need to apply for 3.6 and 2.28 braches as well before merging.

No: the template is only ever read from the default branch. We can't have different templates based on a pull request's target branch.

[yanesca]

Just a minor typo but other than that it looks good

[yanesca]

LGTM

[mpg]

Note: after merging this, we'll want to merge development to development-restricted (the default branch of the restricted repo) so that the new template is used there as well.

[mpg]

Looks pretty good to me other than a markdown typo.

Just one thing: is the template supposed to be self-sufficient or do we expect to have clarifying documentation elsewhere in addition (like, added to one of our guides about the review process)? For example, about the fact the checking the boxes should be done by the gatekeeper, not the PR author or reviewers - but the PR author should provide justifications that reviewers should review.

I don't have a strong opinion about where it should be documented, but I really think this kind of thing should be put in writing somewhere in (other than comments on #9008 I mean).

[mpg]

Markdown: **2.28 PR**

[mpg]

Not clear to me: if I want to add a justification, where should I add it? Before of after the link?

[minosgalanakis]

Originally the proposal was to have a comments section after the special code.

That way it would be easier to parse as a block of text without affecting the template.

e.g.

- [ ] **dev PR** provided, ~~not required~~ (Justification/Comments). Link --> #XYZ

But I removed it since the comment's suggested that we should not increase the text of the checkboxes.

I am happy either way.

[mpg]

Question (does not have to result in a change in this PR, I just want to make sure I understand): for the development PR, will we add a link to self here?

[gilles-peskine-arm]

Regardless, I expect the tooling to work no matter where the link is on the line, and no matter what the source syntax is (#nnnn or a bare link or a Markdown link).

[minosgalanakis]

@mpg Ideally that should only be needed in backport-prs. So for a development PR if we include the comments that link could be

- [ x ] **dev PR** ~~provided~~, not required. This is the dev PR.  Link --> #XYZ

But as @gilles-peskine-arm the tooling should work even if someone does that

- [ x ] **dev PR** provided, ~~not required~~.  Link --> #SELF_NO

[gilles-peskine-arm]

I've done a first round of review. I would like several changes to make the template focus on useful information and more likely to be followed.

[gilles-peskine-arm]

A single ~ is enough for strikethrough. Please minimize.

[gilles-peskine-arm]

Please minimize: Link --> #XYZ → link: # or maybe even just #. Also the link should be after “provided”, not after “not required”.

[minosgalanakis]

The purpose of the Link --> #XYZ was to make clear to the user what its meant to do. I think link: # would be a good compromise, and after people are confident with it we cna remove the link part.

The tool does not care since it will be looking for markdown http or #{0-9} type of linkage in the PR's body anyway.

I can change the order of items like not required, provided, and note required, covered provided for changelog entries, but I do not think it should matter that much,

[gilles-peskine-arm]

Link is not a common case for the changelog, and should only happen for coordinated work within the team.

Suggested change

	- [ ] **changelog** provided, ~~not required~~, ~~covered~~ . Link --> #XYZ
	- [ ] **changelog** provided, ~~not required~~

[minosgalanakis]

I am with you on that. In unique cases that it is covered by other PR's it can be manually reviewed.

[gilles-peskine-arm]

We should push towards having people indicate why something is not required.

Suggested change

	- [ ] **changelog** provided, ~~not required~~, ~~covered~~ . Link --> #XYZ
	- [ ] **changelog** provided, ~~not required because~~, ~~covered~~ . Link --> #XYZ

[minosgalanakis]

I agree, which is why I have originally proposed a template that had a justifiation after the checkboxes and the linkage.

Saying why something is not where it should be, can be ingored by tooling but is very helpfull for the gatekeeper verifyin the validity of the template.

[yanesca]

I agree that we should indicate somehow that we require justification here. I like ~~not required because~~ better than the original ~~not required~~ (comments) because it is more suggestive of the purpose. Maybe adding a semicolon (~~not required because:~~) would make it more likely that whoever edits them keeps the keywords.

[gilles-peskine-arm]

“provided” for backports doesn't really make sense: there's “this is it”, “[link]” and “not needed because …”.

“This is it” would be better conveyed by omitting the line, but unfortunately we can't have target-branch-dependent templates.

[minosgalanakis]

I would expect this to be filled with not required because this is the backport as discussed above .

And again if someone does a provided and self references, that should also work.

But I would refrain from changing the keywords for each entry, since that comes with a significant cost of maintaining the parsing script.

code like that won't work if each of the lines is unique

checkbox_rex = re.complile(r'....')

... find anchors for start/end of checkboxes

for line in text:
  checkbox_rex .search(line)

[yanesca]

How about something like this: provided: #PR, ~not required because:~

[gilles-peskine-arm]

Tests should always be provided if required. “Covered” and “link” should not be suggested options. This line is useful for two reasons:

• To convey that we do want tests for new features and bug fixes.
• Sometimes, to allow the PR author indicate some validation that isn't just “the CI passes”, for example “[CI link] and [CI link] show the same test cases being executed, which justifies that the test dependency refactoring is correct”

So maybe:

Suggested change

	- **tests** provided, ~~not required~~, ~~covered~~ Link --> #XYZ
	- **tests** (indicate any extra validation that can't be automated)

[minosgalanakis]

To be honest I am a bit torn with Tests and changelog. Yes you are right with the idea of that they should be self contained, and either present or explicitely stated why they are not.

And unlike the linking with backports being on the PR template does not offer much more information than if we used two simple labels such as no-changelog, no-tests to indicate the use-cases that are not required.

I am happy to go whichever direction the team wants.

[yanesca]

In an ideal world, we would always add complete testing with bug fixes and new features. And I agree that we shouldn't encourage adding less testing. However, in practice we have exceptions more often than we would like to: bug fixes where adding tests is prohibitively expensive, new feature development where only limited testing can be added at that point (eg. unit tests or compatibility tests, simply because we don't have enough of the feature implemented to do more, or for practical reasons where we can't keep the PRs small and reviewable in any other way than only adding smoke tests and extensive testing comes in a separate PR) I thought that this line could be useful for providing justification in these cases. As a reviewer or a gatekeeper I would prefer to see some justification as opposed to an empty tests section in the template.

[gilles-peskine-arm]

Regardless, I expect the tooling to work no matter where the link is on the line, and no matter what the source syntax is (#nnnn or a bare link or a Markdown link).