Fix PipelineRunStatus Reconciler Updates and Tests for EmbeddedStatus…

… Switch

This commit fixes the updates for reconciling PipelineRunStatus when switching
the EmbeddedStatus feature flag. It resets the status.runs and status.taskruns
to nil with minimal EmbeddedStatus and status.childReferences to nil with
full EmbeddedStatus.

Prior to this change, the childReferences runs and taskruns in pipelineRunStatus
would not have been reset to nil and could have led to validation error when the feature
flag is being switched. The following issue could help prevent such bugs in the future:
Integration tests for changing feature flags #5999

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -1331,20 +1331,32 @@ func (c *Reconciler) updatePipelineRunStatusFromInformer(ctx context.Context, pr
 
 func updatePipelineRunStatusFromChildObjects(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, taskRuns []*v1beta1.TaskRun, runObjects []v1beta1.RunObject) error {
 	cfg := config.FromContextOrDefaults(ctx)
-	fullEmbedded := cfg.FeatureFlags.EmbeddedStatus == config.FullEmbeddedStatus || cfg.FeatureFlags.EmbeddedStatus == config.BothEmbeddedStatus
-	minimalEmbedded := cfg.FeatureFlags.EmbeddedStatus == config.MinimalEmbeddedStatus || cfg.FeatureFlags.EmbeddedStatus == config.BothEmbeddedStatus
 
-	if minimalEmbedded {
-		updatePipelineRunStatusFromChildRefs(logger, pr, taskRuns, runObjects)
-	}
-	if fullEmbedded {
+	switch cfg.FeatureFlags.EmbeddedStatus {
+	case config.FullEmbeddedStatus:
+		// Clear any ChildReferences that are present.
+		// This can occur if the value of "embedded-status" was modified during PipelineRun execution.
+		pr.Status.ChildReferences = nil
+		updatePipelineRunStatusFromTaskRuns(logger, pr, taskRuns)
+		updatePipelineRunStatusFromCustomRunsOrRuns(logger, pr, runObjects)
+	case config.BothEmbeddedStatus:
 		updatePipelineRunStatusFromTaskRuns(logger, pr, taskRuns)
 		updatePipelineRunStatusFromCustomRunsOrRuns(logger, pr, runObjects)
+		updatePipelineRunStatusFromChildRefs(logger, pr, taskRuns, runObjects)
+	default:
+		// Clear any TaskRuns and Runs present in the status.
+		// This can occur if the value of "embedded-status" was modified during PipelineRun execution.
+		pr.Status.Runs = nil
+		pr.Status.TaskRuns = nil
+		updatePipelineRunStatusFromChildRefs(logger, pr, taskRuns, runObjects)
 	}
 
 	return validateChildObjectsInPipelineRunStatus(ctx, pr.Status)
 }
 
+// TODO: https://github.com/tektoncd/pipeline/issues/5999 integration tests for changing
+// feature flags could help prevent validation errors when the feature-flag is being switched.
+// In such case, the error could keep the pipelineRun run indefinitely.
 func validateChildObjectsInPipelineRunStatus(ctx context.Context, prs v1beta1.PipelineRunStatus) error {
 	cfg := config.FromContextOrDefaults(ctx)
 

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -5161,7 +5161,7 @@ status:
 		expectedPr = expectedPrMinimalStatus
 	}
 
-	if d := cmp.Diff(expectedPr, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreCompletionTime, ignoreStartTime); d != "" {
+	if d := cmp.Diff(expectedPr, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreCompletionTime, ignoreStartTime, cmpopts.EquateEmpty()); d != "" {
 		t.Errorf("expected to see pipeline run results created. Diff %s", diff.PrintWantGot(d))
 	}
 }
@@ -5482,7 +5482,7 @@ status:
 		expectedPr = expectedPrMinimalStatus
 	}
 
-	if d := cmp.Diff(expectedPr, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreCompletionTime, ignoreStartTime); d != "" {
+	if d := cmp.Diff(expectedPr, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreCompletionTime, ignoreStartTime, cmpopts.EquateEmpty()); d != "" {
 		t.Errorf("expected to see pipeline run results created. Diff %s", diff.PrintWantGot(d))
 	}
 }
@@ -8947,7 +8947,7 @@ spec:
 			if err != nil {
 				t.Fatalf("Got an error getting reconciled run out of fake client: %s", err)
 			}
-			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime); d != "" {
+			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime, cmpopts.EquateEmpty()); d != "" {
 				t.Errorf("expected PipelineRun was not created. Diff %s", diff.PrintWantGot(d))
 			}
 		})
@@ -9555,7 +9555,7 @@ spec:
 			if err != nil {
 				t.Fatalf("Got an error getting reconciled run out of fake client: %s", err)
 			}
-			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime); d != "" {
+			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime, cmpopts.EquateEmpty()); d != "" {
 				t.Errorf("expected PipelineRun was not created. Diff %s", diff.PrintWantGot(d))
 			}
 		})
@@ -10015,7 +10015,7 @@ status:
 			if err != nil {
 				t.Fatalf("Got an error getting reconciled run out of fake client: %s", err)
 			}
-			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, cmpopts.SortSlices(lessChildReferences)); d != "" {
+			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, cmpopts.SortSlices(lessChildReferences), cmpopts.EquateEmpty()); d != "" {
 				t.Errorf("expected PipelineRun was not created. Diff %s", diff.PrintWantGot(d))
 			}
 		})
@@ -10549,7 +10549,7 @@ spec:
 			if err != nil {
 				t.Fatalf("Got an error getting reconciled run out of fake client: %s", err)
 			}
-			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime); d != "" {
+			if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime, cmpopts.EquateEmpty()); d != "" {
 				t.Errorf("expected PipelineRun was not created. Diff %s", diff.PrintWantGot(d))
 			}
 		})