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/internal/github/client_test.go b/internal/github/client_test.go index 9e81413..0115d99 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -200,34 +200,93 @@ 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{}, + }, + } + + 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", }, } @@ -235,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 14c98ba..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" ) @@ -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 @@ -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 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 {