From 4ff53420990544be858066d87e5dd03c9bf913b2 Mon Sep 17 00:00:00 2001 From: ab-ghosh Date: Fri, 28 Nov 2025 19:40:03 +0530 Subject: [PATCH 1/3] fix: skip MR comment for non-permission errors Signed-off-by: ab-ghosh --- pkg/provider/gitlab/gitlab.go | 19 ++++++- pkg/provider/gitlab/gitlab_test.go | 86 ++++++++++++++++++++++++------ 2 files changed, 88 insertions(+), 17 deletions(-) diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index bf9c0cfd9..647393ca1 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -3,6 +3,7 @@ package gitlab import ( "context" "crypto/subtle" + "errors" "fmt" "net/http" "net/url" @@ -319,12 +320,26 @@ func (v *Provider) CreateStatus(_ context.Context, event *info.Event, statusOpts v.Logger.Debugf("created commit status on source project ID %d", event.TargetProjectID) return nil } - if _, _, err2 := v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt); err2 == nil { + _, _, err2 := v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt) + if err2 == nil { v.Logger.Debugf("created commit status on target project ID %d", event.TargetProjectID) // we managed to set the status on the target repo, all good we are done return nil } - v.Logger.Debugf("cannot set status with the GitLab token on the target project: %v", err) + v.Logger.Debugf("cannot set status with the GitLab token on the target project: %v", err2) + + // Only fall back to creating MR comments if either error is a permission issue (401/403). + // For other errors (e.g., state transition errors like "Cannot transition status via :run from :running"), + // the status might already be set, so we should not create a comment. + isPermErr := func(e error) bool { + var errResp *gitlab.ErrorResponse + return errors.As(e, &errResp) && (errResp.Response.StatusCode == http.StatusUnauthorized || errResp.Response.StatusCode == http.StatusForbidden) + } + if !isPermErr(err) && !isPermErr(err2) { + v.Logger.Debugf("skipping MR comment as error is not a permission issue") + return nil + } + // we only show the first error as it's likely something the user has more control to fix // the second err is cryptic as it needs a dummy gitlab pipeline to start // with and will only give more confusion in the event namespace diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index 3afb5d666..33cfc141a 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -261,7 +261,7 @@ func TestCreateStatus(t *testing.T) { }, }, { - name: "commit status fails on both projects but continues", + name: "commit status fails on both projects with non-permission error skips MR comment", wantClient: true, wantErr: false, fields: fields{ @@ -273,10 +273,50 @@ func TestCreateStatus(t *testing.T) { }, event: &info.Event{ TriggerTarget: "pull_request", - SourceProjectID: 404, // Will fail - TargetProjectID: 405, // Will fail + SourceProjectID: 400, // Will fail with 400 (not permission error) + TargetProjectID: 405, // Will fail with 400 (not permission error) SHA: "abc123", }, + postStr: "", // No MR comment expected for non-permission errors + }, + }, + { + name: "permission error 403 on source creates MR comment", + wantClient: true, + wantErr: false, + fields: fields{ + targetProjectID: 100, + }, + args: args{ + statusOpts: provider.StatusOpts{ + Conclusion: "success", + }, + event: &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 403, // Will fail with 403 Forbidden + TargetProjectID: 400, // Will fail with 400 (not permission error) + SHA: "abc123def", + }, + postStr: "has successfully", + }, + }, + { + name: "permission error 401 on source creates MR comment", + wantClient: true, + wantErr: false, + fields: fields{ + targetProjectID: 100, + }, + args: args{ + statusOpts: provider.StatusOpts{ + Conclusion: "success", + }, + event: &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 401, // Will fail with 401 Unauthorized + TargetProjectID: 400, // Will fail with 400 (not permission error) + SHA: "abc123ghi", + }, postStr: "has successfully", }, }, @@ -318,15 +358,23 @@ func TestCreateStatus(t *testing.T) { // Mock source project commit status endpoint sourceStatusPath := fmt.Sprintf("/projects/%d/statuses/%s", tt.args.event.SourceProjectID, tt.args.event.SHA) mux.HandleFunc(sourceStatusPath, func(rw http.ResponseWriter, _ *http.Request) { - if tt.args.event.SourceProjectID == 404 { - // Simulate failure on source project + switch tt.args.event.SourceProjectID { + case 400: + rw.WriteHeader(http.StatusBadRequest) + fmt.Fprint(rw, `{"message": "400 Bad Request"}`) + case 401: + rw.WriteHeader(http.StatusUnauthorized) + fmt.Fprint(rw, `{"message": "401 Unauthorized"}`) + case 403: + rw.WriteHeader(http.StatusForbidden) + fmt.Fprint(rw, `{"message": "403 Forbidden"}`) + case 404: rw.WriteHeader(http.StatusNotFound) fmt.Fprint(rw, `{"message": "404 Project Not Found"}`) - return + default: + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) } - // Success on source project - rw.WriteHeader(http.StatusCreated) - fmt.Fprint(rw, `{}`) }) } @@ -334,15 +382,23 @@ func TestCreateStatus(t *testing.T) { // Mock target project commit status endpoint targetStatusPath := fmt.Sprintf("/projects/%d/statuses/%s", tt.args.event.TargetProjectID, tt.args.event.SHA) mux.HandleFunc(targetStatusPath, func(rw http.ResponseWriter, _ *http.Request) { - if tt.args.event.TargetProjectID == 404 { - // Simulate failure on target project + switch tt.args.event.TargetProjectID { + case 400, 405: + rw.WriteHeader(http.StatusBadRequest) + fmt.Fprint(rw, `{"message": "400 Bad Request"}`) + case 401: + rw.WriteHeader(http.StatusUnauthorized) + fmt.Fprint(rw, `{"message": "401 Unauthorized"}`) + case 403: + rw.WriteHeader(http.StatusForbidden) + fmt.Fprint(rw, `{"message": "403 Forbidden"}`) + case 404: rw.WriteHeader(http.StatusNotFound) fmt.Fprint(rw, `{"message": "404 Project Not Found"}`) - return + default: + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) } - // Success on target project - rw.WriteHeader(http.StatusCreated) - fmt.Fprint(rw, `{}`) }) } From 132ebe4400dca53c7bce3610f7570d9eaf65149b Mon Sep 17 00:00:00 2001 From: ab-ghosh Date: Fri, 28 Nov 2025 22:49:23 +0530 Subject: [PATCH 2/3] chore: retrigger CI From bf0d4d9cd9341d2897e262b1683aefe4f0d3520f Mon Sep 17 00:00:00 2001 From: ab-ghosh Date: Tue, 9 Dec 2025 16:08:45 +0530 Subject: [PATCH 3/3] fix(gitlab): skip MR comment for state transition errors Signed-off-by: ab-ghosh --- pkg/provider/gitlab/gitlab.go | 17 ++++++------ pkg/provider/gitlab/gitlab_test.go | 42 ++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 647393ca1..fd69b5540 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -3,7 +3,6 @@ package gitlab import ( "context" "crypto/subtle" - "errors" "fmt" "net/http" "net/url" @@ -328,15 +327,15 @@ func (v *Provider) CreateStatus(_ context.Context, event *info.Event, statusOpts } v.Logger.Debugf("cannot set status with the GitLab token on the target project: %v", err2) - // Only fall back to creating MR comments if either error is a permission issue (401/403). - // For other errors (e.g., state transition errors like "Cannot transition status via :run from :running"), - // the status might already be set, so we should not create a comment. - isPermErr := func(e error) bool { - var errResp *gitlab.ErrorResponse - return errors.As(e, &errResp) && (errResp.Response.StatusCode == http.StatusUnauthorized || errResp.Response.StatusCode == http.StatusForbidden) + // Skip creating MR comments if the error is a state transition error + // (e.g., "Cannot transition status via :run from :running"). + // This means the status is already set, so we should not create a comment. + // For other errors (permission issues, network errors, etc.), we fall back to comments. + isTransitionErr := func(e error) bool { + return e != nil && strings.Contains(e.Error(), "Cannot transition status") } - if !isPermErr(err) && !isPermErr(err2) { - v.Logger.Debugf("skipping MR comment as error is not a permission issue") + if isTransitionErr(err) || isTransitionErr(err2) { + v.Logger.Debugf("skipping MR comment as error is a state transition issue (status already set)") return nil } diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index 33cfc141a..ca6fc70fa 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -261,7 +261,7 @@ func TestCreateStatus(t *testing.T) { }, }, { - name: "commit status fails on both projects with non-permission error skips MR comment", + name: "commit status fails with state transition error skips MR comment", wantClient: true, wantErr: false, fields: fields{ @@ -273,11 +273,31 @@ func TestCreateStatus(t *testing.T) { }, event: &info.Event{ TriggerTarget: "pull_request", - SourceProjectID: 400, // Will fail with 400 (not permission error) - TargetProjectID: 405, // Will fail with 400 (not permission error) + SourceProjectID: 422, // Will fail with "Cannot transition status" error + TargetProjectID: 423, // Will fail with "Cannot transition status" error SHA: "abc123", }, - postStr: "", // No MR comment expected for non-permission errors + postStr: "", // No MR comment expected for state transition errors + }, + }, + { + name: "generic error on both projects creates MR comment", + wantClient: true, + wantErr: false, + fields: fields{ + targetProjectID: 100, + }, + args: args{ + statusOpts: provider.StatusOpts{ + Conclusion: "success", + }, + event: &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 400, // Will fail with generic 400 error + TargetProjectID: 405, // Will fail with generic 400 error + SHA: "abc123def", + }, + postStr: "has successfully", // MR comment expected for non-transition errors }, }, { @@ -294,8 +314,8 @@ func TestCreateStatus(t *testing.T) { event: &info.Event{ TriggerTarget: "pull_request", SourceProjectID: 403, // Will fail with 403 Forbidden - TargetProjectID: 400, // Will fail with 400 (not permission error) - SHA: "abc123def", + TargetProjectID: 400, // Will fail with generic 400 error + SHA: "abc123ghi", }, postStr: "has successfully", }, @@ -314,8 +334,8 @@ func TestCreateStatus(t *testing.T) { event: &info.Event{ TriggerTarget: "pull_request", SourceProjectID: 401, // Will fail with 401 Unauthorized - TargetProjectID: 400, // Will fail with 400 (not permission error) - SHA: "abc123ghi", + TargetProjectID: 400, // Will fail with generic 400 error + SHA: "abc123jkl", }, postStr: "has successfully", }, @@ -371,6 +391,9 @@ func TestCreateStatus(t *testing.T) { case 404: rw.WriteHeader(http.StatusNotFound) fmt.Fprint(rw, `{"message": "404 Project Not Found"}`) + case 422: + rw.WriteHeader(http.StatusBadRequest) + fmt.Fprint(rw, `{"message": "Cannot transition status via :run from :running"}`) default: rw.WriteHeader(http.StatusCreated) fmt.Fprint(rw, `{}`) @@ -395,6 +418,9 @@ func TestCreateStatus(t *testing.T) { case 404: rw.WriteHeader(http.StatusNotFound) fmt.Fprint(rw, `{"message": "404 Project Not Found"}`) + case 423: + rw.WriteHeader(http.StatusBadRequest) + fmt.Fprint(rw, `{"message": "Cannot transition status via :run from :running"}`) default: rw.WriteHeader(http.StatusCreated) fmt.Fprint(rw, `{}`)