From dfcd9e9e54f1cc4d0da29e94c36e4775bf40e587 Mon Sep 17 00:00:00 2001 From: Hongwei Liu Date: Thu, 17 Oct 2024 16:42:02 +0800 Subject: [PATCH] fix(STONEINTG-1048): stop reconciliation for unrecoverable error * stop reconciliation for unrecoverable error when getting PR/MR status during group snapshot preparation Signed-off-by: Hongwei Liu --- git/github/github.go | 3 +- helpers/errorhandlers.go | 12 ++++ helpers/errorhandlers_test.go | 5 ++ .../controller/snapshot/snapshot_adapter.go | 19 +++-- .../snapshot/snapshot_adapter_test.go | 34 ++++++++- status/reporter.go | 19 +++-- status/reporter_github.go | 72 ++++++++++++++----- status/reporter_github_test.go | 1 - status/reporter_gitlab.go | 50 +++++++++---- status/status.go | 71 +++++++++++++----- status/status_test.go | 46 +++++++++++- 11 files changed, 267 insertions(+), 65 deletions(-) diff --git a/git/github/github.go b/git/github/github.go index d7dcbf3cc..75e9c3ab6 100644 --- a/git/github/github.go +++ b/git/github/github.go @@ -529,7 +529,8 @@ func (c *Client) CreateCommitStatus(ctx context.Context, owner string, repo stri func (c *Client) GetPullRequest(ctx context.Context, owner string, repo string, prID int) (*ghapi.PullRequest, error) { pr, _, err := c.GetPullRequestsService().Get(ctx, owner, repo, prID) if err != nil { - return nil, fmt.Errorf("failed to get pull request for owner/repo/pull %s/%s/%d: %w", owner, repo, prID, err) + c.logger.Error(err, "failed to get pull request for owner/repo/pull %s/%s/%d", owner, repo, prID) + return nil, err } return pr, err diff --git a/helpers/errorhandlers.go b/helpers/errorhandlers.go index 1e7c769c6..037212bfe 100644 --- a/helpers/errorhandlers.go +++ b/helpers/errorhandlers.go @@ -27,6 +27,7 @@ const ( ReasonInvalidImageDigestError = "InvalidImageDigest" ReasonMissingValidComponentError = "MissingValidComponentError" ReasonUnknownError = "UnknownError" + ReasonUnrecoverableMetadataError = "UnrecoverableMetadataError" ) type IntegrationError struct { @@ -99,3 +100,14 @@ func HandleLoaderError(logger IntegrationLogger, err error, resource, from strin logger.Error(err, fmt.Sprintf("Failed to get %s from the %s", resource, from)) return ctrl.Result{}, err } + +func NewUnrecoverableMetadataError(msg string) error { + return &IntegrationError{ + Reason: ReasonUnrecoverableMetadataError, + Message: fmt.Sprintf("Meeting metadata data error: %s", msg), + } +} + +func IsUnrecoverableMetadataError(err error) bool { + return getReason(err) == ReasonUnrecoverableMetadataError +} diff --git a/helpers/errorhandlers_test.go b/helpers/errorhandlers_test.go index 2c2ac7141..0bd381ce2 100644 --- a/helpers/errorhandlers_test.go +++ b/helpers/errorhandlers_test.go @@ -106,5 +106,10 @@ var _ = Describe("Helpers for error handlers", Ordered, func() { Expect(helpers.IsMissingInfoInPipelineRunError(err)).To(BeFalse()) }) + It("Can define UnrecoverableMetadataError", func() { + err := helpers.NewUnrecoverableMetadataError("undefined annotation") + Expect(helpers.IsUnrecoverableMetadataError(err)).To(BeTrue()) + Expect(err.Error()).To(Equal("Meeting metadata data error: undefined annotation")) + }) }) }) diff --git a/internal/controller/snapshot/snapshot_adapter.go b/internal/controller/snapshot/snapshot_adapter.go index d24017122..8f8c89d0d 100644 --- a/internal/controller/snapshot/snapshot_adapter.go +++ b/internal/controller/snapshot/snapshot_adapter.go @@ -603,17 +603,16 @@ func (a *Adapter) EnsureGroupSnapshotExist() (controller.OperationResult, error) groupSnapshot, componentSnapshotInfos, err := a.prepareGroupSnapshot(a.application, prGroupHash) if err != nil { - if strings.Contains(err.Error(), "failed to get pull request for") { - // Stop processing in case the repo was removed/moved, and we reach 404 when trying to find the status of pull request - return controller.StopProcessing() - } - a.logger.Error(err, "Failed to prepare group snapshot") - // stop reconciliation directly when meeting error before STONEINTG-1048 is resolved - err = gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("failed to prepare group snapshot for pr group %s, skipping group snapshot creation", prGroup), a.client) - if err != nil { - return controller.RequeueWithError(err) + a.logger.Error(err, "failed to prepare group snapshot") + if h.IsUnrecoverableMetadataError(err) || clienterrors.IsNotFound(err) { + err = gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("failed to prepare group snapshot for pr group %s due to error %s, skipping group snapshot creation", prGroup, err.Error()), a.client) + if err != nil { + return controller.RequeueWithError(err) + } + return controller.ContinueProcessing() } - return controller.StopProcessing() + return controller.RequeueWithError(err) + } if groupSnapshot == nil { diff --git a/internal/controller/snapshot/snapshot_adapter_test.go b/internal/controller/snapshot/snapshot_adapter_test.go index 41df3f9dc..eac9522ba 100644 --- a/internal/controller/snapshot/snapshot_adapter_test.go +++ b/internal/controller/snapshot/snapshot_adapter_test.go @@ -1636,7 +1636,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(metadata.HasAnnotation(hasComSnapshot1, gitops.PRGroupCreationAnnotation)).To(BeTrue()) }) - It("Calling en when there is running build PLR belonging to the same pr group sha", func() { + It("Calling EnsureGroupSnapshotExist when there is running build PLR belonging to the same pr group sha", func() { buildPipelineRun1.Status = tektonv1.PipelineRunStatus{ PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ Results: []tektonv1.PipelineRunResult{}, @@ -1679,7 +1679,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(metadata.HasAnnotation(hasComSnapshot1, gitops.PRGroupCreationAnnotation)).To(BeTrue()) }) - It("Calling en when there is failed build PLR belonging to the same pr group sha", func() { + It("Calling EnsureGroupSnapshotExist when there is failed build PLR belonging to the same pr group sha", func() { buildPipelineRun1.Status = tektonv1.PipelineRunStatus{ PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ Results: []tektonv1.PipelineRunResult{}, @@ -1764,6 +1764,36 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(err).ToNot(HaveOccurred()) }) + It("Stop processing when there is no annotationID in snapshot", func() { + var buf bytes.Buffer + log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} + adapter = NewAdapter(ctx, hasComSnapshot1, hasApp, log, loader.NewMockLoader(), k8sClient) + adapter.context = toolkit.GetMockedContext(ctx, []toolkit.MockData{ + { + ContextKey: loader.ApplicationContextKey, + Resource: hasApp, + }, + { + ContextKey: loader.SnapshotContextKey, + Resource: hasComSnapshot1, + }, + { + ContextKey: loader.GetBuildPLRContextKey, + Resource: []tektonv1.PipelineRun{}, + }, + { + ContextKey: loader.ApplicationComponentsContextKey, + Resource: []applicationapiv1alpha1.Component{*hasCom1, *hasCom3}, + }, + }) + + result, err := adapter.EnsureGroupSnapshotExist() + Expect(result.CancelRequest).To(BeFalse()) + Expect(result.RequeueRequest).To(BeFalse()) + Expect(buf.String()).Should(ContainSubstring("failed to get app credentials from Snapshot")) + Expect(err).ToNot(HaveOccurred()) + }) + It("Ensure group snasphot can be created", func() { var buf bytes.Buffer log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} diff --git a/status/reporter.go b/status/reporter.go index 052b44e43..d85c855c4 100644 --- a/status/reporter.go +++ b/status/reporter.go @@ -28,8 +28,10 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" "github.com/konflux-ci/integration-service/gitops" + "github.com/konflux-ci/integration-service/helpers" intgteststat "github.com/konflux-ci/integration-service/pkg/integrationteststatus" ) @@ -69,18 +71,22 @@ type ReporterInterface interface { // GetPACGitProviderToken lookup for configured repo and fetch token from namespace func GetPACGitProviderToken(ctx context.Context, k8sClient client.Client, snapshot *applicationapiv1alpha1.Snapshot) (string, error) { - var err error + log := log.FromContext(ctx) + var err, unRecoverableError error // List all the Repository CRs in the namespace repos := pacv1alpha1.RepositoryList{} if err = k8sClient.List(ctx, &repos, &client.ListOptions{Namespace: snapshot.Namespace}); err != nil { + log.Error(err, fmt.Sprintf("failed to get repo from namespace %s", snapshot.Namespace)) return "", err } // Get the full repo URL url, found := snapshot.GetAnnotations()[gitops.PipelineAsCodeRepoURLAnnotation] if !found { - return "", fmt.Errorf("object annotation not found %q", gitops.PipelineAsCodeRepoURLAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("object annotation not found %q", gitops.PipelineAsCodeRepoURLAnnotation)) + log.Error(unRecoverableError, fmt.Sprintf("object annotation not found %q", gitops.PipelineAsCodeRepoURLAnnotation)) + return "", unRecoverableError } // Find a Repository CR with a matching URL and get its secret details @@ -93,20 +99,25 @@ func GetPACGitProviderToken(ctx context.Context, k8sClient client.Client, snapsh } if repoSecret == nil { - return "", fmt.Errorf("failed to find a Repository matching URL: %q", url) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to find a Repository matching URL: %q", url)) + log.Error(unRecoverableError, fmt.Sprintf("failed to find a Repository matching URL: %q", url)) + return "", unRecoverableError } // Get the pipelines as code secret from the PipelineRun's namespace pacSecret := v1.Secret{} err = k8sClient.Get(ctx, types.NamespacedName{Namespace: snapshot.Namespace, Name: repoSecret.Name}, &pacSecret) if err != nil { + log.Error(err, fmt.Sprintf("failed to get secret %s/%s", snapshot.Namespace, repoSecret.Name)) return "", err } // Get the personal access token from the secret token, found := pacSecret.Data[repoSecret.Key] if !found { - return "", fmt.Errorf("failed to find %s secret key", repoSecret.Key) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to find %s secret key", repoSecret.Key)) + log.Error(unRecoverableError, fmt.Sprintf("failed to find %s secret key", repoSecret.Key)) + return "", unRecoverableError } return string(token), nil diff --git a/status/reporter_github.go b/status/reporter_github.go index 751282517..54d9cc017 100644 --- a/status/reporter_github.go +++ b/status/reporter_github.go @@ -18,7 +18,6 @@ package status import ( "context" - "errors" "fmt" "strconv" @@ -27,12 +26,14 @@ import ( applicationapiv1alpha1 "github.com/konflux-ci/application-api/api/v1alpha1" "github.com/konflux-ci/integration-service/git/github" "github.com/konflux-ci/integration-service/gitops" + "github.com/konflux-ci/integration-service/helpers" intgteststat "github.com/konflux-ci/integration-service/pkg/integrationteststatus" "github.com/konflux-ci/operator-toolkit/metadata" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" ) // Used by statusReport to get pipelines-as-code-secret under NS integration-service @@ -86,37 +87,46 @@ func NewCheckRunStatusUpdater( } func GetAppCredentials(ctx context.Context, k8sclient client.Client, object client.Object) (*appCredentials, error) { - var err error + log := log.FromContext(ctx) + var err, unRecoverableError error var found bool appInfo := appCredentials{} appInfo.InstallationID, err = strconv.ParseInt(object.GetAnnotations()[gitops.PipelineAsCodeInstallationIDAnnotation], 10, 64) if err != nil { - return nil, err + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("Error %s when parsing string annotation %s: %s", err.Error(), gitops.PipelineAsCodeInstallationIDAnnotation, object.GetAnnotations()[gitops.PipelineAsCodeInstallationIDAnnotation])) + log.Error(unRecoverableError, fmt.Sprintf("Error %s when parsing string annotation %s: %s", err.Error(), gitops.PipelineAsCodeInstallationIDAnnotation, object.GetAnnotations()[gitops.PipelineAsCodeInstallationIDAnnotation])) + return nil, unRecoverableError } // Get the global pipelines as code secret pacSecret := v1.Secret{} err = k8sclient.Get(ctx, types.NamespacedName{Namespace: integrationNS, Name: PACSecret}, &pacSecret) if err != nil { + log.Error(err, fmt.Sprintf("failed to get pac secret %s/%s", integrationNS, PACSecret)) return nil, err } // Get the App ID from the secret ghAppIDBytes, found := pacSecret.Data[gitHubApplicationID] if !found { - return nil, errors.New("failed to find github-application-id secret key") + unRecoverableError = helpers.NewUnrecoverableMetadataError("failed to find github-application-id secret key") + log.Error(unRecoverableError, "failed to find github-application-id secret key") + return nil, unRecoverableError } appInfo.AppID, err = strconv.ParseInt(string(ghAppIDBytes), 10, 64) if err != nil { - return nil, err + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("Error %s when parsing ghAppIDBytes", err.Error())) + log.Error(unRecoverableError, "failed to parse gitHub App ID") + return nil, unRecoverableError } // Get the App's private key from the secret appInfo.PrivateKey, found = pacSecret.Data[gitHubPrivateKey] if !found { - return nil, errors.New("failed to find github-private-key secret key") + unRecoverableError = helpers.NewUnrecoverableMetadataError("failed to find github-private-key secret key") + return nil, unRecoverableError } return &appInfo, nil @@ -166,12 +176,14 @@ func (cru *CheckRunStatusUpdater) createCheckRunAdapterForSnapshot(report TestRe conclusion, err := generateCheckRunConclusion(report.Status) if err != nil { + cru.logger.Error(err, fmt.Sprintf("failed to generate conclusion for integrationTestScenario %s and snapshot %s/%s", report.ScenarioName, snapshot.Namespace, snapshot.Name)) return nil, fmt.Errorf("unknown status %s for integrationTestScenario %s and snapshot %s/%s", report.Status, report.ScenarioName, snapshot.Namespace, snapshot.Name) } title, err := generateCheckRunTitle(report.Status) if err != nil { - return nil, fmt.Errorf("unknown status %s for integrationTestScenario %s and snapshot %s/%s", report.Status, report.ScenarioName, snapshot.Namespace, snapshot.Name) + cru.logger.Error(err, fmt.Sprintf("failed to generate title for integrationTestScenario %s and snapshot %s/%s", report.ScenarioName, snapshot.Namespace, snapshot.Name)) + return nil, fmt.Errorf("failed to generate title for integrationTestScenario %s and snapshot %s/%s", report.ScenarioName, snapshot.Namespace, snapshot.Name) } externalID := report.ScenarioName @@ -217,6 +229,7 @@ func (cru *CheckRunStatusUpdater) UpdateStatus(ctx context.Context, report TestR allCheckRuns, err := cru.getAllCheckRuns(ctx) if err != nil { + cru.logger.Error(err, "failed to get all checkruns") return err } @@ -333,6 +346,7 @@ func (csu *CommitStatusUpdater) createCommitStatusAdapterForSnapshot(report Test state, err := generateGithubCommitState(report.Status) if err != nil { + csu.logger.Error(err, fmt.Sprintf("failed to generate commitStatus for integrationTestScenario %s and snapshot %s/%s", report.ScenarioName, snapshot.Namespace, snapshot.Name)) return nil, fmt.Errorf("unknown status %s for integrationTestScenario %s and snapshot %s/%s", report.Status, report.ScenarioName, snapshot.Namespace, snapshot.Name) } @@ -355,34 +369,44 @@ func (csu *CommitStatusUpdater) createCommitStatusAdapterForSnapshot(report Test // updateStatusInComment will create/update a comment in PR which creates snapshot func (csu *CommitStatusUpdater) updateStatusInComment(ctx context.Context, report TestReport) error { + var unRecoverableError error issueNumberStr, found := csu.snapshot.GetAnnotations()[gitops.PipelineAsCodePullRequestAnnotation] if !found { - return fmt.Errorf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation)) + csu.logger.Error(unRecoverableError, "snapshot.Name", report.SnapshotName) + return unRecoverableError } issueNumber, err := strconv.Atoi(issueNumberStr) if err != nil { - return err + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert string issueNumberStr %s to int：%s", issueNumberStr, err.Error())) + csu.logger.Error(unRecoverableError, "snapshot.Name", report.SnapshotName) + return unRecoverableError } comment, err := FormatComment(report.Summary, report.Text) if err != nil { - return fmt.Errorf("failed to generate comment for pull-request %d: %w", issueNumber, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to format comment for pull-request %d: %s", issueNumber, err.Error())) + csu.logger.Error(unRecoverableError, "snapshot.Name", report.SnapshotName) + return unRecoverableError } allComments, err := csu.ghClient.GetAllCommentsForPR(ctx, csu.owner, csu.repo, issueNumber) if err != nil { + csu.logger.Error(err, fmt.Sprintf("error while getting all comments for pull-request %s", issueNumberStr)) return fmt.Errorf("error while getting all comments for pull-request %s: %w", issueNumberStr, err) } existingCommentId := csu.ghClient.GetExistingCommentID(allComments, csu.snapshot.Name, report.ScenarioName) if existingCommentId == nil { _, err = csu.ghClient.CreateComment(ctx, csu.owner, csu.repo, issueNumber, comment) if err != nil { + csu.logger.Error(err, fmt.Sprintf("error while creating comment for pull-request %s", issueNumberStr)) return fmt.Errorf("error while creating comment for pull-request %s: %w", issueNumberStr, err) } } else { _, err = csu.ghClient.EditComment(ctx, csu.owner, csu.repo, *existingCommentId, comment) if err != nil { + csu.logger.Error(err, fmt.Sprintf("error while updating comment for pull-request %s", issueNumberStr)) return fmt.Errorf("error while updating comment for pull-request %s: %w", issueNumberStr, err) } } @@ -395,6 +419,9 @@ func (csu *CommitStatusUpdater) UpdateStatus(ctx context.Context, report TestRep 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 } @@ -409,6 +436,7 @@ func (csu *CommitStatusUpdater) UpdateStatus(ctx context.Context, report TestRep commitStatusExist, err := csu.ghClient.CommitStatusExists(allCommitStatuses, commitStatusAdapter) if err != nil { + csu.logger.Error(err, "failed to check existing commitStatus") return err } @@ -417,12 +445,14 @@ func (csu *CommitStatusUpdater) UpdateStatus(ctx context.Context, report TestRep "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 create commitStatus", "snapshot.NameSpace", csu.snapshot.Namespace, "snapshot.Name", csu.snapshot.Name, "scenarioName", report.ScenarioName) 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 if report.Status != intgteststat.IntegrationTestStatusPending && report.Status != intgteststat.IntegrationTestStatusInProgress { 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 } } @@ -554,21 +584,27 @@ func (r *GitHubReporter) Detect(snapshot *applicationapiv1alpha1.Snapshot) bool // Initialize github reporter. Must be called before updating status func (r *GitHubReporter) Initialize(ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) error { + var unRecoverableError error labels := snapshot.GetLabels() - owner, found := labels[gitops.PipelineAsCodeURLOrgLabel] if !found { - return fmt.Errorf("org label not found %q", gitops.PipelineAsCodeURLOrgLabel) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("org label not found %q", gitops.PipelineAsCodeURLOrgLabel)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } repo, found := labels[gitops.PipelineAsCodeURLRepositoryLabel] if !found { - return fmt.Errorf("repository label not found %q", gitops.PipelineAsCodeURLRepositoryLabel) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("repository label not found %q", gitops.PipelineAsCodeURLRepositoryLabel)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } sha, found := labels[gitops.PipelineAsCodeSHALabel] if !found { - return fmt.Errorf("sha label not found %q", gitops.PipelineAsCodeSHALabel) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("sha label not found %q", gitops.PipelineAsCodeSHALabel)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } // Existence of the Pipelines as Code installation ID annotation signals configuration using GitHub App integration. @@ -580,9 +616,9 @@ func (r *GitHubReporter) Initialize(ctx context.Context, snapshot *applicationap } if err := r.updater.Authenticate(ctx, snapshot); err != nil { - return fmt.Errorf("authentication failed: %w", err) + r.logger.Error(err, fmt.Sprintf("failed to authenticate for snapshot %s/%s", snapshot.Namespace, snapshot.Name)) + return err } - return nil } @@ -594,11 +630,13 @@ func (r *GitHubReporter) GetReporterName() string { // Update status in Github func (r *GitHubReporter) ReportStatus(ctx context.Context, report TestReport) error { if r.updater == nil { + r.logger.Error(nil, fmt.Sprintf("reporter is not initialized for snapshot %s", report.SnapshotName)) return fmt.Errorf("reporter is not initialized") } if err := r.updater.UpdateStatus(ctx, report); err != nil { - return fmt.Errorf("failed to update status: %w", err) + r.logger.Error(err, fmt.Sprintf("failed to update status for snapshot %s", report.SnapshotName)) + return err } return nil } diff --git a/status/reporter_github_test.go b/status/reporter_github_test.go index 5464b44b1..6bb5109d5 100644 --- a/status/reporter_github_test.go +++ b/status/reporter_github_test.go @@ -573,6 +573,5 @@ 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)) }) - }) }) diff --git a/status/reporter_gitlab.go b/status/reporter_gitlab.go index e3fa30340..0f058d619 100644 --- a/status/reporter_gitlab.go +++ b/status/reporter_gitlab.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/konflux-ci/integration-service/gitops" + "github.com/konflux-ci/integration-service/helpers" intgteststat "github.com/konflux-ci/integration-service/pkg/integrationteststatus" ) @@ -68,64 +69,84 @@ func (r *GitLabReporter) GetReporterName() string { // Initialize initializes gitlab reporter func (r *GitLabReporter) Initialize(ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) error { + var unRecoverableError error token, err := GetPACGitProviderToken(ctx, r.k8sClient, snapshot) if err != nil { - r.logger.Error(err, "failed to get token from snapshot", + r.logger.Error(err, "failed to get PAC token from snapshot", "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) - return fmt.Errorf("failed to get PAC token for gitlab provider: %w", err) + return err } annotations := snapshot.GetAnnotations() repoUrl, ok := annotations[gitops.PipelineAsCodeRepoURLAnnotation] if !ok { - return fmt.Errorf("failed to get value of %s annotation from the snapshot %s", gitops.PipelineAsCodeRepoURLAnnotation, snapshot.Name) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to get value of %s annotation from the snapshot %s", gitops.PipelineAsCodeRepoURLAnnotation, snapshot.Name)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } burl, err := url.Parse(repoUrl) if err != nil { - return fmt.Errorf("failed to parse repo-url: %w", err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to parse repo-url %s: %s", repoUrl, err.Error())) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } apiURL := fmt.Sprintf("%s://%s", burl.Scheme, burl.Host) r.client, err = gitlab.NewClient(token, gitlab.WithBaseURL(apiURL)) if err != nil { - return fmt.Errorf("failed to create gitlab client: %w", err) + r.logger.Error(err, "failed to create gitlab client", "apiURL", apiURL, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return err } labels := snapshot.GetLabels() sha, found := labels[gitops.PipelineAsCodeSHALabel] if !found { - return fmt.Errorf("sha label not found %q", gitops.PipelineAsCodeSHALabel) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("sha label not found %q", gitops.PipelineAsCodeSHALabel)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } r.sha = sha targetProjectIDstr, found := annotations[gitops.PipelineAsCodeTargetProjectIDAnnotation] if !found { - return fmt.Errorf("target project ID annotation not found %q", gitops.PipelineAsCodeTargetProjectIDAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("target project ID annotation not found %q", gitops.PipelineAsCodeTargetProjectIDAnnotation)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } r.targetProjectID, err = strconv.Atoi(targetProjectIDstr) if err != nil { - return fmt.Errorf("failed to convert project ID '%s' to integer: %w", targetProjectIDstr, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert project ID '%s' to integer: %s", targetProjectIDstr, err.Error())) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } sourceProjectIDstr, found := annotations[gitops.PipelineAsCodeSourceProjectIDAnnotation] if !found { - return fmt.Errorf("source project ID annotation not found %q", gitops.PipelineAsCodeSourceProjectIDAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("source project ID annotation not found %q", gitops.PipelineAsCodeSourceProjectIDAnnotation)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } r.sourceProjectID, err = strconv.Atoi(sourceProjectIDstr) if err != nil { - return fmt.Errorf("failed to convert project ID '%s' to integer: %w", sourceProjectIDstr, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert project ID '%s' to integer: %s", sourceProjectIDstr, err.Error())) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } mergeRequestStr, found := annotations[gitops.PipelineAsCodePullRequestAnnotation] if !found { - return fmt.Errorf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation)) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } r.mergeRequest, err = strconv.Atoi(mergeRequestStr) if err != nil { - return fmt.Errorf("failed to convert merge request number '%s' to integer: %w", mergeRequestStr, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert merge request number '%s' to integer: %s", mergeRequestStr, err.Error())) + r.logger.Error(unRecoverableError, "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) + return unRecoverableError } r.snapshot = snapshot @@ -188,11 +209,14 @@ func (r *GitLabReporter) setCommitStatus(report TestReport) error { func (r *GitLabReporter) updateStatusInComment(report TestReport) error { comment, err := FormatComment(report.Summary, report.Text) if err != nil { - return fmt.Errorf("failed to generate comment for merge-request %d: %w", r.mergeRequest, err) + unRecoverableError := helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to generate comment for merge-request %d: %s", r.mergeRequest, err.Error())) + r.logger.Error(unRecoverableError, "report.SnapshotName", report.SnapshotName) + return unRecoverableError } allNotes, _, err := r.client.Notes.ListMergeRequestNotes(r.targetProjectID, r.mergeRequest, nil) if err != nil { + r.logger.Error(err, "error while getting all comments for merge-request", "mergeRequest", r.mergeRequest, "report.SnapshotName", report.SnapshotName) return fmt.Errorf("error while getting all comments for merge-request %d: %w", r.mergeRequest, err) } existingCommentId := r.GetExistingNoteID(allNotes, report.ScenarioName, report.SnapshotName) diff --git a/status/status.go b/status/status.go index f1419912c..eb78c64ed 100644 --- a/status/status.go +++ b/status/status.go @@ -25,6 +25,7 @@ import ( "net/url" "os" "strconv" + "strings" "time" "github.com/go-logr/logr" @@ -364,67 +365,89 @@ func (s Status) IsPRMRInSnapshotOpened(ctx context.Context, snapshot *applicatio return s.IsMRInSnapshotOpened(ctx, gitlabReporter, snapshot) } - return false, fmt.Errorf("invalid git provier, valid git provider must be one of github and gitlab") + return false, fmt.Errorf("invalid git provider, valid git provider must be one of github and gitlab") } // IsMRInSnapshotOpened check if the gitlab merge request triggering snapshot is opened func (s Status) IsMRInSnapshotOpened(ctx context.Context, reporter ReporterInterface, snapshot *applicationapiv1alpha1.Snapshot) (bool, error) { + var unRecoverableError error log := log.FromContext(ctx) token, err := GetPACGitProviderToken(ctx, s.client, snapshot) if err != nil { - log.Error(err, "failed to get token from snapshot", + log.Error(err, "failed to get PAC token from snapshot", "snapshot.NameSpace", snapshot.Namespace, "snapshot.Name", snapshot.Name) - return false, fmt.Errorf("failed to get PAC token for gitlab provider: %w", err) + return false, err } annotations := snapshot.GetAnnotations() repoUrl, ok := annotations[gitops.PipelineAsCodeRepoURLAnnotation] if !ok { - return false, fmt.Errorf("failed to get value of %s annotation from the snapshot %s", gitops.PipelineAsCodeRepoURLAnnotation, snapshot.Name) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to get value of %s annotation from the snapshot %s", gitops.PipelineAsCodeRepoURLAnnotation, snapshot.Name)) + log.Error(unRecoverableError, fmt.Sprintf("failed to get value of %s annotation from the snapshot %s", gitops.PipelineAsCodeRepoURLAnnotation, snapshot.Name)) + return false, unRecoverableError } burl, err := url.Parse(repoUrl) if err != nil { - return false, fmt.Errorf("failed to parse repo-url: %w", err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to parse repo-url: %s", err.Error())) + log.Error(unRecoverableError, fmt.Sprintf("failed to parse repo-url: %s", err.Error())) + return false, unRecoverableError } apiURL := fmt.Sprintf("%s://%s", burl.Scheme, burl.Host) gitLabClient, err := gitlab.NewClient(token, gitlab.WithBaseURL(apiURL)) if err != nil { + log.Error(err, "failed to create gitLabClient") return false, fmt.Errorf("failed to create gitlab client: %w", err) } targetProjectIDstr, found := annotations[gitops.PipelineAsCodeTargetProjectIDAnnotation] if !found { - return false, fmt.Errorf("target project ID annotation not found %q", gitops.PipelineAsCodeTargetProjectIDAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("target project ID annotation not found %q", gitops.PipelineAsCodeTargetProjectIDAnnotation)) + log.Error(unRecoverableError, fmt.Sprintf("target project ID annotation not found %q", gitops.PipelineAsCodeTargetProjectIDAnnotation)) + return false, unRecoverableError } targetProjectID, err := strconv.Atoi(targetProjectIDstr) if err != nil { - return false, fmt.Errorf("failed to convert project ID '%s' to integer: %w", targetProjectIDstr, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert project ID '%s' to integer: %s", targetProjectIDstr, err.Error())) + log.Error(unRecoverableError, "failed to convert project ID '%s' to integer", targetProjectIDstr) + return false, unRecoverableError } mergeRequestStr, found := annotations[gitops.PipelineAsCodePullRequestAnnotation] if !found { - return false, fmt.Errorf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation)) + log.Error(unRecoverableError, fmt.Sprintf("pull-request annotation not found %q", gitops.PipelineAsCodePullRequestAnnotation)) + return false, unRecoverableError } mergeRequest, err := strconv.Atoi(mergeRequestStr) if err != nil { - return false, fmt.Errorf("failed to convert merge request number '%s' to integer: %w", mergeRequestStr, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert merge request number '%s' to integer: %s", mergeRequestStr, err.Error())) + log.Error(unRecoverableError, fmt.Sprintf("failed to convert merge request number '%s' to integer", mergeRequestStr)) + return false, unRecoverableError } log.Info(fmt.Sprintf("try to find the status of merge request projectID/pulls %d/%d", targetProjectID, mergeRequest)) getOpts := gitlab.GetMergeRequestsOptions{} mr, _, err := gitLabClient.MergeRequests.GetMergeRequest(targetProjectID, mergeRequest, &getOpts) - if mr != nil { + + if err != nil && strings.Contains(err.Error(), "Not Found") { + log.Info(fmt.Sprintf("not found merge request projectID/pulls %d/%d, it might be deleted", targetProjectID, mergeRequest)) + return false, nil + } + + if mr != nil && err == nil { log.Info(fmt.Sprintf("found merge request projectID/pulls %d/%d", targetProjectID, mergeRequest)) - return mr.State == "opened", err + return mr.State == "opened", nil } - log.Info(fmt.Sprintf("can not find merge request projectID/pulls %d/%d", targetProjectID, mergeRequest)) + + log.Error(err, fmt.Sprintf("can not find merge request projectID/pulls %d/%d", targetProjectID, mergeRequest)) return false, err } // IsPRInSnapshotOpened check if the github pull request triggering snapshot is opened func (s Status) IsPRInSnapshotOpened(ctx context.Context, reporter ReporterInterface, snapshot *applicationapiv1alpha1.Snapshot) (bool, error) { + var unRecoverableError error log := log.FromContext(ctx) ghClient := github.NewClient(s.logger) githubAppCreds, err := GetAppCredentials(ctx, s.client, snapshot) @@ -448,29 +471,45 @@ func (s Status) IsPRInSnapshotOpened(ctx context.Context, reporter ReporterInter owner, found := labels[gitops.PipelineAsCodeURLOrgLabel] if !found { - return false, fmt.Errorf("org label not found %q", gitops.PipelineAsCodeURLOrgLabel) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("org label not found %q", gitops.PipelineAsCodeURLOrgLabel)) + log.Error(unRecoverableError, fmt.Sprintf("org label not found %q", gitops.PipelineAsCodeURLOrgLabel)) + return false, unRecoverableError } repo, found := labels[gitops.PipelineAsCodeURLRepositoryLabel] if !found { - return false, fmt.Errorf("repository label not found %q", gitops.PipelineAsCodeURLRepositoryLabel) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("repository label not found %q", gitops.PipelineAsCodeURLRepositoryLabel)) + log.Error(unRecoverableError, fmt.Sprintf("repository label not found %q", gitops.PipelineAsCodeURLRepositoryLabel)) + return false, unRecoverableError } pullRequestStr, found := labels[gitops.PipelineAsCodePullRequestAnnotation] if !found { - return false, fmt.Errorf("pull request label not found %q", gitops.PipelineAsCodePullRequestAnnotation) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("pull request label not found %q", gitops.PipelineAsCodePullRequestAnnotation)) + log.Error(unRecoverableError, fmt.Sprintf("pull request label not found %q", gitops.PipelineAsCodePullRequestAnnotation)) + return false, unRecoverableError } pullRequest, err := strconv.Atoi(pullRequestStr) if err != nil { - return false, fmt.Errorf("failed to convert pull request number '%s' to integer: %w", pullRequestStr, err) + unRecoverableError = helpers.NewUnrecoverableMetadataError(fmt.Sprintf("failed to convert pull request number '%s' to integer: %s", pullRequestStr, err.Error())) + log.Error(unRecoverableError, fmt.Sprintf("failed to convert pull request number '%s' to integer", pullRequestStr)) + return false, unRecoverableError } log.Info(fmt.Sprintf("try to find the status of pull request owner/repo/pulls %s/%s/%d", owner, repo, pullRequest)) pr, err := ghClient.GetPullRequest(ctx, owner, repo, pullRequest) + + if err != nil && strings.Contains(err.Error(), "Not Found") { + log.Error(err, fmt.Sprintf("not found pull request owner/repo/pulls %s/%s/%d, it might be deleted", owner, repo, pullRequest)) + return false, nil + } + if pr != nil { return *pr.State == "open", nil } + + log.Error(err, fmt.Sprintf("cannot find pull request owner/repo/pulls %s/%s/%d", owner, repo, pullRequest)) return false, err } diff --git a/status/status_test.go b/status/status_test.go index 6f32572a9..447f6ad95 100644 --- a/status/status_test.go +++ b/status/status_test.go @@ -25,14 +25,17 @@ 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" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "go.uber.org/mock/gomock" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/konflux-ci/integration-service/gitops" + "github.com/konflux-ci/integration-service/helpers" "github.com/konflux-ci/integration-service/pkg/integrationteststatus" "github.com/konflux-ci/integration-service/status" "k8s.io/apimachinery/pkg/api/errors" @@ -91,6 +94,7 @@ var _ = Describe("Status Adapter", func() { failedTaskRun *tektonv1.TaskRun skippedTaskRun *tektonv1.TaskRun mockK8sClient *MockK8sClient + repo pacv1alpha1.Repository hasComSnapshot2Name = "hascomsnapshot2-sample" hasComSnapshot3Name = "hascomsnapshot3-sample" @@ -438,7 +442,11 @@ var _ = Describe("Status Adapter", func() { } } }, - listInterceptor: func(list client.ObjectList) {}, + listInterceptor: func(list client.ObjectList) { + if repoList, ok := list.(*pacv1alpha1.RepositoryList); ok { + repoList.Items = []pacv1alpha1.Repository{repo} + } + }, } hasSnapshot = &applicationapiv1alpha1.Snapshot{ @@ -710,6 +718,42 @@ var _ = Describe("Status Adapter", func() { Expect(newSRS.Scenarios).To(HaveLen(1)) }) + It("can return unrecoverable error when label is not defined for githubReporter", func() { + err := metadata.DeleteLabel(hasSnapshot, gitops.PipelineAsCodeURLOrgLabel) + Expect(err).ToNot(HaveOccurred()) + githubReporter := status.NewGitHubReporter(logr.Discard(), mockK8sClient) + err = githubReporter.Initialize(context.Background(), hasSnapshot) + Expect(helpers.IsUnrecoverableMetadataError(err)).To(BeTrue()) + + err = metadata.SetLabel(hasSnapshot, gitops.PipelineAsCodeURLOrgLabel, "org") + Expect(err).ToNot(HaveOccurred()) + err = metadata.DeleteLabel(hasSnapshot, gitops.PipelineAsCodeURLRepositoryLabel) + Expect(err).ToNot(HaveOccurred()) + err = githubReporter.Initialize(context.Background(), hasSnapshot) + Expect(helpers.IsUnrecoverableMetadataError(err)).To(BeTrue()) + + err = metadata.SetLabel(hasSnapshot, gitops.PipelineAsCodeURLRepositoryLabel, "repo") + Expect(err).ToNot(HaveOccurred()) + err = metadata.DeleteLabel(hasSnapshot, gitops.PipelineAsCodeSHALabel) + Expect(err).ToNot(HaveOccurred()) + err = githubReporter.Initialize(context.Background(), hasSnapshot) + Expect(helpers.IsUnrecoverableMetadataError(err)).To(BeTrue()) + }) + + It("can return unrecoverable error when label/annotation is not defined for gitlabReporter", func() { + err := metadata.DeleteAnnotation(hasSnapshot, gitops.PipelineAsCodeRepoURLAnnotation) + Expect(err).ToNot(HaveOccurred()) + gitlabReporter := status.NewGitLabReporter(logr.Discard(), mockK8sClient) + err = gitlabReporter.Initialize(context.Background(), hasSnapshot) + Expect(helpers.IsUnrecoverableMetadataError(err)).To(BeTrue()) + + err = metadata.SetAnnotation(hasSnapshot, gitops.PipelineAsCodeRepoURLAnnotation, "https://test-repo.example.com") + Expect(err).ToNot(HaveOccurred()) + err = metadata.SetAnnotation(hasSnapshot, gitops.PipelineAsCodeSourceProjectIDAnnotation, "qqq") + Expect(err).ToNot(HaveOccurred()) + err = gitlabReporter.Initialize(context.Background(), hasSnapshot) + Expect(helpers.IsUnrecoverableMetadataError(err)).To(BeTrue()) + }) }) })