diff --git a/docs/statusreport-controller.md b/docs/statusreport-controller.md index 3cd8d86aa..28305d423 100644 --- a/docs/statusreport-controller.md +++ b/docs/statusreport-controller.md @@ -54,13 +54,13 @@ flowchart TD get_all_commitStatuses_from_gh(Get all commitStatuses from github
according to commit owner, repo and SHA) create_commitStatusAdapter(Create commitStatusAdapter according to
commit owner, repo, SHA
and integration test status) does_commitStatus_exist{Does commitStatus exist
on github already?} - create_new_commitStatus_on_gh(Create new commitStatus on github) + create_new_commitStatus_on_gh(Create new commitStatus on github
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
snapshot and scenario
) create_new_comment(Create a new comment for
snapshot and scenario
) 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
if MR is not from forked repo) test_iterate(Iterate across all existing related testStatuses) is_test_final{Is
the test in it's
final state?} diff --git a/gitops/snapshot.go b/gitops/snapshot.go index fcde83314..b83d3c1c0 100644 --- a/gitops/snapshot.go +++ b/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 "" +} diff --git a/gitops/snapshot_test.go b/gitops/snapshot_test.go index d24f6a404..7f0b95dca 100644 --- a/gitops/snapshot_test.go +++ b/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")) + }) }) }) }) diff --git a/internal/controller/buildpipeline/buildpipeline_adapter_test.go b/internal/controller/buildpipeline/buildpipeline_adapter_test.go index 6ce86094f..d635e6ef2 100644 --- a/internal/controller/buildpipeline/buildpipeline_adapter_test.go +++ b/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()) diff --git a/status/reporter_github.go b/status/reporter_github.go index 389da5495..22f1bead5 100644 --- a/status/reporter_github.go +++ b/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 diff --git a/status/reporter_github_test.go b/status/reporter_github_test.go index 48303b327..397a6a4af 100644 --- a/status/reporter_github_test.go +++ b/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)) + }) }) }) diff --git a/status/reporter_gitlab.go b/status/reporter_gitlab.go index 39dd4c5e9..d97ef438b 100644 --- a/status/reporter_gitlab.go +++ b/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 diff --git a/status/reporter_gitlab_test.go b/status/reporter_gitlab_test.go index 11a8a1dc3..639cb5708 100644 --- a/status/reporter_gitlab_test.go +++ b/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() {