diff --git a/pkg/apis/pipelinesascode/keys/keys.go b/pkg/apis/pipelinesascode/keys/keys.go index f61b729bf..10febd501 100644 --- a/pkg/apis/pipelinesascode/keys/keys.go +++ b/pkg/apis/pipelinesascode/keys/keys.go @@ -23,43 +23,44 @@ import ( ) const ( - ControllerInfo = pipelinesascode.GroupName + "/controller-info" - Task = pipelinesascode.GroupName + "/task" - Pipeline = pipelinesascode.GroupName + "/pipeline" - URLOrg = pipelinesascode.GroupName + "/url-org" - URLRepository = pipelinesascode.GroupName + "/url-repository" - SHA = pipelinesascode.GroupName + "/sha" - Sender = pipelinesascode.GroupName + "/sender" - EventType = pipelinesascode.GroupName + "/event-type" - Branch = pipelinesascode.GroupName + "/branch" - SourceBranch = pipelinesascode.GroupName + "/source-branch" - Repository = pipelinesascode.GroupName + "/repository" - GitProvider = pipelinesascode.GroupName + "/git-provider" - State = pipelinesascode.GroupName + "/state" - ShaTitle = pipelinesascode.GroupName + "/sha-title" - ShaURL = pipelinesascode.GroupName + "/sha-url" - RepoURL = pipelinesascode.GroupName + "/repo-url" - SourceRepoURL = pipelinesascode.GroupName + "/source-repo-url" - PullRequest = pipelinesascode.GroupName + "/pull-request" - InstallationID = pipelinesascode.GroupName + "/installation-id" - GHEURL = pipelinesascode.GroupName + "/ghe-url" - SourceProjectID = pipelinesascode.GroupName + "/source-project-id" - TargetProjectID = pipelinesascode.GroupName + "/target-project-id" - OriginalPRName = pipelinesascode.GroupName + "/original-prname" - GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret" - CheckRunID = pipelinesascode.GroupName + "/check-run-id" - OnEvent = pipelinesascode.GroupName + "/on-event" - OnComment = pipelinesascode.GroupName + "/on-comment" - OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch" - OnPathChange = pipelinesascode.GroupName + "/on-path-change" - OnLabel = pipelinesascode.GroupName + "/on-label" - OnPathChangeIgnore = pipelinesascode.GroupName + "/on-path-change-ignore" - OnCelExpression = pipelinesascode.GroupName + "/on-cel-expression" - TargetNamespace = pipelinesascode.GroupName + "/target-namespace" - MaxKeepRuns = pipelinesascode.GroupName + "/max-keep-runs" - CancelInProgress = pipelinesascode.GroupName + "/cancel-in-progress" - LogURL = pipelinesascode.GroupName + "/log-url" - ExecutionOrder = pipelinesascode.GroupName + "/execution-order" + ControllerInfo = pipelinesascode.GroupName + "/controller-info" + Task = pipelinesascode.GroupName + "/task" + Pipeline = pipelinesascode.GroupName + "/pipeline" + URLOrg = pipelinesascode.GroupName + "/url-org" + URLRepository = pipelinesascode.GroupName + "/url-repository" + SHA = pipelinesascode.GroupName + "/sha" + Sender = pipelinesascode.GroupName + "/sender" + EventType = pipelinesascode.GroupName + "/event-type" + Branch = pipelinesascode.GroupName + "/branch" + SourceBranch = pipelinesascode.GroupName + "/source-branch" + Repository = pipelinesascode.GroupName + "/repository" + GitProvider = pipelinesascode.GroupName + "/git-provider" + State = pipelinesascode.GroupName + "/state" + ShaTitle = pipelinesascode.GroupName + "/sha-title" + ShaURL = pipelinesascode.GroupName + "/sha-url" + RepoURL = pipelinesascode.GroupName + "/repo-url" + SourceRepoURL = pipelinesascode.GroupName + "/source-repo-url" + PullRequest = pipelinesascode.GroupName + "/pull-request" + InstallationID = pipelinesascode.GroupName + "/installation-id" + GHEURL = pipelinesascode.GroupName + "/ghe-url" + SourceProjectID = pipelinesascode.GroupName + "/source-project-id" + TargetProjectID = pipelinesascode.GroupName + "/target-project-id" + OriginalPRName = pipelinesascode.GroupName + "/original-prname" + GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret" + CheckRunID = pipelinesascode.GroupName + "/check-run-id" + OnEvent = pipelinesascode.GroupName + "/on-event" + OnComment = pipelinesascode.GroupName + "/on-comment" + OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch" + OnPathChange = pipelinesascode.GroupName + "/on-path-change" + OnLabel = pipelinesascode.GroupName + "/on-label" + OnPathChangeIgnore = pipelinesascode.GroupName + "/on-path-change-ignore" + OnCelExpression = pipelinesascode.GroupName + "/on-cel-expression" + TargetNamespace = pipelinesascode.GroupName + "/target-namespace" + MaxKeepRuns = pipelinesascode.GroupName + "/max-keep-runs" + CancelInProgress = pipelinesascode.GroupName + "/cancel-in-progress" + LogURL = pipelinesascode.GroupName + "/log-url" + ExecutionOrder = pipelinesascode.GroupName + "/execution-order" + SCMReportingPLRStarted = pipelinesascode.GroupName + "/scm-reporting-plr-started" // PublicGithubAPIURL default is "https://api.github.com" but it can be overridden by X-GitHub-Enterprise-Host header. PublicGithubAPIURL = "https://api.github.com" GithubApplicationID = "github-application-id" diff --git a/pkg/kubeinteraction/labels.go b/pkg/kubeinteraction/labels.go index 846d6a2e0..50fcf2f85 100644 --- a/pkg/kubeinteraction/labels.go +++ b/pkg/kubeinteraction/labels.go @@ -55,7 +55,6 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu keys.SourceBranch: event.HeadBranch, keys.Repository: repo.GetName(), keys.GitProvider: providerConfig.Name, - keys.State: StateStarted, keys.ControllerInfo: fmt.Sprintf(`{"name":"%s","configmap":"%s","secret":"%s", "gRepo": "%s"}`, paramsinfo.Controller.Name, paramsinfo.Controller.Configmap, paramsinfo.Controller.Secret, paramsinfo.Controller.GlobalRepository), } diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index 4a1bdcbe4..c0384b9f7 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -176,14 +176,10 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi p.logger.Errorf("Error adding labels/annotations to PipelineRun '%s' in namespace '%s': %v", match.PipelineRun.GetName(), match.Repo.GetNamespace(), err) } - // if concurrency is defined then start the pipelineRun in pending state and - // state as queued + // if concurrency is defined then start the pipelineRun in pending state if match.Repo.Spec.ConcurrencyLimit != nil && *match.Repo.Spec.ConcurrencyLimit != 0 { // pending status match.PipelineRun.Spec.Status = tektonv1.PipelineRunSpecStatusPending - // pac state as queued - match.PipelineRun.Labels[keys.State] = kubeinteraction.StateQueued - match.PipelineRun.Annotations[keys.State] = kubeinteraction.StateQueued } // Create the actual pipelineRun @@ -240,23 +236,31 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi OriginalPipelineRunName: pr.GetAnnotations()[keys.OriginalPRName], } + // Patch the pipelineRun with the appropriate annotations and labels. + // Set the state so the watcher will continue with reconciling the pipelineRun + // The watcher reconciles only pipelineRuns that has the state annotation. patchAnnotations := map[string]string{} patchLabels := map[string]string{} whatPatching := "" // if pipelineRun is in pending state then report status as queued + // The pipelineRun can be pending because of PAC's concurrency limit or because of an external mutatingwebhook if pr.Spec.Status == tektonv1.PipelineRunSpecStatusPending { status.Status = queuedStatus if status.Text, err = mt.MakeTemplate(p.vcx.GetTemplate(provider.QueueingPipelineType)); err != nil { return nil, fmt.Errorf("cannot create message template: %w", err) } - // If the PipelineRun is in the "queued" state, add the appropriate label and annotation. - // These are later used by the watcher to determine whether the PipelineRun status - // should be reported back to the Git provider. We do add the `state` annotations and label when - // concurrency is enabled but this would happen when PipelineRun's status has been changed by - // the other controller and PaC is not aware of that change. whatPatching = "annotations.state and labels.state" patchAnnotations[keys.State] = kubeinteraction.StateQueued patchLabels[keys.State] = kubeinteraction.StateQueued + } else { + // Mark that the start will be reported to the Git provider + patchAnnotations[keys.SCMReportingPLRStarted] = "true" + patchAnnotations[keys.State] = kubeinteraction.StateStarted + patchLabels[keys.State] = kubeinteraction.StateStarted + whatPatching = fmt.Sprintf( + "annotation.%s and annotations.state and labels.state", + keys.SCMReportingPLRStarted, + ) } if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil { @@ -278,6 +282,20 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi // unneeded SIGSEGV's return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err) } + currentReason := "" + if len(pr.Status.GetConditions()) > 0 { + currentReason = pr.Status.GetConditions()[0].GetReason() + } + + p.logger.Infof("PipelineRun %s/%s patched successfully - Spec.Status: %s, State annotation: '%s', SCMReportingPLRStarted annotation: '%s', Status reason: '%s', Git provider status: '%s', Patched: %s", + pr.GetNamespace(), + pr.GetName(), + pr.Spec.Status, + pr.GetAnnotations()[keys.State], + pr.GetAnnotations()[keys.SCMReportingPLRStarted], + currentReason, + status.Status, + whatPatching) } return pr, nil diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 9b9f23e5c..17ed9ba2c 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -718,10 +718,19 @@ func TestRun(t *testing.T) { secretName, ok := pr.GetAnnotations()[keys.GitAuthSecret] assert.Assert(t, ok, "Cannot find secret %s on annotations", secretName) } - if tt.concurrencyLimit > 0 || pr.Spec.Status == pipelinev1.PipelineRunSpecStatusPending { + if pr.Spec.Status == pipelinev1.PipelineRunSpecStatusPending { state, ok := pr.GetAnnotations()[keys.State] assert.Assert(t, ok, "State hasn't been set on PR", state) assert.Equal(t, state, kubeinteraction.StateQueued) + + // When PipelineRun is queued, SCMReportingPLRStarted should not be set + _, scmStartedExists := pr.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, !scmStartedExists, "SCMReportingPLRStarted should not be set for queued PipelineRuns") + } else { + // When PipelineRun is not queued, SCMReportingPLRStarted should be set to "true" + scmStarted, scmStartedExists := pr.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, scmStartedExists, "SCMReportingPLRStarted should be set for non-queued PipelineRuns") + assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'") } } } diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 95aba5dc0..58493f289 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -54,6 +54,20 @@ var ( func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun) pkgreconciler.Event { ctx = info.StoreNS(ctx, system.Namespace()) logger := logging.FromContext(ctx).With("namespace", pr.GetNamespace()) + + logger.Debugf("reconciling pipelineRun %s/%s", pr.GetNamespace(), pr.GetName()) + + // make sure we have the latest pipelinerun to reconcile, since there is something updating at the same time + lpr, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("cannot get pipelineRun: %w", err) + } + + if lpr.GetResourceVersion() != pr.GetResourceVersion() { + logger.Debugf("Skipping reconciliation, pipelineRun was updated (cached version %s vs fresh version %s)", pr.GetResourceVersion(), lpr.GetResourceVersion()) + return nil + } + // if pipelineRun is in completed or failed state then return state, exist := pr.GetAnnotations()[keys.State] if exist && (state == kubeinteraction.StateCompleted || state == kubeinteraction.StateFailed) { @@ -69,7 +83,12 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun // another controller outside PaC). To maintain consistency between the PipelineRun // status and the Git provider status, we update both the PipelineRun resource and // the corresponding status on the Git provider here. - if reason == string(tektonv1.PipelineRunReasonRunning) && state == kubeinteraction.StateQueued { + scmReportingPLRStarted, exist := pr.GetAnnotations()[keys.SCMReportingPLRStarted] + startReported := exist && scmReportingPLRStarted == "true" + logger.Debugf("pipelineRun %s/%s scmReportingPLRStarted=%v, exist=%v", pr.GetNamespace(), pr.GetName(), startReported, exist) + + if reason == string(tektonv1.PipelineRunReasonRunning) && !startReported { + logger.Infof("pipelineRun %s/%s is running but not yet reported to provider, updating status", pr.GetNamespace(), pr.GetName()) repoName := pr.GetAnnotations()[keys.Repository] repo, err := r.repoLister.Repositories(pr.Namespace).Get(repoName) if err != nil { @@ -77,6 +96,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun } return r.updatePipelineRunToInProgress(ctx, logger, repo, pr) } + logger.Debugf("pipelineRun %s/%s condition not met: reason='%s', startReported=%v", pr.GetNamespace(), pr.GetName(), reason, startReported) // if its a GitHub App pipelineRun PR then process only if check run id is added otherwise wait if _, ok := pr.Annotations[keys.InstallationID]; ok { @@ -95,16 +115,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun return nil } - // make sure we have the latest pipelinerun to reconcile, since there is something updating at the same time - lpr, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{}) - if err != nil { - return fmt.Errorf("cannot get pipelineRun: %w", err) - } - - if lpr.GetResourceVersion() != pr.GetResourceVersion() { - return nil - } - // If we have a controllerInfo annotation, then we need to get the // configmap configuration for it // @@ -343,16 +353,24 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger * } func (r *Reconciler) updatePipelineRunState(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun, state string) (*tektonv1.PipelineRun, error) { + currentState := pr.GetAnnotations()[keys.State] + logger.Infof("updating pipelineRun %v/%v state from %s to %s", pr.GetNamespace(), pr.GetName(), currentState, state) + annotations := map[string]string{ + keys.State: state, + } + if state == kubeinteraction.StateStarted { + annotations[keys.SCMReportingPLRStarted] = "true" + } + mergePatch := map[string]any{ "metadata": map[string]any{ "labels": map[string]string{ keys.State: state, }, - "annotations": map[string]string{ - keys.State: state, - }, + "annotations": annotations, }, } + // if state is started then remove pipelineRun pending status if state == kubeinteraction.StateStarted { mergePatch["spec"] = map[string]any{ diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index e3c4001a1..b001de831 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -299,6 +299,189 @@ func TestUpdatePipelineRunState(t *testing.T) { assert.Equal(t, updatedPR.Annotations[keys.State], tt.state) assert.Equal(t, updatedPR.Spec.Status, tektonv1.PipelineRunSpecStatus("")) + + // Test SCMReportingPLRStarted annotation for started state + if tt.state == kubeinteraction.StateStarted { + scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, exists, "SCMReportingPLRStarted annotation should exist when state is started") + assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'") + } else { + _, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, !exists, "SCMReportingPLRStarted annotation should not exist for non-started states") + } + }) + } +} + +func TestReconcileKind_SCMReportingLogic(t *testing.T) { + observer, _ := zapobserver.New(zap.InfoLevel) + _ = zap.New(observer).Sugar() + + tests := []struct { + name string + pipelineRun *tektonv1.PipelineRun + shouldCallUpdateToInProgress bool + description string + }{ + { + name: "Running reason without SCMReportingPLRStarted - should call updatePipelineRunToInProgress", + pipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "test-pr", + Annotations: map[string]string{ + keys.State: kubeinteraction.StateQueued, + keys.Repository: "test-repo", + }, + }, + Spec: tektonv1.PipelineRunSpec{}, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + { + Type: knativeapi.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: string(tektonv1.PipelineRunReasonRunning), + }, + }, + }, + }, + }, + shouldCallUpdateToInProgress: true, + description: "PipelineRun is Running but no SCM reporting done yet", + }, + { + name: "Running reason with SCMReportingPLRStarted=true - should NOT call updatePipelineRunToInProgress", + pipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "test-pr", + Annotations: map[string]string{ + keys.State: kubeinteraction.StateStarted, + keys.Repository: "test-repo", + keys.SCMReportingPLRStarted: "true", + }, + }, + Spec: tektonv1.PipelineRunSpec{}, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + { + Type: knativeapi.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: string(tektonv1.PipelineRunReasonRunning), + }, + }, + }, + }, + }, + shouldCallUpdateToInProgress: false, + description: "PipelineRun is Running and SCM reporting already done", + }, + { + name: "Non-Running reason - should NOT call updatePipelineRunToInProgress", + pipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "test-pr", + Annotations: map[string]string{ + keys.State: kubeinteraction.StateQueued, + keys.Repository: "test-repo", + }, + }, + Spec: tektonv1.PipelineRunSpec{ + Status: tektonv1.PipelineRunSpecStatusPending, + }, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + { + Type: knativeapi.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: string(tektonv1.PipelineRunReasonPending), + }, + }, + }, + }, + }, + shouldCallUpdateToInProgress: false, + description: "PipelineRun is not in Running state", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + + testRepo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: tt.pipelineRun.GetNamespace(), + }, + Spec: v1alpha1.RepositorySpec{ + URL: randomURL, + }, + } + + testData := testclient.Data{ + Repositories: []*v1alpha1.Repository{testRepo}, + PipelineRuns: []*tektonv1.PipelineRun{tt.pipelineRun}, + } + stdata, informers := testclient.SeedTestData(t, ctx, testData) + + // Track if updatePipelineRunToInProgress was called by checking state changes + originalState := tt.pipelineRun.GetAnnotations()[keys.State] + + r := &Reconciler{ + repoLister: informers.Repository.Lister(), + run: ¶ms.Run{ + Clients: clients.Clients{ + Tekton: stdata.Pipeline, + }, + Info: info.Info{ + Pac: &info.PacOpts{ + Settings: settings.Settings{}, + }, + }, + }, + } + + err := r.ReconcileKind(ctx, tt.pipelineRun) + + // For test cases that should call updatePipelineRunToInProgress, + // we expect no error and the state should be updated + if tt.shouldCallUpdateToInProgress { + assert.NilError(t, err, tt.description) + + // Get the updated PipelineRun to check if state was changed + updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(tt.pipelineRun.GetNamespace()).Get(ctx, tt.pipelineRun.GetName(), metav1.GetOptions{}) + assert.NilError(t, getErr) + + // Should have been updated to started state with SCMReportingPLRStarted annotation + assert.Equal(t, updatedPR.GetAnnotations()[keys.State], kubeinteraction.StateStarted) + scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, exists, "SCMReportingPLRStarted should be set") + assert.Equal(t, scmStarted, "true") + } else { + // For cases that should NOT call updatePipelineRunToInProgress, + // the state should remain unchanged + updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(tt.pipelineRun.GetNamespace()).Get(ctx, tt.pipelineRun.GetName(), metav1.GetOptions{}) + assert.NilError(t, getErr) + + // State should be unchanged + assert.Equal(t, updatedPR.GetAnnotations()[keys.State], originalState, tt.description) + + // Check SCMReportingPLRStarted annotation based on original state + scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + if originalState == kubeinteraction.StateStarted { + // If original state was already 'started', SCMReportingPLRStarted should exist and be "true" + assert.Assert(t, exists, "SCMReportingPLRStarted should exist when original state is started") + assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'") + } else { + // If original state was not 'started', SCMReportingPLRStarted should not exist + assert.Assert(t, !exists, "SCMReportingPLRStarted should not exist when original state is not started") + } + } }) } }