From 685cfeccb99d661c4b5be2122e3f2326c03b8c98 Mon Sep 17 00:00:00 2001 From: Alejandro Ponce Date: Thu, 12 Mar 2026 16:59:13 +0200 Subject: [PATCH 1/3] Fix 409 conflicts by deduplicating files before commit loop When a file appears multiple times in version_files config (e.g., Chart.yaml with both version and appVersion paths), the commit loop would attempt to commit the same file twice. Since all YAML path changes are applied on disk before any commit, the second commit hits a stale SHA due to GitHub API eventual consistency, causing a 409 conflict. Deduplicate req.Files before iterating so each file is committed exactly once with all its changes. Co-Authored-By: Claude Opus 4.6 --- internal/github/client_test.go | 58 ++++++++++++++++++++++++++++++++++ internal/github/pr.go | 21 +++++++++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 9e81413..bb17e38 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -200,6 +200,64 @@ 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: "single file", + input: []string{"VERSION"}, + want: []string{"VERSION"}, + }, + { + name: "empty slice", + input: []string{}, + want: []string{}, + }, + { + name: "nil slice", + input: nil, + want: []string{}, + }, + } + + 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(). func TestCommitMessageFormat(t *testing.T) { diff --git a/internal/github/pr.go b/internal/github/pr.go index 14c98ba..6dc8774 100644 --- a/internal/github/pr.go +++ b/internal/github/pr.go @@ -45,8 +45,13 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult, return nil, fmt.Errorf("creating branch: %w", 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 the files to the new branch - for _, filePath := range req.Files { + for _, filePath := range uniqueFiles { 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) } @@ -72,6 +77,20 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult, }, nil } +// 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 +} + // commitFile commits a single file to a branch. // 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 { From 0dbffbf1359ebbe1dbc985bacfab492780e2dc46 Mon Sep 17 00:00:00 2001 From: Alejandro Ponce Date: Thu, 12 Mar 2026 17:07:05 +0200 Subject: [PATCH 2/3] linting issues --- .golangci.yml | 1 + main.go | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 5de099f..d6ea599 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -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 diff --git a/main.go b/main.go index 6ca03dc..d105c9b 100644 --- a/main.go +++ b/main.go @@ -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 { From 900a19167c1c8d3a27af8dc46745c5414b7109dd Mon Sep 17 00:00:00 2001 From: Alejandro Ponce Date: Thu, 12 Mar 2026 17:51:22 +0200 Subject: [PATCH 3/3] Batch all file changes into a single Git commit via Git Data API Replace per-file CreateFile/UpdateFile calls with a single atomic commit using the GitHub Git Data API (trees + commits). This produces cleaner git history and eliminates 409 conflicts entirely. Co-Authored-By: Claude Opus 4.6 --- internal/github/client_test.go | 23 +++++----- internal/github/pr.go | 83 +++++++++++++++++++++------------- 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/internal/github/client_test.go b/internal/github/client_test.go index bb17e38..0115d99 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -224,6 +224,11 @@ func TestDeduplicateFiles(t *testing.T) { 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"}, @@ -259,33 +264,29 @@ func TestDeduplicateFiles(t *testing.T) { } // 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", }, } @@ -293,8 +294,8 @@ func TestCommitMessageFormat(t *testing.T) { 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 } diff --git a/internal/github/pr.go b/internal/github/pr.go index 6dc8774..e01fd08 100644 --- a/internal/github/pr.go +++ b/internal/github/pr.go @@ -17,7 +17,6 @@ package github import ( "context" "fmt" - "path/filepath" "github.com/google/go-github/v60/github" ) @@ -50,11 +49,9 @@ func (c *Client) CreateReleasePR(ctx context.Context, req PRRequest) (*PRResult, // GitHub API eventual consistency with sequential SHA updates. uniqueFiles := deduplicateFiles(req.Files) - // Commit the files to the new branch - for _, filePath := range uniqueFiles { - 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) - } + // 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 @@ -91,43 +88,67 @@ func deduplicateFiles(files []string) []string { return unique } -// commitFile commits a single file to a branch. +// 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