Add WithHead option to prcreator for App-auth PR creation#4974
Add WithHead option to prcreator for App-auth PR creation#4974jmguzik wants to merge 3 commits intoopenshift:mainfrom
Conversation
Allow prcreator to skip git operations (fork, commit, push) and only create/update the PR when --head is provided. This enables a split workflow where fork+push is handled externally (e.g. via bash with PAT) and prcreator only does PR creation using GitHub App auth. Signed-off-by: Jakub Guzik <jguzik@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughRefactors PR option passing in the CLI to build a variadic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmguzik The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/github/prcreation/prcreation.go`:
- Around line 124-126: Before calling upsertPR, add fast-fail validation of
inputs: if prArgs.head != "" validate the head value format (e.g., non-empty and
matches expected "user:branch" or "owner/branch" pattern) and return a clear
error if malformed; otherwise (when prArgs.head == "") ensure a token path is
provided (e.g., prArgs.tokenPath is non-empty) and return an error if missing.
Place these checks immediately before the existing conditional that calls
o.upsertPR(org, repo, branch, prTitle, prArgs) so invalid inputs are rejected
early.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
cmd/prcreator/main.gopkg/github/prcreation/orgaware.gopkg/github/prcreation/prcreation.go
pkg/github/prcreation/prcreation.go
Outdated
| if prArgs.head != "" { | ||
| return o.upsertPR(org, repo, branch, prTitle, prArgs) | ||
| } |
There was a problem hiding this comment.
Validate execution mode inputs before branching.
This branch should fail fast for malformed --head and for non-head mode without a token path; otherwise failures happen later during API/git operations and are harder to diagnose.
Suggested fix
if prArgs.head != "" {
+ parts := strings.SplitN(prArgs.head, ":", 2)
+ if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
+ return fmt.Errorf("invalid --head value %q, expected <owner>:<branch>", prArgs.head)
+ }
return o.upsertPR(org, repo, branch, prTitle, prArgs)
}
+
+ if o.TokenPath == "" {
+ return fmt.Errorf("token path is required when --head is not set")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if prArgs.head != "" { | |
| return o.upsertPR(org, repo, branch, prTitle, prArgs) | |
| } | |
| if prArgs.head != "" { | |
| parts := strings.SplitN(prArgs.head, ":", 2) | |
| if len(parts) != 2 || parts[0] == "" || parts[1] == "" { | |
| return fmt.Errorf("invalid --head value %q, expected <owner>:<branch>", prArgs.head) | |
| } | |
| return o.upsertPR(org, repo, branch, prTitle, prArgs) | |
| } | |
| if o.TokenPath == "" { | |
| return fmt.Errorf("token path is required when --head is not set") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/github/prcreation/prcreation.go` around lines 124 - 126, Before calling
upsertPR, add fast-fail validation of inputs: if prArgs.head != "" validate the
head value format (e.g., non-empty and matches expected "user:branch" or
"owner/branch" pattern) and return a clear error if malformed; otherwise (when
prArgs.head == "") ensure a token path is provided (e.g., prArgs.tokenPath is
non-empty) and return an error if missing. Place these checks immediately before
the existing conditional that calls o.upsertPR(org, repo, branch, prTitle,
prArgs) so invalid inputs are rejected early.
prcreation: add GitHub App auth support for direct upstream push
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/github/prcreation/prcreation.go`:
- Around line 167-170: The functions that call os.Chdir (notably upsertWithPAT)
change the global working directory and never restore it; capture the original
working directory with os.Getwd() before calling os.Chdir, then defer restoring
it (e.g., defer func() { _ = os.Chdir(origWd) }()) so the process cwd is always
reset even on early returns; apply the same pattern to the other function(s) in
this file that perform os.Chdir so all paths restore the original directory and
wrap or return an error if Getwd fails.
- Around line 322-326: prArgs.additionalLabels is being aliased by labelsToAdd
so appending self-approval labels can mutate the caller's slice; change the
assignment in the PR creation flow to make a copy (e.g. create a new slice and
copy contents of prArgs.additionalLabels into labelsToAdd) before the
SelfApprove branch, then append labels.Approved and labels.LGTM when
o.SelfApprove is true so callers' slices are not modified.
- Around line 136-139: branchNameFromTitle currently only replaces spaces and
colons and can still produce invalid git ref characters; update
branchNameFromTitle to fully sanitize the title by lowercasing it then replacing
any character that is not a lowercase letter, digit, hyphen, underscore, dot or
slash with a hyphen, collapse consecutive hyphens into one, trim
leading/trailing hyphens, dots and slashes, and ensure the result is non-empty
(fallback to a safe default like "branch") before returning; reference the
branchNameFromTitle function to locate and implement these transformations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59fe509d-1f9a-4db3-a7b2-cb6259a3db2d
📒 Files selected for processing (3)
cmd/prcreator/main.gopkg/github/prcreation/orgaware.gopkg/github/prcreation/prcreation.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/prcreator/main.go
| func branchNameFromTitle(title string) string { | ||
| name := strings.ReplaceAll(strings.ToLower(title), " ", "-") | ||
| name = strings.ReplaceAll(name, ":", "-") | ||
| return name |
There was a problem hiding this comment.
Harden branch name derivation against invalid git ref characters.
Current replacement handles only spaces and :, so some titles can still produce invalid refs and fail at checkout/push time.
Proposed fix
import (
"flag"
"fmt"
"io"
"os"
+ "regexp"
"strings"
"time"
@@
+var invalidBranchRunes = regexp.MustCompile(`[^a-z0-9._/-]+`)
+
func branchNameFromTitle(title string) string {
- name := strings.ReplaceAll(strings.ToLower(title), " ", "-")
+ name := strings.ReplaceAll(strings.ToLower(strings.TrimSpace(title)), " ", "-")
name = strings.ReplaceAll(name, ":", "-")
+ name = invalidBranchRunes.ReplaceAllString(name, "-")
+ name = strings.Trim(name, "./-")
+ if name == "" {
+ name = "autobump"
+ }
return name
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/github/prcreation/prcreation.go` around lines 136 - 139,
branchNameFromTitle currently only replaces spaces and colons and can still
produce invalid git ref characters; update branchNameFromTitle to fully sanitize
the title by lowercasing it then replacing any character that is not a lowercase
letter, digit, hyphen, underscore, dot or slash with a hyphen, collapse
consecutive hyphens into one, trim leading/trailing hyphens, dots and slashes,
and ensure the result is non-empty (fallback to a safe default like "branch")
before returning; reference the branchNameFromTitle function to locate and
implement these transformations.
| func (o *PRCreationOptions) upsertWithPAT(localSourceDir, org, repo, branch, prTitle string, prArgs *PrOptions) error { | ||
| if err := os.Chdir(localSourceDir); err != nil { | ||
| return fmt.Errorf("failed to chdir into %s: %w", localSourceDir, err) | ||
| } |
There was a problem hiding this comment.
Restore the original working directory after os.Chdir.
Both paths mutate process-global cwd and never restore it. In long-lived callers, later file/git ops can run in the wrong directory.
Proposed fix
func (o *PRCreationOptions) upsertWithPAT(localSourceDir, org, repo, branch, prTitle string, prArgs *PrOptions) error {
+ cwd, err := os.Getwd()
+ if err != nil {
+ return fmt.Errorf("failed to get current working directory: %w", err)
+ }
if err := os.Chdir(localSourceDir); err != nil {
return fmt.Errorf("failed to chdir into %s: %w", localSourceDir, err)
}
+ defer func() {
+ _ = os.Chdir(cwd)
+ }()
- changed, err := bumper.HasChanges()
+ changed, err := bumper.HasChanges()
if err != nil {
return fmt.Errorf("failed to check for changes: %w", err)
} func (o *PRCreationOptions) upsertWithAppAuth(localSourceDir, org, repo, branch, prTitle string, prArgs *PrOptions) error {
+ cwd, err := os.Getwd()
+ if err != nil {
+ return fmt.Errorf("failed to get current working directory: %w", err)
+ }
if err := os.Chdir(localSourceDir); err != nil {
return fmt.Errorf("failed to chdir into %s: %w", localSourceDir, err)
}
+ defer func() {
+ _ = os.Chdir(cwd)
+ }()
- changed, err := bumper.HasChanges()
+ changed, err := bumper.HasChanges()
if err != nil {
return fmt.Errorf("failed to check for changes: %w", err)
}Also applies to: 244-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/github/prcreation/prcreation.go` around lines 167 - 170, The functions
that call os.Chdir (notably upsertWithPAT) change the global working directory
and never restore it; capture the original working directory with os.Getwd()
before calling os.Chdir, then defer restoring it (e.g., defer func() { _ =
os.Chdir(origWd) }()) so the process cwd is always reset even on early returns;
apply the same pattern to the other function(s) in this file that perform
os.Chdir so all paths restore the original directory and wrap or return an error
if Getwd fails.
| labelsToAdd := prArgs.additionalLabels | ||
| if o.SelfApprove { | ||
| l.Infof("Self-aproving PR by adding the %q and %q labels", labels.Approved, labels.LGTM) | ||
| logrus.Infof("Self-approving PR by adding the %q and %q labels", labels.Approved, labels.LGTM) | ||
| labelsToAdd = append(labelsToAdd, labels.Approved, labels.LGTM) | ||
| } |
There was a problem hiding this comment.
Copy additionalLabels before appending self-approval labels.
labelsToAdd := prArgs.additionalLabels aliases the caller slice; append may mutate caller-owned data when capacity allows.
Proposed fix
- labelsToAdd := prArgs.additionalLabels
+ labelsToAdd := append([]string(nil), prArgs.additionalLabels...)
if o.SelfApprove {
logrus.Infof("Self-approving PR by adding the %q and %q labels", labels.Approved, labels.LGTM)
labelsToAdd = append(labelsToAdd, labels.Approved, labels.LGTM)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| labelsToAdd := prArgs.additionalLabels | |
| if o.SelfApprove { | |
| l.Infof("Self-aproving PR by adding the %q and %q labels", labels.Approved, labels.LGTM) | |
| logrus.Infof("Self-approving PR by adding the %q and %q labels", labels.Approved, labels.LGTM) | |
| labelsToAdd = append(labelsToAdd, labels.Approved, labels.LGTM) | |
| } | |
| labelsToAdd := append([]string(nil), prArgs.additionalLabels...) | |
| if o.SelfApprove { | |
| logrus.Infof("Self-approving PR by adding the %q and %q labels", labels.Approved, labels.LGTM) | |
| labelsToAdd = append(labelsToAdd, labels.Approved, labels.LGTM) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/github/prcreation/prcreation.go` around lines 322 - 326,
prArgs.additionalLabels is being aliased by labelsToAdd so appending
self-approval labels can mutate the caller's slice; change the assignment in the
PR creation flow to make a copy (e.g. create a new slice and copy contents of
prArgs.additionalLabels into labelsToAdd) before the SelfApprove branch, then
append labels.Approved and labels.LGTM when o.SelfApprove is true so callers'
slices are not modified.
|
@jmguzik: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Allow prcreator to skip git operations (fork, commit, push) and only create/update the PR when --head is provided. This enables a split workflow where fork+push is handled externally (e.g. via bash with PAT) and prcreator only does PR creation using GitHub App auth.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements