HIP: Enhance Helm Pull Request Lifecycle Policy#420
Conversation
Signed-off-by: Scott Rigby <scott@r6by.com>
| 34. `in progress` → GitHub draft PR status handles this | ||
| 35. `keep open` → automation can handle this | ||
| 36. `needs-pick` → release automation handles this | ||
| 37. `oci` → use title prefixes instead |
There was a problem hiding this comment.
Isn't more a "scope" ? Like feat(oci): or bug(sdk):
There was a problem hiding this comment.
This is what I meant by title prefixes 👍
There is a convention some projects use like area/AREA - we have only one label following that syntax, area/helm-test. But other labels like oci could perhaps follow this. At the moment I lean toward just using prefixing in the PR title like you said.
There was a problem hiding this comment.
maybe going with conventional commits language could be good, eg using scope/oci, scope/docs etc. I'm on the fence about this because often a PR touches more than one of these kinds of areas/scopes, and it seems to be a judgement call about whether to add this and if so what scope should be.
banjoh
left a comment
There was a problem hiding this comment.
Is the intention to rely on Conventional Commits as well when automating processes such as generating release notes?
|
|
||
| ### Conventional Commits Integration | ||
|
|
||
| #### Required PR Title Format |
There was a problem hiding this comment.
I'm finding this list quite long to remember. When someone is making a PR, they'll tend to choose from a few of these, meaning that some will never be used.
| - `build: description` for build system changes | ||
| - `ci: description` for CI/CD changes |
There was a problem hiding this comment.
I would bundle ci and build into one. Maybe ci. For changes related to building locally
| ### Policy Implementation | ||
|
|
||
| #### Triage Phase | ||
| - Triager or core maintainers update PR title to follow Conventional Commits format |
There was a problem hiding this comment.
Why not fail the build and expect the author to add the necessary categorisation. I'm leaning towards just using labels and not relying on conventional commits
There was a problem hiding this comment.
+1 for avoiding maintainers to have to perform activities for each PR
There was a problem hiding this comment.
I agree. Yes this HIP proposes to fail the check until required labels and milestones are correctly added. I only meant that maintainers have the ability to change the label category if we feel the PR was incorrectly categorized by the PR author.
This is similar to what we do now, but at the moment it's 100% up to maintainers to add labels – and often, we do not even do this (the problem is somewhat masked because I have on several occasions gone back and retroactively labeled hundreds of PRs that were previously merged without them). But with this HIP, more of this responsibility will be on PR authors and a lot of the rest can be automated. We just still have a manual safety option to change what was automated if we feel it was done incorrectly.
I will update the HIP to make sure this is more clear
| ### Policy Implementation | ||
|
|
||
| #### Triage Phase | ||
| - Triager or core maintainers update PR title to follow Conventional Commits format |
There was a problem hiding this comment.
+1 for avoiding maintainers to have to perform activities for each PR
|
|
||
| ## How to Teach This | ||
|
|
||
| 1. Update CONTRIBUTING.md with Conventional Commits requirements |
There was a problem hiding this comment.
Should we also add content to the website?
There was a problem hiding this comment.
good idea. I will update the HIP with this
TerryHowe
left a comment
There was a problem hiding this comment.
I've looked at this a couple times and while I think there are a lot of good ideas here, I think it would be impossible to follow these rules. My approval definitely does not mean I will actually do this, it just means I generally agree.
I think the review process should be documented in the repo and referenced in AGENTS.md and README.md and linked from helm.sh. In the repo so the robots can follow it.
I need a check list to follow
- Validate title
- Apply type label
- ...
Which would be followed by a section of what the list item means:
# Validate Title
blah blah
# Label Type
blah blah
| ```javascript | ||
| // Example script for generating release notes from PR labels | ||
| const { Octokit } = require("@octokit/rest"); | ||
|
|
||
| async function generateReleaseNotes(milestone) { | ||
| const octokit = new Octokit({ auth: process.env.GITHUB_TOKEN }); | ||
|
|
||
| const prs = await octokit.rest.pulls.list({ | ||
| owner: 'helm', | ||
| repo: 'helm', | ||
| state: 'closed', | ||
| base: 'main' | ||
| }); | ||
|
|
||
| const sections = { | ||
| 'enhancement': '### Features\n', | ||
| 'bug': '### Bug Fixes\n', | ||
| 'documentation': '### Documentation\n', | ||
| 'refactor': '### Refactoring\n', | ||
| 'dependency': '### Dependencies\n' | ||
| }; | ||
|
|
||
| prs.data | ||
| .filter(pr => pr.milestone?.title === milestone) | ||
| .filter(pr => !pr.labels.find(l => l.name === 'no-milestone')) | ||
| .forEach(pr => { | ||
| const typeLabel = pr.labels.find(l => sections[l.name]); | ||
| if (typeLabel) { | ||
| sections[typeLabel.name] += `- ${pr.title} (#${pr.number})\n`; | ||
| } | ||
| }); | ||
|
|
||
| return Object.values(sections).join('\n'); | ||
| } |
There was a problem hiding this comment.
This makes the PR title and label both normative, which creates two sources of truth. I think the HIP should say explicitly which one is authoritative when they disagree, otherwise the automation and maintainer workflow will be ambiguous
There was a problem hiding this comment.
I'm not seeing how this code makes the PR title normative, since it is only included in the output and is not used in the logic.
But, I concur that we should have only one source of truth. However, that truth doesn't have to be available until after the CI has blessed the PR, and we can make agreement between the title and the labels a requirement for that blessing, which means that we can trust the labels in the end (we cannot trust the title because, as the HIP points out, the author can change it after the fact).
So, I think the labels should be the source of truth, not the title.
|
I'm sure this is your intention, but I'll say it anyway: I think the label naming needs to be made consistent throughout the document. One section says needs backport, while later sections use needs-backport. I’d pick one form and use it everywhere, including the examples. |
|
Should we reference the actual standard somewhere in the doc? https://www.conventionalcommits.org/en/v1.0.0/ |
| #### Release Notes Generation | ||
| - Generate changelog sections automatically from type labels | ||
| - Group changes by type (Features, Bug Fixes, Documentation, etc.) | ||
| - Exclude PRs with `no-milestone` label from release notes |
There was a problem hiding this comment.
Another thing I've seen done, and done myself, is to add a section to the PR template with a user-facing-docs codeblock that can be included with the commit message in the release notes.
| - PR authors can modify titles without maintainer oversight | ||
| - Labels provide better programmatic access for automation | ||
| - Maintainers need final categorization authority |
There was a problem hiding this comment.
The justifications for rejecting this idea seems to rest on a broader policy choice that maintainers, not authors, have final authority over PR categorization. If that is the intent, it may be worth stating it directly in the main proposal, since several later requirements appear to depend on it
|
If we had a Helm skills repo, this would be the perfect kind of thing to add a skill for. When I first reviewed this way back I was thinking I'd never be able to keep all these rules in my head, but as a skill it would be easy. |
webbnh
left a comment
There was a problem hiding this comment.
Also, we were going to consider requiring PRs to address an open Issue.
| #### Release Notes Generation | ||
| - Generate changelog sections automatically from type labels | ||
| - Group changes by type (Features, Bug Fixes, Documentation, etc.) | ||
| - Exclude PRs with `no-milestone` label from release notes |
There was a problem hiding this comment.
I think we are conflating (at least) two concepts here: we're using no-milestone to mean "no release note".
Just because a given change isn't intrinsically tied to a milestone does not in itself mean that it should not be release-noted. Arguably, any change shipped in a particular release should have a note, regardless of which release it was originally intended for.
At the same time, I can imagine how we might want the ability to suppress a release note for a given change (e.g., because it targets the CI or something else which is not user-facing).
So, it seems like a label which is used to suppress release notes should be independent of (and orthogonal to) the milestone labeling.
So, while it's fine to have a label which when set means "we intentionally left the 'milestone' field blank", but we should have a separate label (e.g., no-release-note) which suppresses the release note for the change.
| #### Automated Checks | ||
| - Validate PR title format against Conventional Commits syntax | ||
| - Ensure `needs backport` and `enhancement` labels are mutually exclusive |
There was a problem hiding this comment.
It will probably never come up, but are we sure that there will never be an enhancement that we want to backport? (I see the dichotomy between bug and enhancement (and I see how refactor could/should be orthogonal to each of those), but I can also imagine any of those being potential backport candidates.)
PR preview: https://github.com/scottrigby/community/blob/hip-update-pr-policy/hips/hip-9999.md