[WIP] Improve clarity/asks in PR template and request additional metadata #81278
Conversation
This comment has been minimized.
This comment has been minimized.
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
kind/documentation
k8s-ci-robot
requested review from
jeefy,
justaugustus,
mrbobbytables,
onyiny-ang and
saschagrunert
k8s-ci-robot
added
size/L
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
Still going over the doc, but just a note for going forward -- It's generally easier to ask questions or pose discussion points as a PR comment and not in the code or doc itself. That way once consensus is reached on a specific point the code block (even if outdated) will remain and provide more information for folk coming in late to the conversation or later when searching. |
There was a problem hiding this comment.
@paulbouwer Thanks for working on this!
Agree with what @mrbobbytables said. Left some comments from a contribex perspective.
| https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests | ||
| --> | ||
|
|
||
| **Which is the sponsoring or primary SIG associated with this PR?** |
There was a problem hiding this comment.
A PR can touch areas that target multiple SIGs and we already have this information with the sig/ labels, so asking to mention the SIGs here would lead to multiple sources of truth 😬
There was a problem hiding this comment.
@paulbouwer for some additional context. Depending on what areas of the codebase are touched by the PR, label(s) will automatically be applied via theOWNERS files. For example, this PR touches content in the .github directory and k8s-ci-robot automatically applied the label sig/contributor-experience
Lines 15 to 16 in f2c82f4
There was a problem hiding this comment.
@nikhita - this was specifically targeting the primary sig based on the request in issue #668. @onyiny-ang - it may be worth discussing this in more detail. Looking at the release-notes-draft.md, I'm not sure I can see the reason for the primary sig collection.
@mrbobbytables - I have been wondering how that aspect of the label magic worked. Thanks 👍
There was a problem hiding this comment.
Sorry in advance because this will be a bit long winded, but from the release notes standpoint, we were hoping to collect the singular SIG that is primarily responsible for the change the PR brings so that when we are sorting release notes (or ideally programming the release notes tool to collect release notes for a release) we are able to easily identify which SIG the note should be filed under. Having multiple SIG labels helps identify all areas that a PR may impact or contribute changes to and the release notes website makes this easier for users to digest.
However, for the release notes draft, the relnotes tool sorts each note that is tagged with multiple SIG labels to buckets created for that note (ie. there will be a SIG API Machinery bucket for PRs with only that SIG label as well as a bucket for all notes labelled with both SIG API Machinery and SIG Cluster Lifecycle) and this makes things confusing for the relnotes team and contributors trying to find all of the pertinent notes associated with what their SIG has accomplished/changed for a release. For the relnotes team, which may be fairly new contributors, it can be impossible to determine which SIG to sort the PR under without reaching out to every contributor who submitted a PR tagged with multiple labels.
As we're still generating the human curated notes, it would greatly help the relnotes team and contributors who have to look through the release notes draft if we were able to identify the SIG that the release note should be filed under automatically. This was what I had in mind when I was requesting the "primary-sig" field to be added. BUT I'm certainly open to better ideas!
There was a problem hiding this comment.
It seems like this is a non-issue for the release notes website. It is just the markdown release notes draft where this is an issue. And given how many of the current release notes fall into multiple sigs, I can see how this can create a lot of manual work.
|
|
||
| **What type of PR is this?** | ||
| > Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line: |
There was a problem hiding this comment.
I would prefer to keep the "uncomment" format since it's easier than typing it out completely.
There was a problem hiding this comment.
I'll revert and add the "uncomment" text back in. Can a PR specify multiple "kinds"? Does this make sense. If so, should I remove the language around only selecting one?
There was a problem hiding this comment.
Can a PR specify multiple "kinds"?
Generally, most PRs have a single "kind" but it is possible to have multiple "kinds" too -- a PR can have kind/feature and kind/api-change.
If so, should I remove the language around only selecting one?
No strong opinions on this one. I think it should be fine to remove the language around selecting one, but a thing to note here is that we need at least one "kind" or else the PR will get the needs-kind label and won't get merged. 😬
There was a problem hiding this comment.
@nikhita we were primarily hoping for something to signify a deprecation. Whether a kind/deprecation label or just a deprecation label. If kind/deprecation is used, it would make sense to have more than one kind--but this may mean that it is more confusing for contributors. Perhaps just a label would be best.
It may also be useful to have a metrics-change label . . .
We have changed the release notes template significantly over the past couple of releases with deprecations and metrics as well as urgent upgrade notes being added. Since this is becoming the "way we do relnotes by hand" some tooling to support further automation around these headings would prevent a scramble just prior to the release and reduce the strain on the relnotes team/contributors who consistently help out 😄
There was a problem hiding this comment.
@nikhita The use of blockquote here and the requirement for having to hit enter and remove leading whitespace felt complicated. I wish markdown had a line comment character that we could use here.
I have updated the PR by using the ~~ strikethrough wrapping characters. It is less visually busy in the rendered version. Thoughts? At this point, after playing with a number of options to improve this, I'm at a loss. So if the strikeththrough option is not appealing to everyone, I'll move it back to blockquotes.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| <!-- | ||
| Provide a description on what this PR does and/or why it is required. | ||
| --> | ||
| Description here ... |
There was a problem hiding this comment.
I feel like *What this PR does / why we need it is self-explanatory and additional comments are not needed for this one...
There was a problem hiding this comment.
Agree - I went back and forth on this one. I was fighting consistency vs redundancy in my head. I'll remove the comment here.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| If the PR is about failing-tests or flakes, please post the related issues/tests in a comment and do not use the Fixes templates above. | ||
| --> | ||
| Fixes #??? |
There was a problem hiding this comment.
Would prefer to keep this as-is because adding ??? would require the author to remove them first 😬
There was a problem hiding this comment.
💯 In general, the less people have to change / fill out. The more likely they are to use it appropriately. This applies to the other areas below as well.
There was a problem hiding this comment.
Agree on minimising work for folks. Not all PRs fix an issue, they may reference one or more issues, or reference an issue in another repo. Should the Fixes # text not be removed then too, and the comments section guide a PR submitter on how to format.
Or do the overwhelming majority of PRs relate to fixing an issue, which would then make sense to keep the Fixes # text in, minus the ???.
There was a problem hiding this comment.
Good point, @paulbouwer!
While not all PRs might "fix" an issue, I'd prefer to keep Fixes # because I've seen some cases where people forget to add this line and the issue needs to explicitly closed later. However, in case anyone has data around cases where this has led to issues being closed unexpectedly, we can consider removing it. :)
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| --> | ||
| Fixes #??? | ||
|
|
||
| **Have you added or run tests for your PR?** |
There was a problem hiding this comment.
Can you elaborate on why this is needed? There are lots of PRs that might not need tests :)
There was a problem hiding this comment.
Perhaps something more generic like "Has consideration been given to a test plan related to this PR? If appropriate, were tests added/modified to cover this PR?"
I definitely like having a prompt regarding quality assurance.
There was a problem hiding this comment.
There was an original comment in the header of the PR template:
Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
I was moving comments around to place them closer to where they had the most context. There was nothing asking the question about tests, so I added a new section. @tpepper - I like your suggestion. It is closer to what I had in my head for this section. @nikhita - if you are ok with Tim's suggestion, I'll update this section with it.
There was a problem hiding this comment.
Has consideration been given to a test plan related to this PR? If appropriate, were tests added/modified to cover this PR?
Maybe I'm missing something here, but how would this help us in signaling if tests were added? To elaborate -- I see all the other prompts in the issue templates as "taking some action" depending on the authors' response. For example, applying the release note label or closing an issue...
I feel like the comment in the PR header gives the "fyi, it'd be nicer if you added tests" kind of signal right now, which IMHO is good enough. Wdyt?
There was a problem hiding this comment.
Should we consider an additional signal around tests for a PR? I assume if so, this would take the form of another label.
My concern would be as to how useful that signal would be. It would be useful if we want to understand how many PRs that should be accompanied by tests are not. But would not be useful for a PR that doesn't require tests and doesn't have a label - how would you differentiate that from a PR that requires tests and is missing them.
If, after more discussion, there is a requirement for a signal around test coverage for PRs, then we should reconsider. For now, I'll move this back to the header comment, but with the updated text suggested by @tpepper.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| <!-- | ||
| Provide any additional notes that may be helpful to the PR reviewers. | ||
| --> | ||
| None |
There was a problem hiding this comment.
Would prefer to leave this as-is because adding a "special note" would first mean removing the None. Overall, just want to reduce the amount of work that the author would need to do :)
There was a problem hiding this comment.
I thought it would make it clearer when reading the PR - so seeing None instead of a blank line, but I understand the need to reduce any additional overhead for the PR submitter. I'll remove this.
| --> | ||
| ```release-note | ||
|
|
||
| None |
There was a problem hiding this comment.
I'd prefer not to default this because we want people to consciously apply the release note. An empty block will lead to do-not-merge/release-note-label-needed and will prevent merge but defaulting can automatically merge a PR in case someone forgot to mention the release note.
There was a problem hiding this comment.
I'd prefer not to default this because we want people to consciously apply the release note. An empty block will lead to
do-not-merge/release-note-label-neededand will prevent merge but defaulting can automatically merge a PR in case someone forgot to mention the release note.
Very much this ^^^
There was a problem hiding this comment.
I added this in since there are a large number of release notes with variations of none, n/a etc. I wanted the default to help those folks. But I totally see both of your concerns now. I'm slowly building up an understanding of the inter-dependent bits here.
I'll remove the default and adjust the comment appropriately.
|
/cc @cblecker |
| https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests | ||
| --> | ||
|
|
||
| **Which is the sponsoring or primary SIG associated with this PR?** |
There was a problem hiding this comment.
@paulbouwer for some additional context. Depending on what areas of the codebase are touched by the PR, label(s) will automatically be applied via theOWNERS files. For example, this PR touches content in the .github directory and k8s-ci-robot automatically applied the label sig/contributor-experience
Lines 15 to 16 in f2c82f4
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| > /kind failing-test | ||
| > /kind feature | ||
| > /kind flake | ||
| > Should we include a kind type of `deprecation`? And should we then include a `` ```depracation-notes`` code block to provide specific detailed notes around the depracation. |
There was a problem hiding this comment.
I do like the idea of better signaling deprecations or end user ACTION REQUIRED as we have in the user facing changes section. I don't know if it needs its own /kind but if there was an easy way for me as an end user to see all changes that might require them to take action on the rel notes site, that would be immensely useful.
There was a problem hiding this comment.
cc @kubernetes/sig-release @justaugustus
There was a problem hiding this comment.
I guess I should have commented https://github.com/kubernetes/kubernetes/pull/81278/files#r314175841 here 😂
As I said above, I would argue for deprecation to have a label. @jeefy may also have some thoughts on this.
I want to say yes to deprecation-notes too. . .but am also concerned about the PR template becoming cumbersome and intimidating to new contributors.
There was a problem hiding this comment.
I'll add a deprecation-notes block for now, and we can debate the deprecation label. A deprecation label applied on the presence of a non-empty deprecation-notes block would be great for signal and the contents of the deprecation-notes block could be highlighted in the various projections/renderings of the release notes metadata and information.
There was a problem hiding this comment.
I've added a default of None here since this would be the majority case and minimises impact on those contributors.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| If the PR is about failing-tests or flakes, please post the related issues/tests in a comment and do not use the Fixes templates above. | ||
| --> | ||
| Fixes #??? |
There was a problem hiding this comment.
💯 In general, the less people have to change / fill out. The more likely they are to use it appropriately. This applies to the other areas below as well.
|
@mrbobbytables, @nikhita - agree with you both on the comments in the template feedback. Was a bit of a brain fade from me. I'll remove comments from the template and add as comments against code blocks here. |
@paulbouwer no worries! Thanks a lot for working on this again! :) |
|
Hi everyone! I am very much in favor of helping contributors on a short and easy path to success. This PR aims at doing that, but for the most part, it aims to fill in metadata, and I think that's a pretty big ask of a contributor, especially an earnest, new, casual contributor who found a bug for their use case and would like to fix it (but will find an internal workaround, and not improve core Kubernetes if it's too annoying to deal with upstream procedures). I am looking at the raw file content for this template, and...it looks long, and complicated. If I'm not familiar with the community practices, I will see a lot of hoops to jump through, and that's before I even look for reviewers, and shepherd my PR. TL;DR: I would like to avoid a wall of text, but I realize that may not be possible. Many apologies for the derail - I'm also happy for others to put my fears to rest; there's just something about this much text just to open a PR that gets my anxiety going 😅 Regardless, per kubernetes/sig-release#729 we definitely need to update the release notes doc |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| > | ||
| > Should we use the above pattern and update the label prow plugin ? https://github.com/kubernetes/test-infra/tree/master/prow/plugins/label. Logic would have to ensure that there is only a single primary sig associated. This metadata can be leveraged by other processes outside of the release notes. | ||
| > | ||
| > or, if this is ONLY relevant to release notes then, we can add here as a `` ```primary-sig`` code block. |
There was a problem hiding this comment.
Yes. This is only relevant for release notes.
With this field filled in, we can adust the relnotes tool to collect the primary SIG so that the relnote associated with the PR can be sorted as the PR author intended (saving a lot of stress and time for the relnotes team and contributors frantically editing the relnotes in the days just prior to the release 😂 ). All SIG labels would remain as before and would be searchable via relnotes.k8s.io
There was a problem hiding this comment.
So I'll leave the /sig-primary section in for now, and start looking at all the dependencies - like the label prow plugin.
I'll update the PR comments with dependencies based on some final decisions we make.
This aims to address [kubernetes#729](kubernetes/sig-release#729) and provide details to users for the finalized version of [#81278](#kubernetes/kubernetes#81278) as well as some other changes that have already taken place. Still very much incomplete and a WIP. /cc @saschagrunert @paulbouwer @cartyc @kcmartin @guineveresaenger
|
(By the way, didn't mean to put the ominous hold on this without explaining why. I'm going to wait on reviewing this until #81159 merges.) |
cblecker
added
priority/backlog
|
/check-cla |
|
|
||
| **Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.**: |
There was a problem hiding this comment.
I would not change this whole section again in this PR to move faster forward.
|
@paulbouwer can we continue with that one that we're able to finish it until the end of the next week? Otherwise we would have to carry it over to the next release cycle. |
paulbouwer
force-pushed
the
668-improve-PR-template
branch
from
c3b6ecd
to
f729377
Compare
|
@paulbouwer: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@justaugustus @jeefy @cblecker @guineveresaenger @saschagrunert @onyiny-ang I'm going to close this PR, but leave my branch so that we can refer back to this conversation and content. @saschagrunert and I have agreed that there are too many new bits in this PR and the resultant discussions are blocking progress of some of the simpler decisions. I'll create some simpler PRs that focus on individual changes we are proposing. Hopefully this will allow us to move forward. We can spin up some of the topics that require more conversation in an issue where everyone can comment and debate. |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
This PR attempts the following:
Which issue(s) this PR fixes:
Refers to Issue kubernetes/sig-release#668
References PR kubernetes/kubernetes#81159
Special notes for your reviewer:
This requires further discussion. There are sections in the
PULL_REQUEST_TEMPLATE.mdfile marked asking for a discussion.@saschagrunert @jeefy - I have taken the changes in kubernetes/kubernetes#81159, reworked text based on @jeefy's comments, and rolled into the changes here.
This PR Template will require associated changes in the prow label plugin and release notes tool depending on the outcome of the discussion.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @saschagrunert @onyiny-ang @justaugustus @jeefy @guineveresaenger @mrbobbytables
/sig contributor-experience
/sig release