Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/statusreport-controller.md
Original file line number Diff line number Diff line change
Expand Up @@ -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?}
Expand Down
9 changes: 9 additions & 0 deletions gitops/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""
}
8 changes: 8 additions & 0 deletions gitops/snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Expand Down
73 changes: 40 additions & 33 deletions status/reporter_github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
dirgim marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
23 changes: 19 additions & 4 deletions status/reporter_github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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))
})
})
})
11 changes: 9 additions & 2 deletions status/reporter_gitlab.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 26 additions & 0 deletions status/reporter_gitlab_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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() {
Expand Down
Loading