Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ linters:
excludes:
- G302 # OpenFile permissions - 0644 is appropriate for GITHUB_OUTPUT
- G304 # File path from variable is expected for CLI tools
- G703 # Path traversal via taint - file paths from config are expected for CLI tools
- G306 # WriteFile permissions - 0644 is appropriate for VERSION files
- G601
- G602 # Slice bounds checked by prior logic
Expand Down
81 changes: 70 additions & 11 deletions internal/github/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,43 +200,102 @@ func TestClient_ImplementsPRCreator(t *testing.T) {
var _ PRCreator = client
}

// TestDeduplicateFiles tests that duplicate file paths are removed while preserving order.
func TestDeduplicateFiles(t *testing.T) {
t.Parallel()

tests := []struct {
name string
input []string
want []string
}{
{
name: "no duplicates",
input: []string{"a.yaml", "b.yaml", "c.yaml"},
want: []string{"a.yaml", "b.yaml", "c.yaml"},
},
{
name: "single file duplicated",
input: []string{"deploy/charts/operator-crds/Chart.yaml", "deploy/charts/operator-crds/Chart.yaml"},
want: []string{"deploy/charts/operator-crds/Chart.yaml"},
},
{
name: "multiple files with duplicates preserves order",
input: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml", "deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml", "deploy/charts/operator/values.yaml"},
want: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml"},
},
{
name: "multiple files with duplicate names but separate paths preserves order",
input: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml", "deploy/charts/operator-crds/Chart.yaml", "deploy/charts/operator-crds/values.yaml"},
want: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml", "deploy/charts/operator-crds/Chart.yaml", "deploy/charts/operator-crds/values.yaml"},
},
{
name: "single file",
input: []string{"VERSION"},
want: []string{"VERSION"},
},
{
name: "empty slice",
input: []string{},
want: []string{},
},
{
name: "nil slice",
input: nil,
want: []string{},
},
Comment thread
aponcedeleonch marked this conversation as resolved.
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

got := deduplicateFiles(tt.input)
if len(got) != len(tt.want) {
t.Fatalf("deduplicateFiles() returned %d items, want %d\ngot: %v\nwant: %v", len(got), len(tt.want), got, tt.want)
}
for i := range got {
if got[i] != tt.want[i] {
t.Errorf("deduplicateFiles()[%d] = %q, want %q", i, got[i], tt.want[i])
}
}
})
}
}

// TestCommitMessageFormat tests the commit message format with and without git trailer.
// This tests the format logic used in commitFile().
// This tests the format logic used in commitFiles().
func TestCommitMessageFormat(t *testing.T) {
t.Parallel()

tests := []struct {
name string
fileName string
triggeredBy string
wantMessage string
}{
{
name: "without triggered by",
fileName: "VERSION",
triggeredBy: "",
wantMessage: "Update VERSION for release",
wantMessage: "Update release files",
},
{
name: "with triggered by",
fileName: "VERSION",
triggeredBy: "testuser",
wantMessage: "Update VERSION for release\n\nRelease-Triggered-By: testuser",
wantMessage: "Update release files\n\nRelease-Triggered-By: testuser",
},
{
name: "with triggered by on Chart.yaml",
fileName: "Chart.yaml",
name: "with triggered by from releasebot",
triggeredBy: "releasebot",
wantMessage: "Update Chart.yaml for release\n\nRelease-Triggered-By: releasebot",
wantMessage: "Update release files\n\nRelease-Triggered-By: releasebot",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

// Replicate the message format logic from commitFile()
message := "Update " + tt.fileName + " for release"
// Replicate the message format logic from commitFiles()
message := "Update release files"
if tt.triggeredBy != "" {
message += "\n\nRelease-Triggered-By: " + tt.triggeredBy
}
Expand Down
102 changes: 71 additions & 31 deletions internal/github/pr.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package github
import (
"context"
"fmt"
"path/filepath"

"github.com/google/go-github/v60/github"
)
Expand Down Expand Up @@ -45,11 +44,14 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult,
return nil, fmt.Errorf("creating branch: %w", err)
}

// Commit the files to the new branch
for _, filePath := range req.Files {
if err := c.commitFile(ctx, req.Owner, req.Repo, req.HeadBranch, filePath, req.TriggeredBy); err != nil {
return nil, fmt.Errorf("committing file %s: %w", filePath, err)
}
// Deduplicate files - each file already has all YAML path changes applied
// on disk, so committing the same file twice causes 409 conflicts due to
// GitHub API eventual consistency with sequential SHA updates.
uniqueFiles := deduplicateFiles(req.Files)

// Commit all files to the new branch in a single atomic commit
if err := c.commitFiles(ctx, req.Owner, req.Repo, req.HeadBranch, uniqueFiles, req.TriggeredBy); err != nil {
return nil, fmt.Errorf("committing files: %w", err)
}

// Create the pull request
Expand All @@ -72,43 +74,81 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult,
}, nil
}

// commitFile commits a single file to a branch.
// deduplicateFiles returns a new slice with duplicate file paths removed,
// preserving the order of first occurrence.
func deduplicateFiles(files []string) []string {
seen := make(map[string]bool, len(files))
unique := make([]string, 0, len(files))
for _, f := range files {
if !seen[f] {
seen[f] = true
unique = append(unique, f)
}
}
return unique
}

// commitFiles commits all files to a branch in a single atomic commit using the Git Data API.
// If triggeredBy is non-empty, a git trailer is added to the commit message.
func (c *Client) commitFile(ctx context.Context, owner, repo, branch, filePath, triggeredBy string) error {
// Read file content using the fileReader interface
content, err := c.fileReader.ReadFile(filePath)
func (c *Client) commitFiles(ctx context.Context, owner, repo, branch string, files []string, triggeredBy string) error {
// Get the current branch reference
ref, _, err := c.client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch)
if err != nil {
return fmt.Errorf("reading file: %w", err)
return fmt.Errorf("getting branch ref: %w", err)
}

// Get current file (to get SHA for update)
existingFile, _, _, err := c.client.Repositories.GetContents(
ctx, owner, repo, filePath,
&github.RepositoryContentGetOptions{Ref: branch},
)
// Get the commit to find the base tree
baseCommit, _, err := c.client.Git.GetCommit(ctx, owner, repo, ref.GetObject().GetSHA())
if err != nil {
return fmt.Errorf("getting base commit: %w", err)
}

message := fmt.Sprintf("Update %s for release", filepath.Base(filePath))
if triggeredBy != "" {
message += fmt.Sprintf("\n\nRelease-Triggered-By: %s", triggeredBy)
// Build tree entries for all files
entries := make([]*github.TreeEntry, 0, len(files))
for _, filePath := range files {
content, err := c.fileReader.ReadFile(filePath)
if err != nil {
return fmt.Errorf("reading file %s: %w", filePath, err)
}
contentStr := string(content)
entries = append(entries, &github.TreeEntry{
Path: github.String(filePath),
Mode: github.String("100644"),
Type: github.String("blob"),
Content: github.String(contentStr),
})
}

opts := &github.RepositoryContentFileOptions{
Message: github.String(message),
Content: content,
Branch: github.String(branch),
// Create a new tree with all file changes
tree, _, err := c.client.Git.CreateTree(ctx, owner, repo, baseCommit.GetTree().GetSHA(), entries)
if err != nil {
return fmt.Errorf("creating tree: %w", err)
}

if err == nil && existingFile != nil {
// File exists - update it
opts.SHA = existingFile.SHA
_, _, err = c.client.Repositories.UpdateFile(ctx, owner, repo, filePath, opts)
} else {
// File doesn't exist - create it
_, _, err = c.client.Repositories.CreateFile(ctx, owner, repo, filePath, opts)
// Build commit message
message := "Update release files"
if triggeredBy != "" {
message += fmt.Sprintf("\n\nRelease-Triggered-By: %s", triggeredBy)
}

// Create the commit
commit, _, err := c.client.Git.CreateCommit(ctx, owner, repo,
&github.Commit{
Message: github.String(message),
Tree: tree,
Parents: []*github.Commit{baseCommit},
},
nil,
)
if err != nil {
return fmt.Errorf("creating commit: %w", err)
}

// Update the branch reference to point to the new commit
ref.Object.SHA = commit.SHA
_, _, err = c.client.Git.UpdateRef(ctx, owner, repo, ref, false)
if err != nil {
return fmt.Errorf("updating file: %w", err)
return fmt.Errorf("updating ref: %w", err)
}

return nil
Expand Down
6 changes: 3 additions & 3 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,14 +311,14 @@ func validateConfig(cfg Config) {
func generatePRBody(ver, bumpType string, versionFiles []files.VersionFileConfig, ranHelmDocs bool) string {
var sb strings.Builder

sb.WriteString(fmt.Sprintf("## Release v%s\n\n", ver))
fmt.Fprintf(&sb, "## Release v%s\n\n", ver)
sb.WriteString("### Version Bump\n\n")
sb.WriteString(fmt.Sprintf("**%s** release\n\n", bumpType))
fmt.Fprintf(&sb, "**%s** release\n\n", bumpType)
sb.WriteString("### Files Updated\n\n")
sb.WriteString("- `VERSION`\n")

for _, vf := range versionFiles {
sb.WriteString(fmt.Sprintf("- `%s` (path: `%s`)\n", vf.File, vf.Path))
fmt.Fprintf(&sb, "- `%s` (path: `%s`)\n", vf.File, vf.Path)
}

if ranHelmDocs {
Expand Down
Loading