-
Notifications
You must be signed in to change notification settings - Fork 206
HIP: Enhance Helm Pull Request Lifecycle Policy #420
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,321 @@ | ||
| --- | ||
| hip: 9999 | ||
| title: "Enhance Helm Pull Request Lifecycle Policy" | ||
| authors: ["Scott Rigby <[email protected]>"] | ||
| created: "2025-11-05" | ||
| type: "process" | ||
| status: "draft" | ||
| --- | ||
|
|
||
| ## Abstract | ||
|
|
||
| This proposal enhances the Helm pull request lifecycle policy to improve organization and automation of PR triaging, review, changelog generation, and documentation. The proposal introduces a standardized system based on Conventional Commits syntax for PR titles and corresponding labels, establishing clear criteria for review-readiness and merge-readiness while automating release note generation. | ||
|
|
||
| ## Motivation | ||
|
|
||
| The current PR lifecycle lacks standardized categorization and automation, making it difficult to: | ||
| - Generate automated release notes from merged PRs | ||
| - Clearly communicate value to end users when browsing PRs | ||
| - Maintain consistent documentation updates | ||
| - Track when changes will land in specific releases | ||
| - Efficiently triage and organize PRs for review | ||
|
|
||
| ## Rationale | ||
|
|
||
| Adopting Conventional Commits syntax for both PR titles and labels provides a standardized approach that enables automation while maintaining human readability. This system bridges the gap between commit messages and GitHub's PR management features, allowing for reversible categorization decisions and improved maintainer workflow efficiency. | ||
|
|
||
| ## Specification | ||
|
|
||
| ### PR Lifecycle Phases | ||
|
|
||
| The enhanced lifecycle follows this structure: | ||
| 1. **Triage** - Initial categorization and labeling | ||
| 2. **Review Readiness Evaluation** - Ensuring PR meets review criteria | ||
| 3. **Ready for Review** - Label applied when review-ready | ||
| 4. **Maintainer Review** - Standard review process | ||
| 5. **Merge Readiness Evaluation** - Final checks before merge | ||
| 6. **Merge** - Integration with proper categorization | ||
|
|
||
| ### Conventional Commits Integration | ||
|
|
||
| #### Required PR Title Format | ||
| PR titles must follow Conventional Commits syntax: | ||
| - `fix: description` for bug fixes | ||
| - `feat: description` for new features | ||
| - `docs: description` for documentation changes | ||
| - `refactor: description` for code refactoring | ||
| - `perf: description` for performance improvements | ||
| - `test: description` for test additions/changes | ||
| - `build: description` for build system changes | ||
| - `ci: description` for CI/CD changes | ||
|
Comment on lines
+49
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would bundle |
||
| - `chore: description` for maintenance tasks | ||
| - `style: description` for formatting changes | ||
|
|
||
| #### Corresponding Label System | ||
| Each PR must have exactly one primary type label matching the title prefix: | ||
| - `bug` (for fix: titles) | ||
| - `enhancement` (for feat: titles) | ||
| - `documentation` (for docs: titles) | ||
| - `refactor` (for refactor: titles) | ||
| - `dependency` (for dependency updates, may use chore: or build: titles) | ||
|
|
||
| #### Additional Labels | ||
|
|
||
| **Helm 4 Specific:** | ||
| - `backported` - Change was backported from v4 to v3 | ||
| - `needs backport` - Cannot be used with `enhancement` label | ||
|
|
||
| **Milestone Management:** | ||
| - `no-milestone` - For PRs that do not affect any Helm release milestones | ||
|
|
||
| ### Policy Implementation | ||
|
|
||
| #### Triage Phase | ||
| - Triager or core maintainers update PR title to follow Conventional Commits format | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for avoiding maintainers to have to perform activities for each PR |
||
| - Add corresponding primary type label | ||
| - Add milestone or `no-milestone` label | ||
|
|
||
| #### Review Readiness | ||
| PR titles and labels should be finalized during review readiness evaluation, ensuring: | ||
| - Title follows Conventional Commits syntax | ||
| - Appropriate type label is applied | ||
| - Milestone assignment is complete | ||
|
|
||
| #### Pre-Merge Checklist | ||
| Before merging, verify: | ||
| - [ ] PR title follows Conventional Commits format | ||
| - [ ] Corresponding type label is applied | ||
| - [ ] Milestone or `no-milestone` label is assigned | ||
| - [ ] `needs backport` and `enhancement` labels are not both present | ||
|
|
||
| ### Automation Opportunities | ||
|
|
||
| #### Automated Checks | ||
| - Validate PR title format against Conventional Commits syntax | ||
| - Ensure `needs backport` and `enhancement` labels are mutually exclusive | ||
| - Verify milestone assignment (either milestone or `no-milestone` label) | ||
|
|
||
| #### 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 | ||
|
|
||
| ## Backwards Compatibility | ||
|
|
||
| This policy enhancement is fully backwards compatible. Existing PRs can be retroactively categorized using GitHub queries to identify unlabeled PRs and apply appropriate labels. | ||
|
|
||
| ### Migration Strategy | ||
|
|
||
| **Identify Uncategorized PRs:** | ||
| ``` | ||
| is:pr is:merged -label:"bug" -label:"enhancement" -label:"documentation" -label:"refactor" -label:"dependency" | ||
| ``` | ||
|
|
||
| **Identify PRs Without Milestone Assignment:** | ||
| ``` | ||
| is:pr is:merged no:milestone -label:no-milestone | ||
| ``` | ||
|
|
||
| **Label Migration Process:** | ||
| Before implementing the new labeling system, conduct an evaluation and migration of the existing 47 labels: | ||
| 1. Audit current label usage and identify overlapping or outdated labels | ||
| 2. Create mapping from existing labels to new primary type labels | ||
| 3. Standardize label naming convention (lowercase with hyphens) | ||
| 4. Consolidate similar labels to reduce confusion | ||
| 5. Migrate existing PRs to use new label taxonomy | ||
| 6. Archive or remove deprecated labels after migration | ||
|
|
||
| **Example Label Mapping:** | ||
|
|
||
| *Complete list of all 47 existing labels:* | ||
|
|
||
| **Map to new primary type labels:** | ||
|
|
||
| 1. `bug` → `bug` (keep) | ||
| 2. `feature` → `enhancement` (rename) | ||
| 3. `docs` → `documentation` (rename) | ||
| 4. `dependencies` → `dependency` (rename) | ||
| 5. `refactor` → `refactor` (keep) | ||
|
|
||
| **Keep as supplementary labels:** | ||
|
|
||
| 6. `awaiting-review` (rename from `awaiting review`) | ||
| 7. `blocked` (workflow state) | ||
| 8. `breaking` (important for release planning) | ||
| 9. `docs-needed` (rename from `docs needed`) | ||
| 10. `dont-backport` (backport policy) | ||
| 11. `good-first-issue` (rename from `good first issue`) | ||
| 12. `has-one-approval` (rename from `Has One Approval`) | ||
| 13. `help-wanted` (rename from `help wanted`) | ||
| 14. `needs-rebase` (workflow state) | ||
| 15. `needs-test` (workflow state) | ||
| 16. `needs-backport` (rename from `Needs v3 backport`) | ||
| 17. `release-note-required` (rename from `release note required`) | ||
| 18. `size/l` (rename from `size/L`) | ||
| 19. `size/m` (rename from `size/M`) | ||
| 20. `size/s` (rename from `size/S`) | ||
| 21. `size/xl` (rename from `size/XL`) | ||
| 22. `size/xs` (rename from `size/XS`) | ||
| 23. `size/xxl` (rename from `size/XXL`) | ||
| 24. `v3.x` (version targeting) | ||
| 25. `v4.x` (version targeting) | ||
| 26. `v5.x` (version targeting) | ||
|
|
||
| **Consider for removal or consolidation:** | ||
|
|
||
| 27. `approved` → GitHub approval process handles this | ||
| 28. `area/helm-test` → consolidate into `test` type | ||
| 29. `cncf-cla: no` → deprecated, using DCO instead | ||
| 30. `cncf-cla: yes` → deprecated, using DCO instead | ||
| 31. `github_actions` → use title prefixes instead | ||
| 32. `go` → use title prefixes instead | ||
| 33. `lgtm` → GitHub approval process handles this | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't more a "scope" ? Like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I meant by title prefixes 👍 There is a convention some projects use like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe going with conventional commits language could be good, eg using |
||
| 38. `picked` → release automation handles this | ||
| 39. `plugin` → use title prefixes instead | ||
| 40. `proposal` → use GitHub issue templates instead | ||
| 41. `question/support` → use GitHub issue templates instead | ||
| 42. `Stale` → automation can handle this | ||
| 43. `unconfirmed` → issue management labels | ||
| 44. `v2.x` → historical, can archive | ||
| 45. `v3 port complete` → historical, can archive | ||
| 46. `wip` → GitHub draft PR status handles this | ||
| 47. `wont fix` → issue management labels | ||
|
|
||
| ## Security Implications | ||
|
|
||
| This proposal does not introduce security risks. The enhanced labeling and categorization system improves transparency in the development process. | ||
|
|
||
| ## How to Teach This | ||
|
|
||
| 1. Update CONTRIBUTING.md with Conventional Commits requirements | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also add content to the website? |
||
| 2. Add PR template suggesting appropriate title format | ||
| 3. Document the new labeling system in maintainer onboarding | ||
| 4. Implement automated checks with helpful error messages | ||
|
|
||
| ## Reference Implementation | ||
|
|
||
| ### GitHub Actions Workflow | ||
|
|
||
| ```yaml | ||
| name: PR Validation | ||
| on: | ||
| pull_request: | ||
| types: [opened, edited, synchronize] | ||
|
|
||
| jobs: | ||
| validate: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Validate PR Title | ||
| uses: amannn/action-semantic-pull-request@v5 | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| types: | | ||
| fix | ||
| feat | ||
| docs | ||
| style | ||
| refactor | ||
| perf | ||
| test | ||
| build | ||
| ci | ||
| chore | ||
| requireScope: false | ||
|
|
||
| - name: Check Label Consistency | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const title = context.payload.pull_request.title; | ||
| const labels = context.payload.pull_request.labels.map(l => l.name); | ||
| const titleType = title.split(':')[0]; | ||
|
|
||
| const typeMapping = { | ||
| 'fix': 'bug', | ||
| 'feat': 'enhancement', | ||
| 'docs': 'documentation', | ||
| 'refactor': 'refactor', | ||
| 'chore': 'dependency', | ||
| 'build': 'dependency' | ||
| }; | ||
|
|
||
| const expectedLabel = typeMapping[titleType]; | ||
| if (expectedLabel && !labels.includes(expectedLabel)) { | ||
| core.setFailed(`PR title type "${titleType}" requires label "${expectedLabel}"`); | ||
| } | ||
| ``` | ||
|
|
||
| ### Release Notes Automation | ||
|
|
||
| ```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'); | ||
| } | ||
| ``` | ||
|
|
||
| ## Rejected Ideas | ||
|
|
||
| ### Commit Message Only Approach | ||
| Using only commit messages for categorization was rejected because: | ||
| - Git history cannot be modified after merge for reclassification | ||
| - Commit messages may not reflect the overall PR purpose | ||
| - Multiple commits in a PR may have different types | ||
|
|
||
| ### PR Title Only Approach | ||
| Relying solely on PR titles was rejected because: | ||
| - PR authors can modify titles without maintainer oversight | ||
| - Labels provide better programmatic access for automation | ||
| - Maintainers need final categorization authority | ||
|
|
||
| ### Implicit Catch-All Category | ||
| Assuming unlabeled PRs are part of a catch-all category (for example "operations") was rejected because: | ||
| - Differentiates incomplete labeling from intentional categorization | ||
| - Enables systematic identification of unlabeled PRs | ||
| - Provides explicit categorization for all changes | ||
|
|
||
| ## Open Issues | ||
|
|
||
| 1. Should "dependency" remain a separate category or be classified under "refactor" or "chore"? | ||
| 2. What automation level is appropriate for title correction vs. suggestion? | ||
|
|
||
| ## References | ||
|
|
||
| - [Conventional Commits Specification](https://www.conventionalcommits.org/) | ||
| - [commitlint Configuration](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional) | ||
| - [Current Helm Contributing Guidelines](https://github.com/helm/helm/blob/main/CONTRIBUTING.md) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.