Fix PipelineRunStatus Reconciler for EmbeddedStatus Switch

[JeromeJu]

Changes

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

/kind bug

fixes: - #5991
part of: #4954 #5833

Thanks to @abayer @XinruZhang @lbernick

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• [n/a] Has Docs included if any changes are user facing
• Has Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including
  functionality, content, code)
• Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

fixes 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.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-using-tekton to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[JeromeJu]

I also thought https://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/pipelinerun.go#L207-L214 shall be wrapped in a condition check of Full or Both embeddedStatus according to the comments at https://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/pipelinerun.go#L826

But that doesn't seem to be the fix,

if cfg.FeatureFlags.EmbeddedStatus == config.FullEmbeddedStatus || cfg.FeatureFlags.EmbeddedStatus == config.BothEmbeddedStatus {

Would you mind helping to take a look @abayer ?

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[abayer]

I also thought https://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/pipelinerun.go#L207-L214 shall be wrapped in a condition check of Full or Both embeddedStatus according to the comments at https://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/pipelinerun.go#L826

But that doesn't seem to be the fix,

if cfg.FeatureFlags.EmbeddedStatus == config.FullEmbeddedStatus || cfg.FeatureFlags.EmbeddedStatus == config.BothEmbeddedStatus {

Aaaah, right - so with minimal, updateTaskRunsStatusDirectly and updateRunsStatusDirectly are effectively no-ops: if there's nothing in pr.Status.TaskRuns or pr.Status.Runs, respectively, they don't have anything to update in the first place. Since we never populate those fields with minimal, nothing happens when calling these functions.

Lemme try your PR out locally and see if I can figure out what's wrong.

[abayer]

Ah-ha! It's test fixtures with .status.runs: {} and/or .status.taskRuns: {} that's the problem. With your change, when the embedded status is minimal, those fields will no longer be empty maps, but will instead be nil. A commit that fixes this can be found at abayer@ca0dd7d

[JeromeJu]

Thanks @abayer for the fix, I will integrate this with defaulting the minimal embeddedStatus in #5934.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[abayer]

/approve

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[lbernick]

@JeromeJu can you please add code comments that explain the reasons for the change and link to the TODO? That way, people don't have to context switch to the pull request when they're looking at the code in order to find the reason for the change (although we still want to keep this info in the pull request description)

[JeromeJu]

@JeromeJu can you please add code comments that explain the reasons for the change and link to the TODO? That way, people don't have to context switch to the pull request when they're looking at the code in order to find the reason for the change (although we still want to keep this info in the pull request description)

Thanks Lee! That way makes more sense, updated.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, chitrangpatel, lbernick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [abayer,lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.1

[lbernick]

/lgtm

[lbernick]

@JeromeJu can you please update the release notes with details of the bug fix?

[JeromeJu]

@JeromeJu can you please update the release notes with details of the bug fix?

Updated. Thanks for reminding:)