[TEP0100] Default to minimal embedded-status

[JeromeJu]

Changes

This commit changes the default of embedded-status to minimal status.

Prior to this PR, the DefaultEmbeddedStatus is full, the pipelineRun reconciler
will populate both taskruns and runs for pipelineRunStatus. With the change
to minimal, the reconciler populates childReferences instead.

fixes: #5833
part of: #4954

Submitter Checklist

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

• 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

switch the default value of `embedded-status` to `minimal`
the `embedded-status` flag along with the `both` and `full` functionalities are deprecated, and will be removed in v0.45.

[tekton-robot]

@JeromeJu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test pull-tekton-pipeline-alpha-integration-tests
• /test pull-tekton-pipeline-build-tests
• /test pull-tekton-pipeline-integration-tests
• /test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

• /test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lbernick]

/hold cancel

[lbernick]

@JeromeJu the defaults will also need to be changed in https://github.com/tektoncd/pipeline/blob/main/pkg/apis/config/feature_flags.go

[XinruZhang]

Agreed with @lbernick that you will need to update

pipeline/pkg/apis/config/feature_flags.go

Line 82 in c39fd32

DefaultEmbeddedStatus = FullEmbeddedStatus

I assume there could be some unit tests failures because of the default value switch.

[jerop]

Thank you for taking this on @JeromeJu!

You may also need to update feature_flags.go
https://github.com/tektoncd/pipeline/blob/main/pkg/apis/config/feature_flags.go#L79-L80

[JeromeJu]

This PR has not been updated because of the debugging at #5989 which blocks this PR.

Thanks to @XinruZhang for helping out! And thank you for the reviews @lbernick @jerop , will update this PR once #5989 is merged.

[afrittoli]

NIT: PipelineRunStatus -> PipelineRunStatus

The link points to the whole TEP, perhaps it could point to a specific section? It's not clear to me what is meant by "PipelineRunStatus migration". I think the other values of the flag will be deprecated and eventually removed.

Now that the default is set to "minimal", shouldn't we deprecate the other values? They should be added to https://github.com/tektoncd/pipeline/blob/main/docs/deprecations.md and we could link to that document instead

[JeromeJu]

Thanks @afrittoli , done with the release note and NIT suggestion!

[JeromeJu]

Now that the default is set to "minimal", shouldn't we deprecate the other values? They should be added to https://github.com/tektoncd/pipeline/blob/main/docs/deprecations.md and we could link to that document instead

Thanks @afrittoli , I was under the impression that status.taskruns and status.runs were added to https://github.com/tektoncd/pipeline/blob/main/docs/deprecations.md as in TEP100 to indiciate the removal process. I am willing to add this into the deprecation.md in this same PR and link to it if preferred. But from the api_compatibility_policy I might not fully getting the picture but it seems that feature-flags values are not subject to the deprecation.md?

[afrittoli]

Yes, indeed status.taskruns and status.runs are in the deprecation list, which should be or else we could not remove them in the next release :)

About feature flags, in the TEP we said that we cannot remove flags associated to changes in behaviour - see https://github.com/tektoncd/community/blob/main/teps/0033-tekton-feature-gates.md#changes-in-behavior - but this change of behaviour is reflected into the API, so it doesn't make sense to keep the flag around once the API field it controls is deprecated and removed.

That said, we still should give notice to our users about upcoming changes, so we should include in the release notes and ideally in https://github.com/tektoncd/pipeline/blob/main/docs/deprecations.md too that this flag is deprecated, before we remove it in the next release.

[JeromeJu]

Thanks @afrittoli ! That makes sense, updated and linked in deprecation.md and the release note as well.

[afrittoli]

Thank you for this! It looks great, just a couple of comments.
The main question I have is that I think we should start the deprecation counter on the other flag values right away.

[afrittoli]

Looking at #4954, I think we should clearly state that the flag along with the both and full functionalities are deprecated, and will be removed in v0.45. It should be in the release notes as well!

[JeromeJu]

/retest

[JeromeJu]

/retest

[abayer]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer

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]

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

[afrittoli]

Thank you!
/lgtm