Merge pull request #943 from hongweiliu17/STONEINTG-1091

fix(STONEINTG-1091): don't set commitStatus for PR/MR from forked repo

docs/statusreport-controller.md

@@ -54,13 +54,13 @@ flowchart TD
   get_all_commitStatuses_from_gh(Get all commitStatuses from github <br>according to commit owner, repo and SHA)
   create_commitStatusAdapter(Create commitStatusAdapter according to <br>commit owner, repo, SHA <br>and integration test status)
   does_commitStatus_exist{Does commitStatus exist <br>on github already?}
-  create_new_commitStatus_on_gh(Create new commitStatus on github)
+  create_new_commitStatus_on_gh(Create new commitStatus on github<br>if PR is not from forked repo)
   does_comment_exist(Does a comment exist for snapshot and scenario?)
   update_existing_comment(Update the existing comment for <br>snapshot and scenario</br>)
   create_new_comment(Create a new comment for <br>snapshot and scenario</br>)
 
   collect_commit_info_gl(Collect commit projectID, repo-url and SHA from Snapshot)
-  report_commit_status_gl(Create/update commitStatus on Gitlab)
+  report_commit_status_gl(Create/update commitStatus on Gitlab<br>if MR is not from forked repo)
 
   test_iterate(Iterate across all existing related testStatuses)
   is_test_final{Is <br> the test in it's <br>final state?}

gitops/snapshot.go

@@ -1146,3 +1146,12 @@ func UnmarshalJSON(b []byte) ([]*ComponentSnapshotInfo, error) {
 
 	return componentSnapshotInfos, nil
 }
+
+func GetSourceRepoOwnerFromSnapshot(snapshot *applicationapiv1alpha1.Snapshot) string {
+	sourceRepoUrlAnnotation, found := snapshot.GetAnnotations()[PipelineAsCodeGitSourceURLAnnotation]
+	if found {
+		arr := strings.Split(sourceRepoUrlAnnotation, "/")
+		return arr[len(arr)-2]
+	}
+	return ""
+}

gitops/snapshot_test.go

@@ -944,6 +944,14 @@ var _ = Describe("Gitops functions for managing Snapshots", Ordered, func() {
 				}, time.Second*10).Should(BeTrue())
 				Expect(err).ShouldNot(HaveOccurred())
 			})
+
+			It("Can return correct source repo owner for snapshot", func() {
+				sourceRepoOwner := gitops.GetSourceRepoOwnerFromSnapshot(hasSnapshot)
+				Expect(sourceRepoOwner).To(Equal(""))
+				Expect(metadata.SetAnnotation(hasSnapshot, gitops.PipelineAsCodeGitSourceURLAnnotation, "https://github.com/devfile-sample/devfile-sample-go-basic")).To(Succeed())
+				sourceRepoOwner = gitops.GetSourceRepoOwnerFromSnapshot(hasSnapshot)
+				Expect(sourceRepoOwner).To(Equal("devfile-sample"))
+			})
 		})
 	})
 })

internal/controller/buildpipeline/buildpipeline_adapter_test.go

@@ -1033,11 +1033,14 @@ var _ = Describe("Pipeline Adapter", Ordered, func() {
 			})
 			It("add pr group to the build pipelineRun annotations and labels", func() {
 				existingBuildPLR := new(tektonv1.PipelineRun)
-				err := k8sClient.Get(ctx, types.NamespacedName{
-					Namespace: buildPipelineRun.Namespace,
-					Name:      buildPipelineRun.Name,
-				}, existingBuildPLR)
-				Expect(err).NotTo(HaveOccurred())
+				Eventually(func() bool {
+					err := k8sClient.Get(ctx, types.NamespacedName{
+						Namespace: buildPipelineRun.Namespace,
+						Name:      buildPipelineRun.Name,
+					}, existingBuildPLR)
+					return err == nil
+				}, time.Second*10).Should(BeTrue())
+
 				Expect(metadata.HasAnnotation(existingBuildPLR, gitops.PRGroupAnnotation)).To(BeFalse())
 				Expect(metadata.HasLabel(existingBuildPLR, gitops.PRGroupHashLabel)).To(BeFalse())
 

status/reporter_github.go

@@ -417,49 +417,56 @@ func (csu *CommitStatusUpdater) updateStatusInComment(ctx context.Context, repor
 // UpdateStatus updates commit status in PR
 func (csu *CommitStatusUpdater) UpdateStatus(ctx context.Context, report TestReport) error {
 
-	allCommitStatuses, err := csu.getAllCommitStatuses(ctx)
-	if err != nil {
-		csu.logger.Error(err, "failed to get all CommitStatuses for scenario",
-			"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name,
-			"scenario.Name", report.ScenarioName)
-		return err
-	}
-
-	commitStatusAdapter, err := csu.createCommitStatusAdapterForSnapshot(report)
-	if err != nil {
-		csu.logger.Error(err, "failed to create CommitStatusAdapter for scenario, skipping update",
-			"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name,
-			"scenario.Name", report.ScenarioName,
-		)
-		return nil
-	}
+	sourceRepoOwner := gitops.GetSourceRepoOwnerFromSnapshot(csu.snapshot)
+	// we create/update commitStatus only when the source and target repo owner are the same
+	if csu.owner == sourceRepoOwner {
+		allCommitStatuses, err := csu.getAllCommitStatuses(ctx)
+		if err != nil {
+			csu.logger.Error(err, "failed to get all CommitStatuses for scenario",
+				"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name,
+				"scenario.Name", report.ScenarioName)
+			return err
+		}
 
-	commitStatusExist, err := csu.ghClient.CommitStatusExists(allCommitStatuses, commitStatusAdapter)
-	if err != nil {
-		csu.logger.Error(err, "failed to check existing commitStatus")
-		return err
-	}
+		commitStatusAdapter, err := csu.createCommitStatusAdapterForSnapshot(report)
+		if err != nil {
+			csu.logger.Error(err, "failed to create CommitStatusAdapter for scenario, skipping update",
+				"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name,
+				"scenario.Name", report.ScenarioName,
+			)
+			return nil
+		}
 
-	if !commitStatusExist {
-		csu.logger.Info("creating commit status for scenario test status of snapshot",
-			"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
-		_, err = csu.ghClient.CreateCommitStatus(ctx, commitStatusAdapter.Owner, commitStatusAdapter.Repository, commitStatusAdapter.SHA, commitStatusAdapter.State, commitStatusAdapter.Description, commitStatusAdapter.Context, commitStatusAdapter.TargetURL)
+		commitStatusExist, err := csu.ghClient.CommitStatusExists(allCommitStatuses, commitStatusAdapter)
 		if err != nil {
-			csu.logger.Error(err, "failed to create commitStatus", "snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
+			csu.logger.Error(err, "failed to check existing commitStatus")
 			return err
 		}
-		// Create a comment when integration test is neither pending nor inprogress since comment for pending/inprogress is less meaningful and there is commitStatus for all statuses
-		_, isPullRequest := csu.snapshot.GetAnnotations()[gitops.PipelineAsCodePullRequestAnnotation]
-		if report.Status != intgteststat.IntegrationTestStatusPending && report.Status != intgteststat.IntegrationTestStatusInProgress && isPullRequest {
-			err = csu.updateStatusInComment(ctx, report)
+
+		if !commitStatusExist {
+			csu.logger.Info("creating commit status for scenario test status of snapshot",
+				"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
+			_, err = csu.ghClient.CreateCommitStatus(ctx, commitStatusAdapter.Owner, commitStatusAdapter.Repository, commitStatusAdapter.SHA, commitStatusAdapter.State, commitStatusAdapter.Description, commitStatusAdapter.Context, commitStatusAdapter.TargetURL)
 			if err != nil {
-				csu.logger.Error(err, "failed to update comment", "snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
+				csu.logger.Error(err, "failed to create commitStatus", "snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
 				return err
 			}
+		} else {
+			csu.logger.Info("found existing commitStatus for scenario test status of snapshot, no need to create new commit status",
+				"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
 		}
 	} else {
-		csu.logger.Info("found existing commitStatus for scenario test status of snapshot, no need to create new commit status",
-			"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
+		csu.logger.Info("Won't create/update commitStatus since there is access limitation for different source and target Repo Owner",
+			"snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "sourceRepoOwner", sourceRepoOwner, "targetRepoOwner", csu.owner)
+	}
+	// Create a comment when integration test is neither pending nor inprogress since comment for pending/inprogress is less meaningful and there is commitStatus for all statuses
+	_, isPullRequest := csu.snapshot.GetAnnotations()[gitops.PipelineAsCodePullRequestAnnotation]
+	if report.Status != intgteststat.IntegrationTestStatusPending && report.Status != intgteststat.IntegrationTestStatusInProgress && isPullRequest {
+		err := csu.updateStatusInComment(ctx, report)
+		if err != nil {
+			csu.logger.Error(err, "failed to update comment", "snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName)
+			return err
+		}
 	}
 
 	return nil

status/reporter_github_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/go-logr/logr"
 	ghapi "github.com/google/go-github/v45/github"
 	applicationapiv1alpha1 "github.com/konflux-ci/application-api/api/v1alpha1"
+	"github.com/konflux-ci/operator-toolkit/metadata"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	pacv1alpha1 "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
@@ -211,10 +212,11 @@ var _ = Describe("GitHubReporter", func() {
 					"pac.test.appstudio.openshift.io/event-type":     "pull_request",
 				},
 				Annotations: map[string]string{
-					"build.appstudio.redhat.com/commit_sha":         "6c65b2fcaea3e1a0a92476c8b5dc89e92a85f025",
-					"appstudio.redhat.com/updateComponentOnSuccess": "false",
-					"pac.test.appstudio.openshift.io/git-provider":  "github",
-					"pac.test.appstudio.openshift.io/repo-url":      "https://github.com/devfile-sample/devfile-sample-go-basic",
+					"build.appstudio.redhat.com/commit_sha":           "6c65b2fcaea3e1a0a92476c8b5dc89e92a85f025",
+					"appstudio.redhat.com/updateComponentOnSuccess":   "false",
+					"pac.test.appstudio.openshift.io/git-provider":    "github",
+					"pac.test.appstudio.openshift.io/repo-url":        "https://github.com/devfile-sample/devfile-sample-go-basic",
+					"pac.test.appstudio.openshift.io/source-repo-url": "https://github.com/devfile-sample/devfile-sample-go-basic",
 				},
 			},
 			Spec: applicationapiv1alpha1.SnapshotSpec{
@@ -595,5 +597,18 @@ var _ = Describe("GitHubReporter", func() {
 			expectedLogEntry := "found existing commitStatus for scenario test status of snapshot, no need to create new commit status"
 			Expect(buf.String()).Should(ContainSubstring(expectedLogEntry))
 		})
+
+		It("don't create commit status when source and target repo owner are different", func() {
+			Expect(metadata.DeleteAnnotation(hasSnapshot, gitops.PipelineAsCodeGitSourceURLAnnotation)).To(Succeed())
+			testReport := status.TestReport{
+				ScenarioName: "scenario2",
+				FullName:     "test/scenario2",
+				Status:       integrationteststatus.IntegrationTestStatusPending,
+				Summary:      "Integration test for snapshot snapshot-sample and scenario scenario2 is pending",
+			}
+			Expect(reporter.ReportStatus(context.TODO(), testReport)).To(Succeed())
+			expectedLogEntry := "Won't create/update commitStatus since there is access limitation for different source and target Repo Owner"
+			Expect(buf.String()).Should(ContainSubstring(expectedLogEntry))
+		})
 	})
 })

status/reporter_gitlab.go

@@ -271,8 +271,15 @@ func (r *GitLabReporter) ReportStatus(ctx context.Context, report TestReport) er
 		return fmt.Errorf("gitlab reporter is not initialized")
 	}
 
-	if err := r.setCommitStatus(report); err != nil {
-		return fmt.Errorf("failed to set gitlab commit status: %w", err)
+	// We only create/update commitStatus when source project and target project are
+	// the same one due to the access limitation for forked repo
+	// refer to the same issue in pipelines-as-code https://github.com/openshift-pipelines/pipelines-as-code/blob/2f78eb8fd04d149b266ba93f2bea706b4b026403/pkg/provider/gitlab/gitlab.go#L207
+	if r.sourceProjectID == r.targetProjectID {
+		if err := r.setCommitStatus(report); err != nil {
+			return fmt.Errorf("failed to set gitlab commit status: %w", err)
+		}
+	} else {
+		r.logger.Info("Won't create/update commitStatus due to the access limitation for forked repo", "r.sourceProjectID", r.sourceProjectID, "r.targetProjectID", r.targetProjectID)
 	}
 
 	// Create a note when integration test is neither pending nor inprogress since comment for pending/inprogress is less meaningful

status/reporter_gitlab_test.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/go-logr/logr"
 	applicationapiv1alpha1 "github.com/konflux-ci/application-api/api/v1alpha1"
+	"github.com/konflux-ci/operator-toolkit/metadata"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	pacv1alpha1 "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
@@ -334,6 +335,31 @@ var _ = Describe("GitLabReporter", func() {
 			Expect(*existingNoteID).To(Equal(note.ID))
 		})
 
+		It("don't create commit status when source and target project ID are different", func() {
+			Expect(metadata.SetAnnotation(hasSnapshot, gitops.PipelineAsCodeSourceProjectIDAnnotation, "0")).To(Succeed())
+			Expect(reporter.Initialize(context.TODO(), hasSnapshot)).To(Succeed())
+
+			PipelineRunName := "TestPipeline"
+			expectedURL := status.FormatPipelineURL(PipelineRunName, hasSnapshot.Namespace, logr.Discard())
+
+			muxCommitStatusPost(mux, sourceProjectID, digest, expectedURL)
+			muxMergeNotes(mux, sourceProjectID, mergeRequest, "")
+			muxCommitStatusesGet(mux, sourceProjectID, digest, nil)
+
+			Expect(reporter.ReportStatus(
+				context.TODO(),
+				status.TestReport{
+					FullName:            "fullname/scenario1",
+					ScenarioName:        "scenario1",
+					TestPipelineRunName: PipelineRunName,
+					Status:              integrationteststatus.IntegrationTestStatusInProgress,
+					Summary:             "summary",
+					Text:                "detailed text here",
+				})).To(Succeed())
+
+			expectedLogEntry := "Won't create/update commitStatus due to the access limitation for forked repo"
+			Expect(buf.String()).Should(ContainSubstring(expectedLogEntry))
+		})
 	})
 
 	Describe("Test helper functions", func() {