[TEP-0100] Fields/flags/docs for embedded TaskRun and Run statuses in PipelineRuns #4705

abayer · 2022-03-23T16:16:07Z

Changes

Replaces #4694 (in part - there'll be a followup for the actual implementation)

See https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md

This adds new choices for how TaskRun and Run statuses are stored/referenced in PipelineRun
statuses, depending on how the new feature flag, embedded-status is set:

full - the current default, with the TaskRun and Run statuses embedded in full.
minimal - instead of storing the full embedded statuses, information is stored about the task's
version, kind, PipelineTask name, and the underlying name of the TaskRun or Run. This can be
used to look up the TaskRun or Run itself and get the full status from there. Information
which is specific to the PipelineRun, namely condition checks status and when expressions which
may have blocked creation of an actual TaskRun or Run are stored as well.
both - both the full embedded TaskRun and Run statuses and the minimal references are stored.

The condition check information will be removed from the minimal references once the now-deprecated
condition check functionality is removed from Tekton Pipeline.

As described in https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md#beta-api,
9 months after this change has been released in v1beta1, the default value for the
embedded-status feature flag will be changed from full to minimal, and a few months after
that, the embedded-status feature flag will be removed entirely and only the minimal behavior
will remain.

Note that this also may address #4657, since I had to add documentation for PipelineRunStatus,
and while I was there, I added some missing top-level fields to the PipelineRun definition.

The actual implementation will follow this PR.

/kind tep
/kind api-change

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Added syntactic support and feature flags for minimal `TaskRun` and `Run` statuses within `PipelineRun` statuses.

abayer · 2022-03-23T16:16:21Z

/assign @lbernick
/assign @jerop

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 This implements TEP-0100, building on top of the flags/fields/docs changes in tektoncd#4705. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2022-03-23T16:18:59Z

/assign @jerop

abayer · 2022-03-23T16:19:24Z

Fun, assign isn't working, I guess.

tekton-robot · 2022-03-23T16:45:11Z

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/apis/config/feature_flags.go	86.0%	84.6%	-1.4

abayer · 2022-03-23T17:11:59Z

/retest

tekton-robot · 2022-03-23T19:37:41Z

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/apis/config/feature_flags.go	86.0%	84.6%	-1.4

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 This implements TEP-0100, building on top of the flags/fields/docs changes in tektoncd#4705. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2022-03-23T20:06:59Z

/retest

lbernick

Thanks Andrew!

lbernick · 2022-03-24T13:40:05Z

docs/install.md

+- `embedded-status`: set this flag to "full" to enable full embedding of `TaskRun` and `Run` statuses in the 
+ `PipelineRun` status. Set it to "minimal" to populate the `ChildReferences` field in the `PipelineRun` status with
+  name, kind, and API version information for each `TaskRun` and `Run` in the `PipelineRun` instead. Set it to "both" to 
+  do both. For mroe information, see [Configuring usage of `TaskRun` and `Run` embedded statuses](pipelineruns.md#configuring-usage-of-taskrun-and-run-embedded-statuses)


typo: mroe -> more

Also I would just add a quick note that this isn't implemented yet, which you can remove in the follow-up PR.

lbernick · 2022-03-24T13:44:11Z

pkg/apis/config/testdata/feature-flags-invalid-embedded-status.yaml

+  name: feature-flags
+  namespace: tekton-pipelines
+data:
+  enable-api-fields: "im-not-a-valid-feature-gate"


I think this should be embedded-status

I'm ashamed of how many YAML files I messed up like this. =)

lbernick · 2022-03-24T13:45:33Z

pkg/apis/pipeline/v1beta1/pipelinerun_types.go

+	PipelineTaskName string `json:"pipelineTaskName,omitempty"`
+
+	// ConditionChecks is the the list of condition checks, including their names and statuses, for the PipelineTask.
+	// Deprecated: This field will be removed when conditions are removed for v1.


nit: conditions may be removed in v1beta1 as well according to our compatibility policy so I'd just remove the "v1" bit

lbernick · 2022-03-24T13:50:02Z

pkg/apis/pipeline/v1beta1/pipelinerun_types.go

@@ -408,10 +427,12 @@ type PipelineRunStatusFields struct {
 	// +optional
 	CompletionTime *metav1.Time `json:"completionTime,omitempty"`

+	// Deprecated - use ChildReferences instead.


optional comment: I wonder if it's possible to add an annotation that will allow these fields to display as deprecated in code editors? I did a quick search and see // +k8s:prerelease-lifecycle-gen:deprecated=1.20 for some stuff in our k8s dependencies so it's probably possible

That'd be pretty dang nifty - mind opening an issue for that separate of this? It seems like something worth doing comprehensively.

lbernick · 2022-03-24T13:54:47Z

test/ignore_test.go.ignore

+	fmt.Printf("yaml:\n%s\n", string(ty))
+}
+
+func TestCompBetaAlpha(t *testing.T) {


I'm a bit confused about what this test is for; this almost looks like a test for pipelineresources? why is it called ignore_test? I'm not sure the changes you're introducing need a new e2e test.

Hahaha, this is detritus from who knows when that was still sitting in my local clone and somehow inadvertently got added. Nuking it now!

tekton-robot · 2022-03-24T14:18:16Z

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/apis/config/feature_flags.go	86.0%	88.5%	2.4

lbernick · 2022-03-24T14:24:20Z

/lgtm

… PipelineRuns See https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md This adds new choices for how `TaskRun` and `Run` statuses are stored/referenced in `PipelineRun` statuses, depending on how the new feature flag, `embedded-status` is set: * `full` - the current default, with the `TaskRun` and `Run` statuses embedded in full. * `minimal` - instead of storing the full embedded statuses, information is stored about the task's version, kind, `PipelineTask` name, and the underlying name of the `TaskRun` or `Run`. This can be used to look up the `TaskRun` or `Run` itself and get the full status from there. Information which is specific to the `PipelineRun`, namely condition checks status and when expressions which may have blocked creation of an actual `TaskRun` or `Run` are stored as well. * `both` - both the full embedded `TaskRun` and `Run` statuses and the minimal references are stored. The condition check information will be removed from the minimal references once the now-deprecated condition check functionality is removed from Tekton Pipeline. As described in https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md#beta-api, 9 months after this change has been released in `v1beta1`, the default value for the `embedded-status` feature flag will be changed from `full` to `minimal`, and a few months after that, the `embedded-status` feature flag will be removed entirely and only the `minimal` behavior will remain. Note that this also may address tektoncd#4657, since I had to add documentation for `PipelineRunStatus`, and while I was there, I added some missing top-level fields to the `PipelineRun` definition. The actual implementation will follow this PR. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 This implements TEP-0100, building on top of the flags/fields/docs changes in tektoncd#4705. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#3140 This implements TEP-0100, building on top of the flags/fields/docs changes in tektoncd#4705. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#3140 This implements TEP-0100, building on top of the flags/fields/docs changes in tektoncd#4705 and the test changes in tektoncd#4734. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of the flags/fields/docs changes in tektoncd#4705 and the test changes in Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (tektoncd#4705, tektoncd#4734, tektoncd#4753, tektoncd#4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (#4705, #4734, #4753, #4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#4753 * tektoncd#4757 * tektoncd#4760 * tektoncd#3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * #4705 * #4734 * #4753 * #4757 * #4760 * #3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (tektoncd#4705, tektoncd#4734, tektoncd#4753, tektoncd#4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#4753 * tektoncd#4757 * tektoncd#4760 * tektoncd#3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

This was referenced Mar 23, 2022

[TEP-0100] Implementation for embedded TaskRun and Run statuses in PipelineRuns #4706

Closed

WIP: [TEP-0100] Embedded TaskRun and Run statuses in PipelineRuns #4694

Closed

tekton-robot assigned jerop and lbernick Mar 23, 2022

tekton-robot requested review from bobcatfish and vdemeester March 23, 2022 16:43

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 23, 2022

abayer force-pushed the embedded-statuses-fields-flags-docs branch from df1e8aa to e2b4d76 Compare March 23, 2022 19:35

lbernick reviewed Mar 24, 2022

View reviewed changes

abayer force-pushed the embedded-statuses-fields-flags-docs branch from e2b4d76 to 8573baa Compare March 24, 2022 14:15

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022

abayer force-pushed the embedded-statuses-fields-flags-docs branch from 8573baa to 08ac11e Compare March 24, 2022 14:55

tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 24, 2022

abayer force-pushed the embedded-statuses-fields-flags-docs branch from 08ac11e to e02670d Compare March 24, 2022 15:07

abayer mentioned this pull request Apr 4, 2022

IGNORE ME abayer/tektoncd-pipeline#2

Open

abayer mentioned this pull request Apr 6, 2022

[TEP-0100] Implementation for embedded TaskRun and Run statuses in PipelineRuns #4739

Merged

5 tasks

abayer mentioned this pull request Apr 14, 2022

[TEP-0100] Add new updatePipelineRunStatusFromChildRefs function #4760

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0100] Fields/flags/docs for embedded TaskRun and Run statuses in PipelineRuns #4705

[TEP-0100] Fields/flags/docs for embedded TaskRun and Run statuses in PipelineRuns #4705

abayer commented Mar 23, 2022

abayer commented Mar 23, 2022

abayer commented Mar 23, 2022

abayer commented Mar 23, 2022

tekton-robot commented Mar 23, 2022

abayer commented Mar 23, 2022

tekton-robot commented Mar 23, 2022

abayer commented Mar 23, 2022

lbernick left a comment

lbernick Mar 24, 2022

abayer Mar 24, 2022

lbernick Mar 24, 2022

abayer Mar 24, 2022

lbernick Mar 24, 2022

abayer Mar 24, 2022

lbernick Mar 24, 2022

abayer Mar 24, 2022

lbernick Mar 24, 2022

abayer Mar 24, 2022

tekton-robot commented Mar 24, 2022

lbernick commented Mar 24, 2022

[TEP-0100] Fields/flags/docs for embedded TaskRun and Run statuses in PipelineRuns #4705

[TEP-0100] Fields/flags/docs for embedded TaskRun and Run statuses in PipelineRuns #4705

Conversation

abayer commented Mar 23, 2022

Changes

Submitter Checklist

Release Notes

abayer commented Mar 23, 2022

abayer commented Mar 23, 2022

abayer commented Mar 23, 2022

tekton-robot commented Mar 23, 2022

abayer commented Mar 23, 2022

tekton-robot commented Mar 23, 2022

abayer commented Mar 23, 2022

lbernick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Mar 24, 2022

lbernick commented Mar 24, 2022