diff --git a/docs/content/docs/guide/repositorycrd.md b/docs/content/docs/guide/repositorycrd.md index 3e19397419..561701b816 100644 --- a/docs/content/docs/guide/repositorycrd.md +++ b/docs/content/docs/guide/repositorycrd.md @@ -2,6 +2,7 @@ title: Repository CR weight: 1 --- + # Repository CR The Repository CR serves the following purposes: @@ -127,7 +128,29 @@ access to the infrastructure. ## Disabling all comments for PipelineRuns on GitLab MR -`comment_strategy` allows you to disable the comments on GitLab MR for a Repository +By default, Pipelines-as-Code attempts to update the commit status through the +GitLab API. It first tries the source project (fork), then falls back to the +target project (upstream repository). The source project update succeeds when +the configured token has access to the source repository and GitLab creates +pipeline entries for external CI systems like Pipelines-as-Code. The target +project fallback may fail if there's no CI pipeline running for that commit +in the target repository, since GitLab only creates pipeline entries for commits +that actually trigger CI in that specific project. If either status update +succeeds, no comment is posted on the Merge Request. + +When a status update succeeds, you can see the status in the GitLab UI in the `Pipelines` tab, as +shown in the following example: + +![Gitlab Pipelines from Pipelines-as-Code](/images/gitlab-pipelines-tab.png) + +Comments are only posted when: + +- Both commit status updates fail (typically due to insufficient token permissions) +- The event type and repository settings allow commenting +- The `comment_strategy` is not set to `disable_all` + +To explicitly disable all comments on GitLab Merge Requests for a Repository, +use the `comment_strategy` setting: ```yaml spec: @@ -136,8 +159,6 @@ spec: comment_strategy: "disable_all" ``` -When you set the value of `comment_strategy` to `disable_all` it will not add any comment on the merge request for the start and the end of PipelineRun - ## Disabling all comments for PipelineRuns in GitHub Pull Requests on GitHub Webhook Setup `comment_strategy` allows you to disable the comments on GitHub PR for a Repository diff --git a/docs/static/images/gitlab-pipelines-tab.png b/docs/static/images/gitlab-pipelines-tab.png new file mode 100644 index 0000000000..8d85f72291 Binary files /dev/null and b/docs/static/images/gitlab-pipelines-tab.png differ diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 5d62c6dbfe..7b25b40f16 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -280,17 +280,27 @@ func (v *Provider) CreateStatus(_ context.Context, event *info.Event, statusOpts Context: gitlab.Ptr(contextName), } - // In case we have access, set the status. Typically, on a Merge Request (MR) - // from a fork in an upstream repository, the token needs to have write access - // to the fork repository in order to create a status. However, the token set on the - // Repository CR usually doesn't have such broad access, preventing from creating - // a status comment on it. - // This would work on a push or an MR from a branch within the same repo. - // Ignoring errors because of the write access issues, - if _, _, err := v.Client().Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt); err != nil { - v.eventEmitter.EmitMessage(v.repo, zap.ErrorLevel, "FailedToSetCommitStatus", - "cannot set status with the GitLab token because of: "+err.Error()) + // setCommitStatus attempts to set the commit status for a given SHA, handling GitLab's permission model. + // First tries the source project (fork), then falls back to the target project (upstream repo). + // This fallback is necessary because: + // - In fork/MR scenarios, the GitLab token may not have write access to the contributor's fork + // - This ensures commit status can be displayed somewhere useful regardless + // of permission differences + // Returns nil if status is set on either project, logs error if both attempts fail. + _, _, err := v.Client().Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt) + if err == nil { + v.Logger.Debugf("created commit status on source project ID %d", event.SourceProjectID) + return nil + } + if _, _, err2 := v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt); err2 == nil { + v.Logger.Debugf("created commit status on target project ID %d", event.TargetProjectID) + return nil } + v.Logger.Debugf( + "Failed to create commit status: source project ID %d, target project ID %d. "+ + "If you want Gitlab Pipeline Status update, ensure your GitLab token has access "+ + "to the source repository. Error: %v", + event.SourceProjectID, event.TargetProjectID, err) eventType := triggertype.IsPullRequestType(event.EventType) // When a GitOps command is sent on a pushed commit, it mistakenly treats it as a pull_request diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index aff2b3251d..ab60a63dcb 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -201,6 +201,66 @@ func TestCreateStatus(t *testing.T) { postStr: "has completed", }, }, + { + name: "commit status success on source project", + wantClient: true, + wantErr: false, + fields: fields{ + targetProjectID: 100, + }, + args: args{ + statusOpts: provider.StatusOpts{ + Conclusion: "success", + }, + event: &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 200, + TargetProjectID: 100, + SHA: "abc123", + }, + postStr: "has successfully", + }, + }, + { + name: "commit status falls back to target project", + wantClient: true, + wantErr: false, + fields: fields{ + targetProjectID: 100, + }, + args: args{ + statusOpts: provider.StatusOpts{ + Conclusion: "success", + }, + event: &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 404, // Will fail to find this project + TargetProjectID: 100, + SHA: "abc123", + }, + postStr: "has successfully", + }, + }, + { + name: "commit status fails on both projects but continues", + wantClient: true, + wantErr: false, + fields: fields{ + targetProjectID: 100, + }, + args: args{ + statusOpts: provider.StatusOpts{ + Conclusion: "success", + }, + event: &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 404, // Will fail + TargetProjectID: 405, // Will fail + SHA: "abc123", + }, + postStr: "has successfully", + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -216,6 +276,7 @@ func TestCreateStatus(t *testing.T) { v := &Provider{ targetProjectID: tt.fields.targetProjectID, run: params.New(), + Logger: logger, pacInfo: &info.PacOpts{ Settings: settings.Settings{ ApplicationName: settings.PACApplicationNameDefaultValue, @@ -232,6 +293,40 @@ func TestCreateStatus(t *testing.T) { client, mux, tearDown := thelp.Setup(t) v.SetGitLabClient(client) defer tearDown() + + // Mock commit status endpoints for both source and target projects + if tt.args.event.SourceProjectID != 0 { + // 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 + rw.WriteHeader(http.StatusNotFound) + fmt.Fprint(rw, `{"message": "404 Project Not Found"}`) + return + } + // Success on source project + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) + }) + } + + if tt.args.event.TargetProjectID != 0 { + // 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 + rw.WriteHeader(http.StatusNotFound) + fmt.Fprint(rw, `{"message": "404 Project Not Found"}`) + return + } + // Success on target project + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) + }) + } + thelp.MuxNotePost(t, mux, v.targetProjectID, tt.args.event.PullRequestNumber, tt.args.postStr) } @@ -1044,9 +1139,12 @@ func TestGitLabCreateComment(t *testing.T) { t.Run(tt.name, func(t *testing.T) { fakeclient, mux, teardown := thelp.Setup(t) defer teardown() + observer, _ := zapobserver.New(zap.InfoLevel) + logger := zap.New(observer).Sugar() if tt.clientNil { p := &Provider{ + Logger: logger, sourceProjectID: 666, } err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker) diff --git a/test/gitlab_merge_request_test.go b/test/gitlab_merge_request_test.go index 2d4fad838d..ee253fb603 100644 --- a/test/gitlab_merge_request_test.go +++ b/test/gitlab_merge_request_test.go @@ -20,7 +20,6 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx" tgitlab "github.com/openshift-pipelines/pipelines-as-code/test/pkg/gitlab" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload" - "github.com/openshift-pipelines/pipelines-as-code/test/pkg/repository" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/scm" twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" @@ -29,6 +28,7 @@ import ( "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" "knative.dev/pkg/apis" ) @@ -104,6 +104,17 @@ func TestGitlabMergeRequest(t *testing.T) { assert.Equal(t, "Merge Request", prsNew.Items[i].Annotations[keys.EventType]) } + // Get the MR to fetch the SHA + mr, _, err := glprovider.Client().MergeRequests.GetMergeRequest(opts.ProjectID, mrID, nil) + assert.NilError(t, err) + + // Check GitLab pipelines via API - should have 1 pipeline from normal MR processing + pipelines, _, err := glprovider.Client().Pipelines.ListProjectPipelines(opts.ProjectID, &clientGitlab.ListProjectPipelinesOptions{ + SHA: &mr.SHA, + }) + assert.NilError(t, err) + assert.Assert(t, len(pipelines) == 1, "Expected 1 GitLab pipeline from normal MR processing, got %d", len(pipelines)) + runcnx.Clients.Log.Infof("Sending /test comment on MergeRequest %s/-/merge_requests/%d", projectinfo.WebURL, mrID) _, _, err = glprovider.Client().Notes.CreateMergeRequestNote(opts.ProjectID, mrID, &clientGitlab.CreateMergeRequestNoteOptions{ Body: clientGitlab.Ptr("/test"), @@ -115,21 +126,9 @@ func TestGitlabMergeRequest(t *testing.T) { OnEvent: opscomments.TestAllCommentEventType.String(), TargetNS: targetNS, NumberofPRMatch: 5, // this is the max we get in repos status - SHA: "", + SHA: mr.SHA, } - runcnx.Clients.Log.Info("Checking that PAC has posted successful comments for all PR that has been tested") twait.Succeeded(ctx, t, runcnx, opts, sopt) - - notes, _, err := glprovider.Client().Notes.ListMergeRequestNotes(opts.ProjectID, mrID, nil) - assert.NilError(t, err) - successCommentsPost := 0 - for _, n := range notes { - if successRegexp.MatchString(n.Body) { - successCommentsPost++ - } - } - // we get 2 PRS initially, 2 prs from the push update and 2 prs from the /test == 6 - assert.Equal(t, 6, successCommentsPost) } func TestGitlabOnLabel(t *testing.T) { @@ -500,7 +499,7 @@ func TestGitlabIssueGitopsComment(t *testing.T) { twait.Succeeded(ctx, t, runcnx, opts, sopt) } -func TestGitlabDisableCommentsOnMR(t *testing.T) { +func TestGitlabDisableCommentsOnMRNotCreated(t *testing.T) { targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns") ctx := context.Background() runcnx, opts, glprovider, err := tgitlab.Setup(ctx) @@ -510,7 +509,7 @@ func TestGitlabDisableCommentsOnMR(t *testing.T) { projectinfo, resp, err := glprovider.Client().Projects.GetProject(opts.ProjectID, nil) assert.NilError(t, err) if resp != nil && resp.StatusCode == http.StatusNotFound { - t.Errorf("Repository %s not found in %s", opts.Organization, opts.Repo) + t.Fatalf("Repository %s not found in %s", opts.Organization, opts.Repo) // Use Fatalf to stop test on critical error } settings := v1alpha1.Settings{ @@ -541,9 +540,10 @@ func TestGitlabDisableCommentsOnMR(t *testing.T) { TargetRefName: targetRefName, BaseRefName: projectinfo.DefaultBranch, } - _ = scm.PushFilesToRefGit(t, scmOpts, entries) + // NEW: Capture the commit SHA returned by the push operation. + sha := scm.PushFilesToRefGit(t, scmOpts, entries) + runcnx.Clients.Log.Infof("Commit %s has been created and pushed to branch %s", sha, targetRefName) - runcnx.Clients.Log.Infof("Branch %s has been created and pushed with files", targetRefName) mrTitle := "TestMergeRequest - " + targetRefName mrID, err := tgitlab.CreateMR(glprovider.Client(), opts.ProjectID, targetRefName, projectinfo.DefaultBranch, mrTitle) assert.NilError(t, err) @@ -555,13 +555,60 @@ func TestGitlabDisableCommentsOnMR(t *testing.T) { OnEvent: "Merge Request", TargetNS: targetNS, NumberofPRMatch: 1, - SHA: "", + SHA: sha, // NEW: Pass the captured SHA to ensure we wait for the correct PipelineRun } twait.Succeeded(ctx, t, runcnx, opts, sopt) prsNew, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{}) assert.NilError(t, err) assert.Assert(t, len(prsNew.Items) == 1) + runcnx.Clients.Log.Infof("Checking status of GitLab pipeline for commit: %s", sha) + // Define constants for polling + const pipelineCheckTimeout = 5 * time.Minute + const pipelineCheckInterval = 10 * time.Second + + var pipeline *clientGitlab.Pipeline + // Use a polling mechanism to wait for the pipeline to succeed. + err = wait.PollUntilContextTimeout(ctx, pipelineCheckInterval, pipelineCheckTimeout, true, func(_ context.Context) (bool, error) { + // Find the pipeline associated with our specific commit SHA + pipelines, _, listErr := glprovider.Client().Pipelines.ListProjectPipelines(opts.ProjectID, &clientGitlab.ListProjectPipelinesOptions{ + SHA: &sha, + }) + if listErr != nil { + return false, listErr // Propagate API errors + } + if len(pipelines) == 0 { + runcnx.Clients.Log.Info("Waiting for pipeline to be created...") + return false, nil // Pipeline not found yet, continue polling + } + if len(pipelines) > 1 { + // This is unexpected, fail fast + return false, fmt.Errorf("expected 1 pipeline for SHA %s, but found %d", sha, len(pipelines)) + } + + // Get the latest status of our specific pipeline + p, _, getErr := glprovider.Client().Pipelines.GetPipeline(opts.ProjectID, pipelines[0].ID) + if getErr != nil { + return false, getErr + } + + runcnx.Clients.Log.Infof("Current pipeline status: %s", p.Status) + switch p.Status { + case "success": + pipeline = p + return true, nil // Success! Stop polling. + case "failed", "canceled", "skipped": + // The pipeline has finished but not successfully. + return false, fmt.Errorf("pipeline finished with non-success status: %s", p.Status) + default: + // The pipeline is still running or pending, continue polling. + return false, nil + } + }) + assert.NilError(t, err, "failed while waiting for GitLab pipeline to succeed") + assert.Equal(t, "success", pipeline.Status, "The final pipeline status was not 'success'") + runcnx.Clients.Log.Infof("✅ GitLab pipeline ID %d has succeeded!", pipeline.ID) + // No comments will be added related to pipelineruns info notes, _, err := glprovider.Client().Notes.ListMergeRequestNotes(opts.ProjectID, mrID, nil) @@ -574,46 +621,8 @@ func TestGitlabDisableCommentsOnMR(t *testing.T) { } } // Since Gitlab comment strategy is disabled, - // no comments will be posted related to pipelineruns + // no comments will be posted related to PipelineRuns assert.Equal(t, 0, successCommentsPost) - - // Update the repo setting to nil, and comment /test to restart the pipelinerun - // and now comments will be added on the MR - waitOpts := twait.Opts{ - RepoName: targetNS, - Namespace: targetNS, - TargetSHA: "", - } - - runcnx.Clients.Log.Info("Updating Gitlab Comment Strategy to nil...") - err = repository.UpdateRepo(ctx, waitOpts.Namespace, waitOpts.RepoName, runcnx.Clients) - assert.NilError(t, err) - - runcnx.Clients.Log.Infof("Sending /test comment on MergeRequest %s/-/merge_requests/%d", projectinfo.WebURL, mrID) - _, _, err = glprovider.Client().Notes.CreateMergeRequestNote(opts.ProjectID, mrID, &clientGitlab.CreateMergeRequestNoteOptions{ - Body: clientGitlab.Ptr("/test"), - }) - assert.NilError(t, err) - - sopt = twait.SuccessOpt{ - Title: commitTitle, - OnEvent: opscomments.TestAllCommentEventType.String(), - TargetNS: targetNS, - NumberofPRMatch: 2, // this is the max we get in repos status - SHA: "", - } - runcnx.Clients.Log.Info("Checking that PAC has posted successful comments for all PR that has been tested") - twait.Succeeded(ctx, t, runcnx, opts, sopt) - - notes, _, err = glprovider.Client().Notes.ListMergeRequestNotes(opts.ProjectID, mrID, nil) - assert.NilError(t, err) - successCommentsPost = 0 - for _, n := range notes { - if commentRegexp.MatchString(n.Body) { - successCommentsPost++ - } - } - assert.Equal(t, 2, successCommentsPost) } func TestGitlabMergeRequestOnUpdateAtAndLabelChange(t *testing.T) { diff --git a/test/pkg/scm/scm.go b/test/pkg/scm/scm.go index fe50bb0f4f..04bd662ccf 100644 --- a/test/pkg/scm/scm.go +++ b/test/pkg/scm/scm.go @@ -130,7 +130,7 @@ func PushFilesToRefGit(t *testing.T, opts *Opts, entries map[string]string) stri assert.NilError(t, err) gitPushPullRetry(t, opts, path) - return sha + return strings.TrimSpace(sha) } func ChangeFilesRefGit(t *testing.T, opts *Opts, fileChanges []FileChange) {