diff --git a/pkg/apis/pipelinesascode/keys/keys.go b/pkg/apis/pipelinesascode/keys/keys.go index ef25cce505..64b9cf9294 100644 --- a/pkg/apis/pipelinesascode/keys/keys.go +++ b/pkg/apis/pipelinesascode/keys/keys.go @@ -48,6 +48,7 @@ const ( OriginalPRName = pipelinesascode.GroupName + "/original-prname" GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret" CheckRunID = pipelinesascode.GroupName + "/check-run-id" + GitLabPipelineID = pipelinesascode.GroupName + "/gitlab-pipeline-id" OnEvent = pipelinesascode.GroupName + "/on-event" OnComment = pipelinesascode.GroupName + "/on-comment" OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch" diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 09da6e4593..e9bde16827 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -12,6 +12,8 @@ import ( "strconv" "strings" + "github.com/openshift-pipelines/pipelines-as-code/pkg/action" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/changedfiles" "github.com/openshift-pipelines/pipelines-as-code/pkg/events" @@ -354,6 +356,18 @@ func (v *Provider) CreateStatus(ctx context.Context, event *info.Event, statusOp Context: gitlab.Ptr(contextName), } + // Read a previously stored pipeline ID from PipelineRun annotations so + // that all commit statuses land in the same GitLab pipeline instead of + // GitLab potentially auto-creating a new "external" pipeline mid-stream. + if statusOpts.PipelineRun != nil { + if id, ok := statusOpts.PipelineRun.GetAnnotations()[keys.GitLabPipelineID]; ok { + pid, err := strconv.ParseInt(id, 10, 64) + if err == nil { + opt.PipelineID = gitlab.Ptr(pid) + } + } + } + // 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 @@ -361,15 +375,23 @@ func (v *Provider) CreateStatus(ctx context.Context, event *info.Event, statusOp // 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, - _, _, err := v.Client().Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt) + commitStatus, _, err := v.Client().Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt) if err != nil { v.Logger.Debugf("cannot set status with the GitLab token on the source project: %v", err) } else { + v.patchPipelineIDAnnotation(ctx, statusOpts, commitStatus) // we managed to set the status on the source repo, all good we are done v.Logger.Debugf("created commit status on source project ID %d", event.TargetProjectID) return nil } - if _, _, err = v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt); err == nil { + // Clear pipeline ID when falling back to the target project — the cached + // ID belongs to the source project's pipeline namespace and is invalid on + // a different project (fork MR scenario). + if event.SourceProjectID != event.TargetProjectID { + opt.PipelineID = nil + } + if commitStatus, _, err = v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt); err == nil { + v.patchPipelineIDAnnotation(ctx, statusOpts, commitStatus) 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 @@ -860,3 +882,32 @@ func (v *Provider) formatPipelineComment(sha string, status providerstatus.Statu return fmt.Sprintf("%s **%s: %s/%s for %s**\n\n%s\n\nFull log available [here](%s)", emoji, status.Title, v.pacInfo.ApplicationName, status.OriginalPipelineRunName, sha, status.Text, status.DetailsURL) } + +// patchPipelineIDAnnotation stores the GitLab pipeline ID from a successful +// SetCommitStatus response as a PipelineRun annotation. This allows the +// reconciler (which creates a new Provider instance) to read it back and +// pass it on subsequent status updates, keeping all statuses in the same +// GitLab pipeline. +func (v *Provider) patchPipelineIDAnnotation(ctx context.Context, statusOpts providerstatus.StatusOpts, cs *gitlab.CommitStatus) { + if cs == nil || cs.PipelineID == 0 { + return + } + pr := statusOpts.PipelineRun + if pr == nil || (pr.GetName() == "" && pr.GetGenerateName() == "") { + return + } + // Skip if annotation is already set — avoid unnecessary patches. + if _, ok := pr.GetAnnotations()[keys.GitLabPipelineID]; ok { + return + } + mergePatch := map[string]any{ + "metadata": map[string]any{ + "annotations": map[string]string{ + keys.GitLabPipelineID: strconv.FormatInt(cs.PipelineID, 10), + }, + }, + } + if _, err := action.PatchPipelineRun(ctx, v.Logger, "gitlabPipelineID", v.run.Clients.Tekton, pr, mergePatch); err != nil { + v.Logger.Debugf("failed to patch pipelinerun with gitlab pipeline ID: %v", err) + } +} diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index 9ede25241e..31c2bc8d2d 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/events" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" @@ -25,6 +26,8 @@ import ( providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" + tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" gitlab "gitlab.com/gitlab-org/api/client-go" "go.opentelemetry.io/otel" @@ -441,6 +444,306 @@ func TestCreateStatus(t *testing.T) { } } +func TestCreateStatusPipelineIDAnnotation(t *testing.T) { + tests := []struct { + name string + prAnnotations map[string]string + wantPipelineID bool + expectedPID int64 + responsePID int64 + wantAnnotationPID string + }{ + { + name: "first call patches annotation with pipeline ID from response", + prAnnotations: map[string]string{}, + wantPipelineID: false, + responsePID: 9999, + wantAnnotationPID: "9999", + }, + { + name: "subsequent call reads pipeline ID from annotation", + prAnnotations: map[string]string{keys.GitLabPipelineID: "9999"}, + wantPipelineID: true, + expectedPID: 9999, + responsePID: 9999, + wantAnnotationPID: "9999", + }, + { + name: "pipeline ID 0 in response does not patch annotation", + prAnnotations: map[string]string{}, + wantPipelineID: false, + responsePID: 0, + wantAnnotationPID: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{}) + run := ¶ms.Run{ + Clients: clients.Clients{ + Kube: stdata.Kube, + Tekton: stdata.Pipeline, + Log: log, + }, + } + + client, mux, tearDown := thelp.Setup(t) + defer tearDown() + + v := &Provider{ + run: run, + Logger: log, + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + ApplicationName: settings.PACApplicationNameDefaultValue, + }, + }, + eventEmitter: events.NewEventEmitter(run.Clients.Kube, log), + } + v.SetGitLabClient(client) + + event := &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: 500, + TargetProjectID: 500, + SHA: "deadbeef", + PullRequestNumber: 1, + } + + mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", event.SourceProjectID, event.SHA), + func(rw http.ResponseWriter, r *http.Request) { + var reqBody map[string]any + if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil { + t.Fatalf("failed to decode request body: %v", err) + } + if tt.wantPipelineID { + pid, ok := reqBody["pipeline_id"] + assert.Assert(t, ok, "request must include pipeline_id") + pidFloat, ok := pid.(float64) + assert.Assert(t, ok, "pipeline_id must be a number") + assert.Equal(t, int64(pidFloat), tt.expectedPID) + } else { + _, hasPipelineID := reqBody["pipeline_id"] + assert.Assert(t, !hasPipelineID, + "request should not have pipeline_id") + } + rw.WriteHeader(http.StatusCreated) + fmt.Fprintf(rw, `{"id": 1, "pipeline_id": %d}`, tt.responsePID) + }) + + statusOpts := providerstatus.StatusOpts{ + Conclusion: "success", + OriginalPipelineRunName: "pr-1", + PipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pr", + Namespace: "default", + Annotations: tt.prAnnotations, + }, + }, + } + + err := v.CreateStatus(ctx, event, statusOpts) + assert.NilError(t, err) + }) + } +} + +func TestCreateStatusPipelineIDAnnotationTargetFallback(t *testing.T) { + tests := []struct { + name string + sourceProjectID int64 + targetProjectID int64 + prAnnotations map[string]string + wantTargetPID bool + expectedTargetPID int64 + targetResponsePID int64 + }{ + { + name: "source fails and target caches pipeline ID without cross-project leak", + sourceProjectID: 404, + targetProjectID: 600, + prAnnotations: map[string]string{}, + wantTargetPID: false, + targetResponsePID: 7777, + }, + { + name: "source fails with existing annotation and pipeline ID is cleared for different target project", + sourceProjectID: 404, + targetProjectID: 600, + prAnnotations: map[string]string{keys.GitLabPipelineID: "1234"}, + wantTargetPID: false, + targetResponsePID: 7777, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{}) + run := ¶ms.Run{ + Clients: clients.Clients{ + Kube: stdata.Kube, + Tekton: stdata.Pipeline, + Log: log, + }, + } + + client, mux, tearDown := thelp.Setup(t) + defer tearDown() + + v := &Provider{ + targetProjectID: tt.targetProjectID, + run: run, + Logger: log, + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + ApplicationName: settings.PACApplicationNameDefaultValue, + }, + }, + eventEmitter: events.NewEventEmitter(run.Clients.Kube, log), + } + v.SetGitLabClient(client) + + event := &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: tt.sourceProjectID, + TargetProjectID: tt.targetProjectID, + SHA: "aabb1122", + PullRequestNumber: 1, + } + + // Source returns 404 + mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", tt.sourceProjectID, event.SHA), + func(rw http.ResponseWriter, _ *http.Request) { + rw.WriteHeader(http.StatusNotFound) + fmt.Fprint(rw, `{"message": "404 Not Found"}`) + }) + + mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", tt.targetProjectID, event.SHA), + func(rw http.ResponseWriter, r *http.Request) { + var reqBody map[string]any + if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil { + t.Fatalf("failed to decode request body: %v", err) + } + if tt.wantTargetPID { + pid, ok := reqBody["pipeline_id"] + assert.Assert(t, ok, "target request must include pipeline_id") + pidFloat, ok := pid.(float64) + assert.Assert(t, ok, "pipeline_id must be a number") + assert.Equal(t, int64(pidFloat), tt.expectedTargetPID) + } else { + _, hasPipelineID := reqBody["pipeline_id"] + assert.Assert(t, !hasPipelineID, + "target request should not have pipeline_id") + } + rw.WriteHeader(http.StatusCreated) + fmt.Fprintf(rw, `{"id": 1, "pipeline_id": %d}`, tt.targetResponsePID) + }) + + statusOpts := providerstatus.StatusOpts{ + Conclusion: "success", + OriginalPipelineRunName: "pr-1", + PipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pr", + Namespace: "default", + Annotations: tt.prAnnotations, + }, + }, + } + + err := v.CreateStatus(ctx, event, statusOpts) + assert.NilError(t, err) + }) + } +} + +// TestCreateStatusPipelineIDAnnotationSameProjectFallback tests the scenario +// where source and target are the same project, the source call fails, and +// the target call should still see the annotation's pipeline ID since no +// cross-project clearing is needed. +func TestCreateStatusPipelineIDAnnotationSameProjectFallback(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{}) + run := ¶ms.Run{ + Clients: clients.Clients{ + Kube: stdata.Kube, + Tekton: stdata.Pipeline, + Log: log, + }, + } + + client, mux, tearDown := thelp.Setup(t) + defer tearDown() + + projectID := int64(600) + v := &Provider{ + targetProjectID: projectID, + run: run, + Logger: log, + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + ApplicationName: settings.PACApplicationNameDefaultValue, + }, + }, + eventEmitter: events.NewEventEmitter(run.Clients.Kube, log), + } + v.SetGitLabClient(client) + + event := &info.Event{ + TriggerTarget: "pull_request", + SourceProjectID: projectID, + TargetProjectID: projectID, + SHA: "aabb1122", + PullRequestNumber: 1, + } + + // Source == target, so both calls hit the same URL. + // First call (source) fails with 403, second call (target) succeeds. + var callCount int + mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", projectID, event.SHA), + func(rw http.ResponseWriter, r *http.Request) { + callCount++ + if callCount == 1 { + rw.WriteHeader(http.StatusForbidden) + fmt.Fprint(rw, `{"message": "403 Forbidden"}`) + return + } + var reqBody map[string]any + if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil { + t.Fatalf("failed to decode request body: %v", err) + } + // Same project — pipeline ID from annotation should be preserved + pid, ok := reqBody["pipeline_id"] + assert.Assert(t, ok, "target request must include pipeline_id when same project") + pidFloat, ok := pid.(float64) + assert.Assert(t, ok, "pipeline_id must be a number") + assert.Equal(t, int64(pidFloat), int64(5555)) + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{"id": 1, "pipeline_id": 5555}`) + }) + + statusOpts := providerstatus.StatusOpts{ + Conclusion: "success", + OriginalPipelineRunName: "pr-1", + PipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pr", + Namespace: "default", + Annotations: map[string]string{keys.GitLabPipelineID: "5555"}, + }, + }, + } + + err := v.CreateStatus(ctx, event, statusOpts) + assert.NilError(t, err) + assert.Equal(t, callCount, 2) +} + func TestGetCommitInfo(t *testing.T) { tests := []struct { name string