Skip to content

Commit 0b129ef

Browse files
fix: remove redundant error wrapping in pull request operations (#704)
* Initial plan * fix: remove redundant error message wrapping in pull request operations Co-authored-by: crenshaw-dev <[email protected]> * test: add test for merge error message format and fix lint issues Co-authored-by: crenshaw-dev <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: crenshaw-dev <[email protected]>
1 parent 30ef17f commit 0b129ef

File tree

6 files changed

+73
-19
lines changed

6 files changed

+73
-19
lines changed

internal/controller/pullrequest_controller.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func (r *PullRequestReconciler) handleStateTransitions(ctx context.Context, pr *
206206
if pr.Status.State == pr.Spec.State {
207207
logger.Info("Updating PullRequest")
208208
if err := r.updatePullRequest(ctx, *pr, provider); err != nil {
209-
return false, fmt.Errorf("failed to update pull request: %w", err)
209+
return false, fmt.Errorf("failed to update pull request: %w", err) // Top-level wrap for update errors
210210
}
211211
return false, nil
212212
}
@@ -217,13 +217,13 @@ func (r *PullRequestReconciler) handleStateTransitions(ctx context.Context, pr *
217217
// Because status id is empty, we need to create a new pull request
218218
logger.Info("Creating PullRequest")
219219
if err := r.createPullRequest(ctx, pr, provider); err != nil {
220-
return false, fmt.Errorf("failed to create pull request: %w", err)
220+
return false, fmt.Errorf("failed to create pull request: %w", err) // Top-level wrap for create errors
221221
}
222222
}
223223
case promoterv1alpha1.PullRequestMerged:
224224
logger.Info("Merging PullRequest")
225225
if err := r.mergePullRequest(ctx, pr, provider); err != nil {
226-
return false, fmt.Errorf("failed to merge pull request: %w", err)
226+
return false, fmt.Errorf("failed to merge pull request: %w", err) // Top-level wrap for merge errors
227227
}
228228
if err := r.Delete(ctx, pr); err != nil {
229229
return false, fmt.Errorf("failed to delete PullRequest: %w", err)
@@ -232,7 +232,7 @@ func (r *PullRequestReconciler) handleStateTransitions(ctx context.Context, pr *
232232
case promoterv1alpha1.PullRequestClosed:
233233
logger.Info("Closing PullRequest")
234234
if err := r.closePullRequest(ctx, pr, provider); err != nil {
235-
return false, fmt.Errorf("failed to close pull request: %w", err)
235+
return false, fmt.Errorf("failed to close pull request: %w", err) // Top-level wrap for close errors
236236
}
237237
if err := r.Delete(ctx, pr); err != nil {
238238
return false, fmt.Errorf("failed to delete PullRequest: %w", err)
@@ -325,7 +325,7 @@ func (r *PullRequestReconciler) handleFinalizer(ctx context.Context, pr *promote
325325
// In this case, we can just remove the finalizer without attempting to close the PR.
326326
if pr.Status.ID != "" {
327327
if err := r.closePullRequest(ctx, pr, provider); err != nil {
328-
return false, fmt.Errorf("failed to close pull request: %w", err)
328+
return false, fmt.Errorf("failed to close pull request: %w", err) // Top-level wrap for close errors
329329
}
330330
}
331331

@@ -339,7 +339,7 @@ func (r *PullRequestReconciler) handleFinalizer(ctx context.Context, pr *promote
339339
func (r *PullRequestReconciler) createPullRequest(ctx context.Context, pr *promoterv1alpha1.PullRequest, provider scms.PullRequestProvider) error {
340340
id, err := provider.Create(ctx, pr.Spec.Title, pr.Spec.SourceBranch, pr.Spec.TargetBranch, pr.Spec.Description, *pr)
341341
if err != nil {
342-
return fmt.Errorf("failed to create pull request: %w", err)
342+
return err //nolint:wrapcheck // Error wrapping handled at top level
343343
}
344344
pr.Status.State = promoterv1alpha1.PullRequestOpen
345345
pr.Status.PRCreationTime = metav1.Now()
@@ -356,7 +356,7 @@ func (r *PullRequestReconciler) createPullRequest(ctx context.Context, pr *promo
356356

357357
func (r *PullRequestReconciler) updatePullRequest(ctx context.Context, pr promoterv1alpha1.PullRequest, provider scms.PullRequestProvider) error {
358358
if err := provider.Update(ctx, pr.Spec.Title, pr.Spec.Description, pr); err != nil {
359-
return fmt.Errorf("failed to update pull request: %w", err)
359+
return err //nolint:wrapcheck // Error wrapping handled at top level
360360
}
361361
r.Recorder.Event(&pr, "Normal", constants.PullRequestUpdatedReason, fmt.Sprintf("Pull Request %s updated", pr.Name))
362362
return nil
@@ -379,7 +379,7 @@ func (r *PullRequestReconciler) mergePullRequest(ctx context.Context, pr *promot
379379
pr.Spec.Commit.Message = updatedMessage
380380

381381
if err := provider.Merge(ctx, *pr); err != nil {
382-
return fmt.Errorf("failed to merge pull request: %w", err)
382+
return err //nolint:wrapcheck // Error wrapping handled at top level
383383
}
384384
pr.Status.State = promoterv1alpha1.PullRequestMerged
385385
return nil
@@ -407,7 +407,7 @@ func (r *PullRequestReconciler) closePullRequest(ctx context.Context, pr *promot
407407
return nil
408408
}
409409
if err := provider.Close(ctx, *pr); err != nil {
410-
return fmt.Errorf("failed to close pull request: %w", err)
410+
return err //nolint:wrapcheck // Error wrapping handled at top level
411411
}
412412
pr.Status.State = promoterv1alpha1.PullRequestClosed
413413
return nil

internal/controller/pullrequest_controller_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,60 @@ var _ = Describe("PullRequest Controller", func() {
157157
g.Expect(pullRequest.Status.Conditions[0].Message).To(ContainSubstring("secret from ScmProvider not found"))
158158
}, constants.EventuallyTimeout).Should(Succeed())
159159
})
160+
161+
It("should report merge error without redundant wrapping", func() {
162+
By("Creating a PullRequest with invalid merge SHA to trigger merge failure")
163+
164+
name, scmSecret, scmProvider, gitRepo, pullRequest := pullRequestResources(ctx, "merge-error-message-test")
165+
166+
typeNamespacedName := types.NamespacedName{
167+
Name: name,
168+
Namespace: "default",
169+
}
170+
171+
// Set an invalid merge SHA that won't match the actual source branch HEAD
172+
pullRequest.Spec.MergeSha = "0000000000000000000000000000000000000000"
173+
174+
Expect(k8sClient.Create(ctx, scmSecret)).To(Succeed())
175+
Expect(k8sClient.Create(ctx, scmProvider)).To(Succeed())
176+
Expect(k8sClient.Create(ctx, gitRepo)).To(Succeed())
177+
Expect(k8sClient.Create(ctx, pullRequest)).To(Succeed())
178+
179+
By("Waiting for PullRequest to be created and open")
180+
Eventually(func(g Gomega) {
181+
g.Expect(k8sClient.Get(ctx, typeNamespacedName, pullRequest)).To(Succeed())
182+
g.Expect(pullRequest.Status.State).To(Equal(promoterv1alpha1.PullRequestOpen))
183+
g.Expect(pullRequest.Status.ID).ToNot(BeEmpty())
184+
}, constants.EventuallyTimeout).Should(Succeed())
185+
186+
By("Attempting to merge the PullRequest with invalid SHA")
187+
Eventually(func(g Gomega) {
188+
g.Expect(k8sClient.Get(ctx, typeNamespacedName, pullRequest)).To(Succeed())
189+
pullRequest.Spec.State = promoterv1alpha1.PullRequestMerged
190+
g.Expect(k8sClient.Update(ctx, pullRequest)).To(Succeed())
191+
}, constants.EventuallyTimeout).Should(Succeed())
192+
193+
By("Verifying the error message is not redundantly wrapped")
194+
Eventually(func(g Gomega) {
195+
g.Expect(k8sClient.Get(ctx, typeNamespacedName, pullRequest)).To(Succeed())
196+
g.Expect(pullRequest.Status.Conditions).ToNot(BeEmpty())
197+
g.Expect(meta.IsStatusConditionFalse(pullRequest.Status.Conditions, string(conditions.Ready))).To(BeTrue())
198+
g.Expect(pullRequest.Status.Conditions[0].Reason).To(Equal(string(conditions.ReconciliationError)))
199+
200+
// The error message should contain "Reconciliation failed" and "failed to merge pull request" only once each
201+
message := pullRequest.Status.Conditions[0].Message
202+
g.Expect(message).To(ContainSubstring("Reconciliation failed"))
203+
g.Expect(message).To(ContainSubstring("failed to merge pull request"))
204+
205+
// Count occurrences - should not have redundant wrapping
206+
// The message should be: "Reconciliation failed: failed to merge pull request: <actual error>"
207+
// NOT: "Reconciliation failed: failed to merge pull request: failed to merge pull request: failed to merge pull request: <actual error>"
208+
g.Expect(message).ToNot(ContainSubstring("failed to merge pull request: failed to merge pull request"))
209+
}, constants.EventuallyTimeout).Should(Succeed())
210+
211+
By("Cleaning up the PullRequest")
212+
Expect(k8sClient.Delete(ctx, pullRequest)).To(Succeed())
213+
})
160214
})
161215

162216
Context("When attempting to create a PullRequest with invalid initial state", func() {

internal/scms/fake/pullrequest.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func (pr *PullRequest) Merge(ctx context.Context, pullRequest v1alpha1.PullReque
183183

184184
_, err = pr.runGitCmd(ctx, gitPath, "merge", "--no-ff", "origin/"+pullRequest.Spec.SourceBranch, "-m", pullRequest.Spec.Commit.Message)
185185
if err != nil {
186-
return fmt.Errorf("failed to merge branch: %w", err)
186+
return err
187187
}
188188

189189
_, err = pr.runGitCmd(ctx, gitPath, "push")

internal/scms/forgejo/pullrequest.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func (pr *PullRequest) Create(ctx context.Context, title, head, base, descriptio
6565
metrics.RecordSCMCall(repo, metrics.SCMAPIPullRequest, metrics.SCMOperationCreate, resp.StatusCode, time.Since(start), nil)
6666
}
6767
if err != nil {
68-
return "", fmt.Errorf("failed to create pull request: %w", err)
68+
return "", err //nolint:wrapcheck // Error wrapping handled at top level
6969
}
7070

7171
logger.V(4).Info("forgejo response status", "status", resp.Status)
@@ -100,7 +100,7 @@ func (pr *PullRequest) Update(ctx context.Context, title, description string, pr
100100
metrics.RecordSCMCall(repo, metrics.SCMAPIPullRequest, metrics.SCMOperationCreate, resp.StatusCode, time.Since(start), nil)
101101
}
102102
if err != nil {
103-
return fmt.Errorf("failed to update pull request: %w", err)
103+
return err //nolint:wrapcheck // Error wrapping handled at top level
104104
}
105105

106106
logger.V(4).Info("forgejo response status", "status", resp.Status)
@@ -140,7 +140,7 @@ func (pr *PullRequest) Close(ctx context.Context, prObj promoterv1alpha1.PullReq
140140
metrics.RecordSCMCall(repo, metrics.SCMAPIPullRequest, metrics.SCMOperationCreate, resp.StatusCode, time.Since(start), nil)
141141
}
142142
if err != nil {
143-
return fmt.Errorf("failed to close pull request: %w", err)
143+
return err //nolint:wrapcheck // Error wrapping handled at top level
144144
}
145145

146146
logger.V(4).Info("forgejo response status", "status", resp.Status)
@@ -181,7 +181,7 @@ func (pr *PullRequest) Merge(ctx context.Context, prObj promoterv1alpha1.PullReq
181181
metrics.RecordSCMCall(repo, metrics.SCMAPIPullRequest, metrics.SCMOperationCreate, resp.StatusCode, time.Since(start), nil)
182182
}
183183
if err != nil {
184-
return fmt.Errorf("failed to merge pull request: %w", err)
184+
return err //nolint:wrapcheck // Error wrapping handled at top level
185185
}
186186
logger.V(4).Info("forgejo response status", "status", resp.Status)
187187
return nil

internal/scms/github/pullrequest.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func (pr *PullRequest) Create(ctx context.Context, title, head, base, descriptio
6060
metrics.RecordSCMCall(gitRepo, metrics.SCMAPIPullRequest, metrics.SCMOperationCreate, response.StatusCode, time.Since(start), getRateLimitMetrics(response.Rate))
6161
}
6262
if err != nil {
63-
return "", fmt.Errorf("failed to create pull request: %w", err)
63+
return "", err //nolint:wrapcheck // Error wrapping handled at top level
6464
}
6565
logger.Info("github rate limit",
6666
"limit", response.Rate.Limit,
@@ -134,7 +134,7 @@ func (pr *PullRequest) Close(ctx context.Context, pullRequest v1alpha1.PullReque
134134
metrics.RecordSCMCall(gitRepo, metrics.SCMAPIPullRequest, metrics.SCMOperationClose, response.StatusCode, time.Since(start), getRateLimitMetrics(response.Rate))
135135
}
136136
if err != nil {
137-
return fmt.Errorf("failed to close pull request: %w", err)
137+
return err //nolint:wrapcheck // Error wrapping handled at top level
138138
}
139139
logger.Info("github rate limit",
140140
"limit", response.Rate.Limit,
@@ -176,7 +176,7 @@ func (pr *PullRequest) Merge(ctx context.Context, pullRequest v1alpha1.PullReque
176176
metrics.RecordSCMCall(gitRepo, metrics.SCMAPIPullRequest, metrics.SCMOperationMerge, response.StatusCode, time.Since(start), getRateLimitMetrics(response.Rate))
177177
}
178178
if err != nil {
179-
return fmt.Errorf("failed to merge pull request: %w", err)
179+
return err //nolint:wrapcheck // Error wrapping handled at top level
180180
}
181181
logger.Info("github rate limit",
182182
"limit", response.Rate.Limit,

internal/scms/gitlab/pullrequest.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func (pr *PullRequest) Create(ctx context.Context, title, head, base, desc strin
6666
metrics.RecordSCMCall(repo, metrics.SCMAPIPullRequest, metrics.SCMOperationCreate, resp.StatusCode, time.Since(start), nil)
6767
}
6868
if err != nil {
69-
return "", fmt.Errorf("failed to create pull request: %w", err)
69+
return "", err //nolint:wrapcheck // Error wrapping handled at top level
7070
}
7171

7272
logGitLabRateLimitsIfAvailable(
@@ -212,7 +212,7 @@ func (pr *PullRequest) Merge(ctx context.Context, prObj v1alpha1.PullRequest) er
212212
metrics.RecordSCMCall(repo, metrics.SCMAPIPullRequest, metrics.SCMOperationMerge, resp.StatusCode, time.Since(start), nil)
213213
}
214214
if err != nil {
215-
return fmt.Errorf("failed to merge request: %w", err)
215+
return err //nolint:wrapcheck // Error wrapping handled at top level
216216
}
217217

218218
logGitLabRateLimitsIfAvailable(

0 commit comments

Comments
 (0)