diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index bf9c0cfd9..fd69b5540 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -319,12 +319,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) + + // 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 isTransitionErr(err) || isTransitionErr(err2) { + v.Logger.Debugf("skipping MR comment as error is a state transition issue (status already set)") + 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..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 but continues", + name: "commit status fails with state transition error skips MR comment", wantClient: true, wantErr: false, fields: fields{ @@ -273,10 +273,70 @@ func TestCreateStatus(t *testing.T) { }, event: &info.Event{ TriggerTarget: "pull_request", - SourceProjectID: 404, // Will fail - TargetProjectID: 405, // Will fail + 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 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 + }, + }, + { + 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 generic 400 error + SHA: "abc123ghi", + }, + 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 generic 400 error + SHA: "abc123jkl", + }, postStr: "has successfully", }, }, @@ -318,15 +378,26 @@ 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 + 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, `{}`) } - // Success on source project - rw.WriteHeader(http.StatusCreated) - fmt.Fprint(rw, `{}`) }) } @@ -334,15 +405,26 @@ 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 + 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, `{}`) } - // Success on target project - rw.WriteHeader(http.StatusCreated) - fmt.Fprint(rw, `{}`) }) }