Add krel release-notes rerun subcommand with push support and test coverage#4349
Add krel release-notes rerun subcommand with push support and test coverage#4349kernel-kun wants to merge 6 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
680c0f9 to
8a0d188
Compare
Trigger markdown re-render when SIGs change in map files, and fix threshold from len > 1 to len >= 1 so single-SIG PRs retain their [SIG XYZ] suffix during krel release-notes rerun.
- Add Example field to rerunCmd with realistic invocations - Expand Long description with numbered process steps - Replace static release-x.yz placeholder in maps-from warning with dynamic path parsed from --tag (e.g. release-1.32) - Add GITHUB_TOKEN check to validateRerunOpts for early failure - Update tests for pre-release tags and proper logrus output capture
2e48988 to
b8da3da
Compare
…mmand functionality
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kernel-kun The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds a new krel release-notes rerun subcommand to support regenerating release-notes drafts against an existing draft branch (optionally pushing updates), plus fixes/coverage for notes mapping and SIG label rendering.
Changes:
- Introduce
release-notes reruncobra subcommand with source/push fork+branch flags and validation, plus end-to-end rerun implementation. - Fix release-note markdown re-rendering so single-SIG notes retain
[SIG ...], and ensure SIG changes trigger markdown rebuild. - Make
DirectoryMapProvider.GetMapsForPRinitialization thread-safe and add tests/docs for the new workflow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/krel/cmd/release_notes.go |
Adds rerun subcommand, rerun implementation, and rerun-specific option validation. |
cmd/krel/cmd/release_notes_test.go |
Adds unit tests for validateRerunOpts (required flags, normalization, defaults, warning). |
pkg/notes/notes.go |
Fixes SIG label retention and markdown re-render when SIGs are mapped. |
pkg/notes/notes_map.go |
Uses sync.Once to make DirectoryMapProvider map loading concurrency-safe. |
pkg/notes/notes_map_test.go |
Adds concurrent test coverage for DirectoryMapProvider.GetMapsForPR. |
docs/krel/release-notes.md |
Documents the new rerun workflow, flags, and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Parse --draft-pr-source-fork: if no "/" present, append "/sig-release" | ||
| if !strings.Contains(o.draftPRSourceFork, "/") { | ||
| o.draftPRSourceFork = o.draftPRSourceFork + "/" + git.DefaultGithubReleaseRepo | ||
| } | ||
|
|
||
| // Parse --draft-pr-push-fork: if no "/" present, append "/sig-release" | ||
| if o.draftPRPushFork != "" && !strings.Contains(o.draftPRPushFork, "/") { | ||
| o.draftPRPushFork = o.draftPRPushFork + "/" + git.DefaultGithubReleaseRepo | ||
| } |
There was a problem hiding this comment.
validateRerunOpts normalizes --draft-pr-*-fork by checking for the presence of a "/", but it does not validate that the value is actually in org or org/repo form (e.g., it would accept org/, org/repo/extra, or org//repo). Since rerunDraftNotes assumes exactly two non-empty parts when splitting, add explicit validation (e.g., via git.ParseRepoSlug plus checks that both org and repo are non-empty) and return a clear error if the format is invalid.
| // The release path inside the sig-release repository | ||
| releasePath := filepath.Join("releases", fmt.Sprintf("release-%d.%d", tagVersion.Major, tagVersion.Minor)) | ||
| releaseDir := filepath.Join(sigReleaseRepo.Dir(), releasePath) | ||
|
|
There was a problem hiding this comment.
rerunDraftNotes does not check whether releaseDir exists before trying to create the work subdirectories. If the releases/release-x.y directory is missing for the provided tag, this will fail with a less actionable mkdir ... no such file or directory error. Mirror createDraftPR by checking helpers.Exists(releaseDir) and returning a targeted error (e.g. "could not find release directory ...").
| if !helpers.Exists(releaseDir) { | |
| return fmt.Errorf("could not find release directory %s", releaseDir) | |
| } |
| sigReleaseRepo, err := git.CleanCloneGitHubRepo( | ||
| git.DefaultGithubOrg, git.DefaultGithubReleaseRepo, | ||
| false, true, opts, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("cloning kubernetes/sig-release: %w", err) | ||
| } | ||
|
|
||
| // Do NOT call Cleanup — preserve the local clone for user inspection | ||
|
|
||
| // Add the source fork as a remote named "source" | ||
| logrus.Infof("Adding remote 'source' for %s/%s", sourceOrg, sourceRepo) | ||
|
|
||
| // Use HTTPS for the source remote — it's a read-only fetch from a public fork | ||
| if err := sigReleaseRepo.AddRemote("source", sourceOrg, sourceRepo, false); err != nil { | ||
| return fmt.Errorf("adding source remote %s/%s: %w", sourceOrg, sourceRepo, err) |
There was a problem hiding this comment.
rerunDraftNotes hard-codes HTTPS (useSSH=false) when cloning and when adding remotes. This ignores the inherited --use-ssh flag and can make pushes fail for users who rely on SSH auth. Thread releaseNotesOpts.useSSH (and likely releaseNotesOpts.updateRepo) through the clone/remote setup, at minimum for the push remote.
ApplyMap() used noteMap.PRBody only for change-detection logging but never actually assigned it to the release note. Every other mapped field (Text, Author, SIGs, Areas, Kinds, etc.) had an explicit `rn.X = *noteMap.X` assignment, but PRBody was missing one. This caused map entries with `pr_body: ""` to be silently ignored during rerun: the note kept the full GitHub PR body fetched from the API, which then appeared in the JSON output despite the map intending to blank it out. Add `rn.PRBody = *noteMap.PRBody` so the field is applied like all other mapped fields. Combined with the existing `omitempty` JSON tag, an empty pr_body in the map now correctly omits the field from output.
/kind feature
What this PR does / why we need it:
Adds a
krel release-notes rerunsubcommand that fetches an existing draft branch from a source fork of k/sig-release, regenerates the release notes (applying maps), commits the result, and optionally pushes to a destination fork. This supports iterative release notes workflows where the draft needs to be regenerated multiple times during a release cycle.Also fixes a bug where SIG labels were dropped from the markdown during
ApplyMapre-rendering, and makesDirectoryMapProvider.GetMapsForPRthread-safe.Changes:
cmd/krel/cmd/release_notes.gorerunCmdas a proper cobra subcommand ofrelease-notes(not a flag) with its own flags, validation, help text, and examples--draft-pr-source-fork(required): specifies the k/sig-release fork to fetch the existing draft branch from (orgororg/repo)--draft-pr-source-branch: branch to fetch (defaults torelease-notes-draft-<tag>)--draft-pr-push-fork: optional destination fork to push regenerated notes to (orgororg/repo, omit to skip push)--draft-pr-push-branch: branch to push to on the destination fork (defaults to source branch)rerunDraftNotes()function: clones upstream k/sig-release, fetches the draft branch from source fork, auto-discovers maps from the branch, regenerates release notes, commits, and optionally pushes with--force-with-leasevalidateRerunOpts(): validatesGITHUB_TOKEN, required flags, fork normalization ("myorg"→"myorg/sig-release"), branch default resolution, and dynamic--maps-fromwarning (e.g.releases/release-1.32/release-notes/mapsinstead ofrelease-x.yz)Longdescription with numbered process steps andExamplefield with realistic invocationspkg/notes/notes.goApplyMapre-render: changelen(rn.SIGs) > 1tolen(rn.SIGs) >= 1so single-SIG notes retain their[SIG Foo]suffixreRenderMarkdown = truewhennoteMap.ReleaseNote.SIGsis applied, ensuring the markdown is rebuilt with the updated SIG listrn.PRBody = *noteMap.PRBodyso thatpr_bodyis applied from maps to.jsonfile (similar to all other fields)pkg/notes/notes_map.goMapswithsync.OnceinDirectoryMapProvider.GetMapsForPRto make concurrent calls thread-safecmd/krel/cmd/release_notes_test.go(new file)TestValidateRerunOpts_RequiredFlags: missing--draft-pr-source-forkand missing--tagboth errorTestValidateRerunOpts_ForkNormalization: org-only source/push fork gets/sig-releaseappended; fullorg/repostays unchangedTestValidateRerunOpts_BranchDefaults: empty source branch defaults torelease-notes-draft-<tag>; empty push branch defaults to source branchTestValidateRerunOpts_ValidOptions: all required flags set, validation returns nilTestValidateRerunOpts_DynamicMapsWarning: verifies the warning contains the dynamic path for alpha, beta, and rc tagspkg/notes/notes_map_test.goTestGetMapsForPR_Concurrent: 100 goroutines callingGetMapsForPRon the sameDirectoryMapProviderinstance, asserting consistent results and no panicsHelp output:
Which issue(s) this PR fixes:
#4348 #4121
Special notes for your reviewer:
sut_test.go/ff_test.goare unrelated to this PR — they fail onmasteras well (git push errors in the test harness).go test -race.golangci-lint runpasses clean.rerunfunctionality was initially implemented as a--rerunflag, then extracted into a proper cobra subcommand for better UX (dedicated help, examples, flag scoping).Does this PR introduce a user-facing change?