From a77f07f4be1aeb99df0fc1a441f3289f2372d7d7 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Sun, 12 Oct 2025 19:35:02 +0530 Subject: [PATCH] fix: prevent stale failure statuses on commits Ensures a consistent context key is used for all GitLab commit statuses, allowing failures from invalid PipelineRuns to be correctly overwritten on subsequent runs. The Problem: When a repository contained multiple PipelineRun definitions and one was invalid (e.g., due to a validation error), the initial commit would correctly report a "failed" status to GitLab for that invalid run. However, the context key for this failure was generic (e.g., "ApplicationName") because a full PipelineRun wasn't provided. After a developer fixed the invalid PipelineRun and pushed a new commit, the now-successful run would post its status with a more specific context key (e.g., "ApplicationName/PipelineRunName"). Because the GitLab API does not allow deleting commit pipeline jobs, the original, generic "failed" status remained on the commit forever. This resulted in the overall commit pipeline being permanently and incorrectly marked as "failed," even though all pipelines eventually succeeded. The Solution: this commit provides PipelineRun name from "OriginalPRName" annotation so that contextKey is uniform for "success" PipelineRun and failed PipelineRuns due to validation. https://issues.redhat.com/browse/SRVKP-9044 Signed-off-by: Zaki Shaikh --- .yamllint | 1 + pkg/consoleui/openshift.go | 12 ++ pkg/consoleui/openshift_test.go | 6 + pkg/consoleui/tektondashboard.go | 6 +- pkg/pipelineascode/pipelineascode.go | 3 + test/gitlab_merge_request_test.go | 145 +++++++++++++++++++- test/pkg/payload/get_entries.go | 9 +- test/testdata/failures/bad-pipelinerun.yaml | 11 ++ test/testdata/good-pipelinerun.yaml | 19 +++ 9 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 test/testdata/failures/bad-pipelinerun.yaml create mode 100644 test/testdata/good-pipelinerun.yaml diff --git a/.yamllint b/.yamllint index 68e2508c39..785cbc3440 100644 --- a/.yamllint +++ b/.yamllint @@ -3,6 +3,7 @@ ignore: | .git test/testdata/failures/pipeline_bad_format.yaml test/testdata/failures/bad-yaml.yaml + test/testdata/failures/bad-pipelinerun.yaml *badyaml.yaml .github/workflows/* diff --git a/pkg/consoleui/openshift.go b/pkg/consoleui/openshift.go index cf54c8eedc..9e9f5039a9 100644 --- a/pkg/consoleui/openshift.go +++ b/pkg/consoleui/openshift.go @@ -34,18 +34,30 @@ func (o *OpenshiftConsole) GetName() string { } func (o *OpenshiftConsole) URL() string { + if o.host == "" { + return "https://openshift.url.is.not.configured" + } return "https://" + o.host } func (o *OpenshiftConsole) DetailURL(pr *tektonv1.PipelineRun) string { + if o.host == "" { + return fmt.Sprintf(openShiftPipelineDetailViewURL, "openshift.url.is.not.configured", pr.GetNamespace(), pr.GetName()) + } return fmt.Sprintf(openShiftPipelineDetailViewURL, o.host, pr.GetNamespace(), pr.GetName()) } func (o *OpenshiftConsole) TaskLogURL(pr *tektonv1.PipelineRun, taskRunStatus *tektonv1.PipelineRunTaskRunStatus) string { + if o.host == "" { + return fmt.Sprintf(openShiftPipelineTaskLogURL, o.DetailURL(pr), taskRunStatus.PipelineTaskName) + } return fmt.Sprintf(openShiftPipelineTaskLogURL, o.DetailURL(pr), taskRunStatus.PipelineTaskName) } func (o *OpenshiftConsole) NamespaceURL(pr *tektonv1.PipelineRun) string { + if o.host == "" { + return fmt.Sprintf(openShiftPipelineNamespaceViewURL, "openshift.url.is.not.configured", pr.GetNamespace()) + } return fmt.Sprintf(openShiftPipelineNamespaceViewURL, o.host, pr.GetNamespace()) } diff --git a/pkg/consoleui/openshift_test.go b/pkg/consoleui/openshift_test.go index 578ea6957a..6307c59856 100644 --- a/pkg/consoleui/openshift_test.go +++ b/pkg/consoleui/openshift_test.go @@ -127,4 +127,10 @@ func TestOpenshiftConsoleURLs(t *testing.T) { assert.Equal(t, o.DetailURL(pr), "https://fakeconsole/k8s/ns/theNS/tekton.dev~v1~PipelineRun/pr") assert.Equal(t, o.TaskLogURL(pr, trStatus), "https://fakeconsole/k8s/ns/theNS/tekton.dev~v1~PipelineRun/pr/logs/task") assert.Equal(t, o.NamespaceURL(pr), "https://fakeconsole/pipelines/ns/theNS/pipeline-runs") + + emptyHost := OpenshiftConsole{host: ""} + assert.Equal(t, emptyHost.URL(), "https://openshift.url.is.not.configured") + assert.Equal(t, emptyHost.DetailURL(pr), "https://openshift.url.is.not.configured/k8s/ns/theNS/tekton.dev~v1~PipelineRun/pr") + assert.Equal(t, emptyHost.TaskLogURL(pr, trStatus), "https://openshift.url.is.not.configured/k8s/ns/theNS/tekton.dev~v1~PipelineRun/pr/logs/task") + assert.Equal(t, emptyHost.NamespaceURL(pr), "https://openshift.url.is.not.configured/pipelines/ns/theNS/pipeline-runs") } diff --git a/pkg/consoleui/tektondashboard.go b/pkg/consoleui/tektondashboard.go index 882cd42bff..cc53a5c592 100644 --- a/pkg/consoleui/tektondashboard.go +++ b/pkg/consoleui/tektondashboard.go @@ -19,7 +19,7 @@ func (t *TektonDashboard) GetName() string { } func (t *TektonDashboard) DetailURL(pr *tektonv1.PipelineRun) string { - return fmt.Sprintf("%s/#/namespaces/%s/pipelineruns/%s", t.BaseURL, pr.GetNamespace(), pr.GetName()) + return fmt.Sprintf("%s/#/namespaces/%s/pipelineruns/%s", t.URL(), pr.GetNamespace(), pr.GetName()) } func (t *TektonDashboard) NamespaceURL(pr *tektonv1.PipelineRun) string { @@ -31,6 +31,10 @@ func (t *TektonDashboard) TaskLogURL(pr *tektonv1.PipelineRun, taskRunStatus *te } func (t *TektonDashboard) URL() string { + // if BaseURL is not provided, return fake URL + if t.BaseURL == "" || t.BaseURL == "http://" || t.BaseURL == "https://" { + return "https://dashboard.url.is.not.configured" + } return t.BaseURL } diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index e9e9af1f90..aa56e90220 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -121,6 +121,9 @@ func (p *PacRun) Run(ctx context.Context) error { errMsgM := fmt.Sprintf("There was an error creating the PipelineRun: %s\n\n%s", match.PipelineRun.GetGenerateName(), err.Error()) p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositoryPipelineRun", errMsg) createStatusErr := p.vcx.CreateStatus(ctx, p.event, provider.StatusOpts{ + PipelineRunName: match.PipelineRun.GetName(), + PipelineRun: match.PipelineRun, + OriginalPipelineRunName: match.PipelineRun.GetAnnotations()[keys.OriginalPRName], Status: CompletedStatus, Conclusion: failureConclusion, Text: errMsgM, diff --git a/test/gitlab_merge_request_test.go b/test/gitlab_merge_request_test.go index ee253fb603..008f4f459c 100644 --- a/test/gitlab_merge_request_test.go +++ b/test/gitlab_merge_request_test.go @@ -1,6 +1,3 @@ -//go:build e2e -// +build e2e - package test import ( @@ -15,6 +12,7 @@ import ( "github.com/google/go-github/v74/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" + "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx" @@ -812,6 +810,147 @@ func TestGitlabMergeRequestValidationErrorsFromFork(t *testing.T) { assert.Assert(t, foundValidationComment, "Validation error comment should appear on original project's MR. ") } +func TestGitlabConsistentCommitStatusOnMR(t *testing.T) { + targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns") + ctx := context.Background() + runcnx, opts, glprovider, err := tgitlab.Setup(ctx) + assert.NilError(t, err) + ctx, err = cctx.GetControllerCtxInfo(ctx, runcnx) + assert.NilError(t, err) + runcnx.Clients.Log.Info("Testing GitLab consistent commit status on Merge Request scenario") + + projectinfo, resp, err := glprovider.Client().Projects.GetProject(opts.ProjectID, nil) + assert.NilError(t, err) + if resp != nil && resp.StatusCode == http.StatusNotFound { + t.Errorf("Repository %d not found", opts.ProjectID) + } + + err = tgitlab.CreateCRD(ctx, projectinfo, runcnx, opts, targetNS, nil) + assert.NilError(t, err) + + entries, err := payload.GetEntries(map[string]string{ + ".tekton/bad-pipelinerun.yaml": "testdata/failures/bad-pipelinerun.yaml", + ".tekton/good-pipelinerun.yaml": "testdata/pipelinerun.yaml", + }, targetNS, projectinfo.DefaultBranch, + triggertype.PullRequest.String(), map[string]string{"PipelineName": "good-pipelinerun"}) + assert.NilError(t, err) + + targetRefName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test") + cloneURL, err := scm.MakeGitCloneURL(projectinfo.WebURL, opts.UserName, opts.Password) + assert.NilError(t, err) + + commitTitle := "Add invalid .tekton file - " + targetRefName + scmOpts := &scm.Opts{ + GitURL: cloneURL, + CommitTitle: commitTitle, + Log: runcnx.Clients.Log, + WebURL: projectinfo.WebURL, + TargetRefName: targetRefName, + BaseRefName: projectinfo.DefaultBranch, + } + _ = scm.PushFilesToRefGit(t, scmOpts, entries) + runcnx.Clients.Log.Infof("Pushed invalid .tekton files to branch: %s", targetRefName) + + mrTitle := "TestConsistentCommitStatusOnMR - " + targetRefName + mrOptions := &clientGitlab.CreateMergeRequestOptions{ + Title: &mrTitle, + SourceBranch: &targetRefName, + TargetBranch: &projectinfo.DefaultBranch, + } + + mr, _, err := glprovider.Client().MergeRequests.CreateMergeRequest(projectinfo.ID, mrOptions) + assert.NilError(t, err) + runcnx.Clients.Log.Infof("Created merge request: %d", mr.IID) + + defer tgitlab.TearDown(ctx, t, runcnx, glprovider, mr.IID, targetRefName, targetNS, projectinfo.ID) + + sopt := twait.SuccessOpt{ + Title: commitTitle, + OnEvent: "Merge Request", + TargetNS: targetNS, + NumberofPRMatch: 1, // there must be one PipelineRun because one is invalid + SHA: mr.SHA, + } + twait.Succeeded(ctx, t, runcnx, opts, sopt) + labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(mr.SHA)) + prsNew, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + }) + assert.NilError(t, err) + assert.Assert(t, len(prsNew.Items) == 1) + + commitStatuses, _, err := glprovider.Client().Commits.GetCommitStatuses(projectinfo.ID, mr.SHA, &clientGitlab.GetCommitStatusesOptions{}) + assert.NilError(t, err) + assert.Assert(t, len(commitStatuses) == 2) + + // here we don't know which status is returned first from GitLab API + // so we need to check both by their names + for _, cs := range commitStatuses { + switch cs.Name { + case "Pipelines as Code CI / bad-pipelinerun": + assert.Assert(t, cs.Status == "failed") + case "Pipelines as Code CI / good-pipelinerun": + assert.Assert(t, cs.Status == "success") + default: + t.Fatalf("unexpected commit status name: %s", cs.Name) + } + } + + entries, err = payload.GetEntries(map[string]string{ + ".tekton/bad-pipelinerun.yaml": "testdata/good-pipelinerun.yaml", + ".tekton/good-pipelinerun.yaml": "testdata/pipelinerun.yaml", + }, targetNS, projectinfo.DefaultBranch, + triggertype.PullRequest.String(), map[string]string{"PipelineName": "good-pipelinerun"}) + assert.NilError(t, err) + + commitTitle = "Add good .tekton file - " + targetRefName + scmOpts = &scm.Opts{ + GitURL: cloneURL, + CommitTitle: commitTitle, + Log: runcnx.Clients.Log, + WebURL: projectinfo.WebURL, + TargetRefName: targetRefName, + BaseRefName: projectinfo.DefaultBranch, + PushForce: true, + } + _ = scm.PushFilesToRefGit(t, scmOpts, entries) + runcnx.Clients.Log.Infof("Pushed good .tekton files to branch: %s", targetRefName) + + // get latest MR because of last commit it is update + mr, _, err = glprovider.Client().MergeRequests.GetMergeRequest(projectinfo.ID, mr.IID, nil) + assert.NilError(t, err) + + sopt = twait.SuccessOpt{ + Title: commitTitle, + OnEvent: "Merge Request", + TargetNS: targetNS, + NumberofPRMatch: 2, + SHA: mr.SHA, + } + + twait.Succeeded(ctx, t, runcnx, opts, sopt) + labelSelector = fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(mr.SHA)) + prsNew, err = runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + }) + assert.NilError(t, err) + assert.Assert(t, len(prsNew.Items) == 2) + + commitStatuses, _, err = glprovider.Client().Commits.GetCommitStatuses(projectinfo.ID, mr.SHA, &clientGitlab.GetCommitStatusesOptions{}) + assert.NilError(t, err) + assert.Assert(t, len(commitStatuses) == 2) + + // now both should be success + for _, cs := range commitStatuses { + switch cs.Name { + case "Pipelines as Code CI / bad-pipelinerun", "Pipelines as Code CI / good-pipelinerun": + assert.Assert(t, cs.Status == "success") + default: + t.Fatalf("unexpected commit status name: %s", cs.Name) + } + } +} + // Local Variables: // compile-command: "go test -tags=e2e -v -run ^TestGitlabMergeRequest$" // End: diff --git a/test/pkg/payload/get_entries.go b/test/pkg/payload/get_entries.go index 761ccdec5d..bffbcc2938 100644 --- a/test/pkg/payload/get_entries.go +++ b/test/pkg/payload/get_entries.go @@ -26,10 +26,11 @@ func GetEntries(yamlfile map[string]string, targetNS, targetBranch, targetEvent } entries := map[string]string{} for target, file := range yamlfile { - name := strings.TrimSuffix(filepath.Base(target), filepath.Ext(target)) - // add some random character to name so that each PR has different name - extraParams["PipelineName"] = name + "-" + strings.ToLower(random.AlphaString(4)) - // PipelineName can be overridden by extraParams + if extraParams["PipelineName"] == "" { + name := strings.TrimSuffix(filepath.Base(target), filepath.Ext(target)) + // add some random character to name so that each PR has different name + extraParams["PipelineName"] = name + "-" + strings.ToLower(random.AlphaString(4)) + } newParams := vinceMap(params, extraParams) output, err := ApplyTemplate(file, newParams) diff --git a/test/testdata/failures/bad-pipelinerun.yaml b/test/testdata/failures/bad-pipelinerun.yaml new file mode 100644 index 0000000000..80491be71a --- /dev/null +++ b/test/testdata/failures/bad-pipelinerun.yaml @@ -0,0 +1,11 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: bad-pipelinerun + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" +spec: + pipelineRef: diff --git a/test/testdata/good-pipelinerun.yaml b/test/testdata/good-pipelinerun.yaml new file mode 100644 index 0000000000..bb660664f2 --- /dev/null +++ b/test/testdata/good-pipelinerun.yaml @@ -0,0 +1,19 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: bad-pipelinerun # it is still named bad because it is considered fixed version of bad-pipelinerun.yaml + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" +spec: + pipelineSpec: + tasks: + - name: task + displayName: "The Task name is Task" + taskSpec: + steps: + - name: task + image: registry.access.redhat.com/ubi9/ubi-micro + command: ["/bin/echo", "HELLOMOTO"]