Skip to content

Commit 80e8d81

Browse files
Batch all file changes into a single atomic Git commit (#25)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent cd90dbc commit 80e8d81

File tree

4 files changed

+145
-45
lines changed

4 files changed

+145
-45
lines changed

.golangci.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ linters:
4343
excludes:
4444
- G302 # OpenFile permissions - 0644 is appropriate for GITHUB_OUTPUT
4545
- G304 # File path from variable is expected for CLI tools
46+
- G703 # Path traversal via taint - file paths from config are expected for CLI tools
4647
- G306 # WriteFile permissions - 0644 is appropriate for VERSION files
4748
- G601
4849
- G602 # Slice bounds checked by prior logic

internal/github/client_test.go

Lines changed: 70 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -200,43 +200,102 @@ func TestClient_ImplementsPRCreator(t *testing.T) {
200200
var _ PRCreator = client
201201
}
202202

203+
// TestDeduplicateFiles tests that duplicate file paths are removed while preserving order.
204+
func TestDeduplicateFiles(t *testing.T) {
205+
t.Parallel()
206+
207+
tests := []struct {
208+
name string
209+
input []string
210+
want []string
211+
}{
212+
{
213+
name: "no duplicates",
214+
input: []string{"a.yaml", "b.yaml", "c.yaml"},
215+
want: []string{"a.yaml", "b.yaml", "c.yaml"},
216+
},
217+
{
218+
name: "single file duplicated",
219+
input: []string{"deploy/charts/operator-crds/Chart.yaml", "deploy/charts/operator-crds/Chart.yaml"},
220+
want: []string{"deploy/charts/operator-crds/Chart.yaml"},
221+
},
222+
{
223+
name: "multiple files with duplicates preserves order",
224+
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"},
225+
want: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml"},
226+
},
227+
{
228+
name: "multiple files with duplicate names but separate paths preserves order",
229+
input: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml", "deploy/charts/operator-crds/Chart.yaml", "deploy/charts/operator-crds/values.yaml"},
230+
want: []string{"deploy/charts/operator/Chart.yaml", "deploy/charts/operator/values.yaml", "deploy/charts/operator-crds/Chart.yaml", "deploy/charts/operator-crds/values.yaml"},
231+
},
232+
{
233+
name: "single file",
234+
input: []string{"VERSION"},
235+
want: []string{"VERSION"},
236+
},
237+
{
238+
name: "empty slice",
239+
input: []string{},
240+
want: []string{},
241+
},
242+
{
243+
name: "nil slice",
244+
input: nil,
245+
want: []string{},
246+
},
247+
}
248+
249+
for _, tt := range tests {
250+
t.Run(tt.name, func(t *testing.T) {
251+
t.Parallel()
252+
253+
got := deduplicateFiles(tt.input)
254+
if len(got) != len(tt.want) {
255+
t.Fatalf("deduplicateFiles() returned %d items, want %d\ngot: %v\nwant: %v", len(got), len(tt.want), got, tt.want)
256+
}
257+
for i := range got {
258+
if got[i] != tt.want[i] {
259+
t.Errorf("deduplicateFiles()[%d] = %q, want %q", i, got[i], tt.want[i])
260+
}
261+
}
262+
})
263+
}
264+
}
265+
203266
// TestCommitMessageFormat tests the commit message format with and without git trailer.
204-
// This tests the format logic used in commitFile().
267+
// This tests the format logic used in commitFiles().
205268
func TestCommitMessageFormat(t *testing.T) {
206269
t.Parallel()
207270

208271
tests := []struct {
209272
name string
210-
fileName string
211273
triggeredBy string
212274
wantMessage string
213275
}{
214276
{
215277
name: "without triggered by",
216-
fileName: "VERSION",
217278
triggeredBy: "",
218-
wantMessage: "Update VERSION for release",
279+
wantMessage: "Update release files",
219280
},
220281
{
221282
name: "with triggered by",
222-
fileName: "VERSION",
223283
triggeredBy: "testuser",
224-
wantMessage: "Update VERSION for release\n\nRelease-Triggered-By: testuser",
284+
wantMessage: "Update release files\n\nRelease-Triggered-By: testuser",
225285
},
226286
{
227-
name: "with triggered by on Chart.yaml",
228-
fileName: "Chart.yaml",
287+
name: "with triggered by from releasebot",
229288
triggeredBy: "releasebot",
230-
wantMessage: "Update Chart.yaml for release\n\nRelease-Triggered-By: releasebot",
289+
wantMessage: "Update release files\n\nRelease-Triggered-By: releasebot",
231290
},
232291
}
233292

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

238-
// Replicate the message format logic from commitFile()
239-
message := "Update " + tt.fileName + " for release"
297+
// Replicate the message format logic from commitFiles()
298+
message := "Update release files"
240299
if tt.triggeredBy != "" {
241300
message += "\n\nRelease-Triggered-By: " + tt.triggeredBy
242301
}

internal/github/pr.go

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ package github
1717
import (
1818
"context"
1919
"fmt"
20-
"path/filepath"
2120

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

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

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

75-
// commitFile commits a single file to a branch.
77+
// deduplicateFiles returns a new slice with duplicate file paths removed,
78+
// preserving the order of first occurrence.
79+
func deduplicateFiles(files []string) []string {
80+
seen := make(map[string]bool, len(files))
81+
unique := make([]string, 0, len(files))
82+
for _, f := range files {
83+
if !seen[f] {
84+
seen[f] = true
85+
unique = append(unique, f)
86+
}
87+
}
88+
return unique
89+
}
90+
91+
// commitFiles commits all files to a branch in a single atomic commit using the Git Data API.
7692
// If triggeredBy is non-empty, a git trailer is added to the commit message.
77-
func (c *Client) commitFile(ctx context.Context, owner, repo, branch, filePath, triggeredBy string) error {
78-
// Read file content using the fileReader interface
79-
content, err := c.fileReader.ReadFile(filePath)
93+
func (c *Client) commitFiles(ctx context.Context, owner, repo, branch string, files []string, triggeredBy string) error {
94+
// Get the current branch reference
95+
ref, _, err := c.client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch)
8096
if err != nil {
81-
return fmt.Errorf("reading file: %w", err)
97+
return fmt.Errorf("getting branch ref: %w", err)
8298
}
8399

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

90-
message := fmt.Sprintf("Update %s for release", filepath.Base(filePath))
91-
if triggeredBy != "" {
92-
message += fmt.Sprintf("\n\nRelease-Triggered-By: %s", triggeredBy)
106+
// Build tree entries for all files
107+
entries := make([]*github.TreeEntry, 0, len(files))
108+
for _, filePath := range files {
109+
content, err := c.fileReader.ReadFile(filePath)
110+
if err != nil {
111+
return fmt.Errorf("reading file %s: %w", filePath, err)
112+
}
113+
contentStr := string(content)
114+
entries = append(entries, &github.TreeEntry{
115+
Path: github.String(filePath),
116+
Mode: github.String("100644"),
117+
Type: github.String("blob"),
118+
Content: github.String(contentStr),
119+
})
93120
}
94121

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

101-
if err == nil && existingFile != nil {
102-
// File exists - update it
103-
opts.SHA = existingFile.SHA
104-
_, _, err = c.client.Repositories.UpdateFile(ctx, owner, repo, filePath, opts)
105-
} else {
106-
// File doesn't exist - create it
107-
_, _, err = c.client.Repositories.CreateFile(ctx, owner, repo, filePath, opts)
128+
// Build commit message
129+
message := "Update release files"
130+
if triggeredBy != "" {
131+
message += fmt.Sprintf("\n\nRelease-Triggered-By: %s", triggeredBy)
132+
}
133+
134+
// Create the commit
135+
commit, _, err := c.client.Git.CreateCommit(ctx, owner, repo,
136+
&github.Commit{
137+
Message: github.String(message),
138+
Tree: tree,
139+
Parents: []*github.Commit{baseCommit},
140+
},
141+
nil,
142+
)
143+
if err != nil {
144+
return fmt.Errorf("creating commit: %w", err)
108145
}
109146

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

114154
return nil

main.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,14 +311,14 @@ func validateConfig(cfg Config) {
311311
func generatePRBody(ver, bumpType string, versionFiles []files.VersionFileConfig, ranHelmDocs bool) string {
312312
var sb strings.Builder
313313

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

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

324324
if ranHelmDocs {

0 commit comments

Comments
 (0)