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

abayer · 2022-04-06T18:05:56Z

Changes

See:

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
Runs, or both, building on top of all the other PRs referenced above.

/kind tep
/kind feature

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
(if there are no user facing changes, use release note "NONE")

Release Notes

Added implementation for minimal `TaskRun` and `Run` statuses within `PipelineRun` statuses.

abayer · 2022-04-06T18:06:11Z

/assign @lbernick
/assign @jerop

tekton-robot · 2022-04-06T18:10:19Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	84.0%	-5.5
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	86.3%	2.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

abayer · 2022-04-06T18:57:31Z

/retest

tekton-robot · 2022-04-06T19:13:24Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	84.0%	-5.5
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	86.3%	2.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

lbernick · 2022-04-06T18:17:20Z

test/v1alpha1/retry_test.go

@@ -44,6 +48,9 @@ func TestTaskRunRetry(t *testing.T) {
 	knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
 	defer tearDown(ctx, t, c, namespace)

+	// Skip if the "embedded-status" feature flag has any value but "full".


this comment appears stale

Nope, that's still relevant in my mind - v1alpha1 should, in theory, always be full. Though in practice because of how we have the type aliases set up, I guess you could mess around there. Eh, I'm fine either way.

either way makes sense, but the comment here doesn't reflect the code.

Oy, duh, fixed. =)

lbernick · 2022-04-06T20:05:37Z

pkg/reconciler/pipelinerun/cancel.go

+	// If pr.Status.ChildReferences is populated, use that as source of truth for TaskRun and Run names.
+	if len(pr.Status.ChildReferences) > 0 {
+		// Loop over the ChildReferences in the PipelineRun status.
+		for _, cr := range pr.Status.ChildReferences {


I might prefer a helper function which returns the runs and taskruns to cancel based on the embedded status value.

Lemme see what I can do here.

lbernick · 2022-04-06T20:11:15Z

pkg/reconciler/pipelinerun/pipelinerun.go


 	return nil
 }

-func updatePipelineRunStatusFromTaskRuns(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, trs []*v1beta1.TaskRun) {
+func updatePipelineRunStatusFromSlices(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, taskRuns []*v1beta1.TaskRun, runs []*v1alpha1.Run) {


nit: could this function be updatePipelineRunStatusFromChildObjects? IMO including things like "slices" in variable names can be redundant

Good idea! I did not like the name but couldn't think of a better one.

lbernick · 2022-04-06T20:12:30Z

pkg/reconciler/pipelinerun/pipelinerun.go

+	if fullEmbedded {
+		updatePipelineRunStatusFromTaskRuns(logger, pr, taskRuns, taskNameByPipelineTask)
+		updatePipelineRunStatusFromRuns(logger, pr, runs)
+	} else {


I don't think this else block should be necessary. If something depends on these values being non-nil let's change that code instead of initializing these.

Lemme see how much breaks if we don't initialize 'em.

Huh, nothing but tests - nice!

lbernick · 2022-04-06T20:14:55Z

pkg/reconciler/pipelinerun/pipelinerun.go

+// updatePipelineRunStatusFromTaskRuns takes a PipelineRun, a list of TaskRuns within that PipelineRun, and a map of
+// existing PipelineTask names to TaskRun (or Run) names, and updates the PipelineRun's .Status.TaskRuns, including
+// ensuring that any (deprecated) condition checks are represented.
+func updatePipelineRunStatusFromTaskRuns(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, trs []*v1beta1.TaskRun, taskRunNameByPipelineTask map[string]string) {


this function should be able to stay the same, no? (except for the small change removing the nil check for trs which I asked for :) )

The changes here are either minor cleanup/renames (I really didn't like the taskrun variable name - I twitch when I see a variable name overriding an imported package name!), necessary signature changes, or the condition check changes with getNewConditionChecksForTaskRun. That said, I'll unrename things and clean up one or two inadvertent changes I made.

cleanup changes are great! I think they belong in a separate PR though

At this point there's just the one bit of cleanup left - switching if trs == nil || len(trs) == 0 { to if len(trs) == 0 {.

(oops accidentally hit resolve!)

lbernick · 2022-04-06T20:20:04Z

pkg/reconciler/pipelinerun/pipelinerun.go

+	for _, tr := range trs {
+		// Only process TaskRuns that are owned by this PipelineRun.
+		// This skips TaskRuns that are indirectly created by the PipelineRun (e.g. by custom tasks).
+		if len(tr.OwnerReferences) < 1 || tr.OwnerReferences[0].UID != pr.ObjectMeta.UID {


this block is repeated in a few places, can it be reused?

It shows up twice for TaskRun and twice for Run - not sure if it's worth doing that as two functions, but I'm open to it.

these two functions in general share a lot of code, could you experiment to see how it can be consolidated?

So fwiw on this particular block, I just tried going with a helper function or two (i.e., either one function each for TaskRun and Run, or one generic one with a type switch) and it didn't really make things any clearer, since we still have to have a block here for the continue. I'm still seeing if there are any other shared bits I can consolidate, but this particular one doesn't feel worth doing.

Actually, turns out that once I decided to try doing the filtering of condition TaskRuns before entering the main loop, I saw the obvious and realized that we could just filter out the "not owned by this PipelineRun" TaskRuns and Runs before entering the main loop as well. 🤦 =) How does 6c8d615 look?

lbernick · 2022-04-06T20:22:55Z

pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go

+					actualPrStatus.ChildReferences = fixedChildRefs
+				}
+
+				// Sort the ChildReferences to deal with annoying ordering issues.


rather than sorting the output, I think you can provide extra inputs/flags to cmp.Diff to get it to ignore ordering of some fields.

Oh yeaaaah - I just cargo-culted that over from the original ...FromTaskRuns test blindly.

And actually, I'm not sure if cmpopts.SortSlices would make much of a difference here - it'd just be moving the same code around to be a parameter rather than standing alone...

lbernick · 2022-04-06T20:24:33Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -585,7 +594,13 @@ func getConditionCheckName(taskRunStatus map[string]*v1beta1.PipelineRunTaskRunS
 }

 // GetTaskRunName should return a unique name for a `TaskRun` if one has not already been defined, and the existing one otherwise.
-func GetTaskRunName(taskRunsStatus map[string]*v1beta1.PipelineRunTaskRunStatus, ptName, prName string) string {
+func GetTaskRunName(taskRunsStatus map[string]*v1beta1.PipelineRunTaskRunStatus, childRefs []v1beta1.ChildStatusReference, ptName, prName string) string {


super nit: I think you can import and use constants for the kind of Run and TaskRun (here and elsewhere in the PR)

I swear that I couldn't figure out where to find that to import. Maybe I missed something?

they're here but actually on second thought importing and using the controller name is a bit weird :/

Oh right, I did look at that and decided it felt too squirelly - I don't quite trust that the controller names are always going to be guaranteed to map one to one with the kinds. Feels like it might be worthwhile to add constants specifically for the kind name somewhere...

lbernick · 2022-04-06T20:28:30Z

test/custom_task_test.go

-	if len(pr.Status.Runs) != 2 {
-		t.Fatalf("PipelineRun had unexpected .status.runs; got %d, want 2", len(pr.Status.Runs))
+	var runNames []string
+	if embeddedStatusValue != config.MinimalEmbeddedStatus {


I think the branches are off here, you probably want

if embeddedStatusValue != config.MinimalEmbeddedStatus {} if embeddedStatusValue != config.FullEmbeddedStatus{}

both here and below (ensuring config.BothEmbeddedStatus is tested correctly)

Good catch!

same thing on line 218 and in custom_task_test

Well I feel a silly man. Fixed.

lbernick · 2022-04-06T20:32:28Z

test/featureflags.go

+
+// GetEmbeddedStatusValue gets the current value for the "embedded-status" feature flag.
+// If the flag is not set, it returns the default value.
+func GetEmbeddedStatusValue(ctx context.Context, t *testing.T, kubeClient kubernetes.Interface) string {


super nit: this function can just be called "getEmbeddedStatus"

tekton-robot · 2022-04-06T21:18:09Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	84.0%	-5.5
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	86.2%	2.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

tekton-robot · 2022-04-06T21:25:45Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	84.0%	-5.5
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	85.4%	1.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

lbernick · 2022-04-07T13:41:08Z

pkg/reconciler/pipelinerun/pipelinerun.go

+				len(prs.ChildReferences))
+		}
+
+		for _, cr := range prs.ChildReferences {


thanks for adding this function! just a few quick comments:

I would collect the names of child refs not in taskruns/runs and return an error containing all of them; this is more informative

make sure you're handling the case where childRef isn't a taskrun or a run

please add some tests for this function

👍 on all of that!

lbernick · 2022-04-07T13:44:56Z

pkg/reconciler/pipelinerun/pipelinerun.go


 	return nil
 }

-func updatePipelineRunStatusFromTaskRuns(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, trs []*v1beta1.TaskRun) {
+func updatePipelineRunStatusFromSlices(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, taskRuns []*v1beta1.TaskRun, runs []*v1alpha1.Run) {
+	taskNameByPipelineTask := make(map[string]string)


got it-- any way to factor out the condition checks and pass them into the updatePipelineRunStatus functions? it seems like the updatePipelineRunStatus functions could be made more modular: they should just take some info and use it to update the PR status, without having to return data that will be used in other updates

lbernick · 2022-04-07T13:45:49Z

pkg/reconciler/pipelinerun/pipelinerun.go

+	for _, tr := range trs {
+		// Only process TaskRuns that are owned by this PipelineRun.
+		// This skips TaskRuns that are indirectly created by the PipelineRun (e.g. by custom tasks).
+		if len(tr.OwnerReferences) < 1 || tr.OwnerReferences[0].UID != pr.ObjectMeta.UID {


these two functions in general share a lot of code, could you experiment to see how it can be consolidated?

lbernick · 2022-04-07T13:49:47Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -585,7 +594,13 @@ func getConditionCheckName(taskRunStatus map[string]*v1beta1.PipelineRunTaskRunS
 }

 // GetTaskRunName should return a unique name for a `TaskRun` if one has not already been defined, and the existing one otherwise.
-func GetTaskRunName(taskRunsStatus map[string]*v1beta1.PipelineRunTaskRunStatus, ptName, prName string) string {
+func GetTaskRunName(taskRunsStatus map[string]*v1beta1.PipelineRunTaskRunStatus, childRefs []v1beta1.ChildStatusReference, ptName, prName string) string {


they're here but actually on second thought importing and using the controller name is a bit weird :/

lbernick · 2022-04-07T13:50:28Z

test/custom_task_test.go

-	if len(pr.Status.Runs) != 2 {
-		t.Fatalf("PipelineRun had unexpected .status.runs; got %d, want 2", len(pr.Status.Runs))
+	var runNames []string
+	if embeddedStatusValue != config.MinimalEmbeddedStatus {


same thing on line 218 and in custom_task_test

lbernick · 2022-04-07T13:52:06Z

test/v1alpha1/retry_test.go

@@ -44,6 +48,9 @@ func TestTaskRunRetry(t *testing.T) {
 	knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
 	defer tearDown(ctx, t, c, namespace)

+	// Skip if the "embedded-status" feature flag has any value but "full".


either way makes sense, but the comment here doesn't reflect the code.

tekton-robot · 2022-04-07T18:11:47Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	93.2%	3.7
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	85.4%	1.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

abayer · 2022-04-07T18:43:09Z

/retest

abayer · 2022-04-07T18:43:39Z

/hold

Since I'm not squashing as I go, I'm putting this on hold until I get the thumbs-up for the whole PR and then squashing.

tekton-robot · 2022-04-07T19:33:22Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	93.2%	3.7
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	86.5%	2.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

lbernick · 2022-04-08T15:04:33Z

pkg/reconciler/pipelinerun/cancel.go

@@ -122,6 +127,45 @@ func cancelPipelineTaskRuns(ctx context.Context, logger *zap.SugaredLogger, pr *
 	return errs
 }

+// getChildObjectsToCancel looks in the PipelineRunStatus for TaskRun and Run names to cancel. It looks in


nit: this function comment describes the implementation, and the name describes how the function will be used.
I'd prefer something like:
// getChildObjects returns taskruns and runs owned by the pipelineRun, based on the value of the embedded status flag.

👍 I'm renaming it to filterChildObjectsFromPRStatus.

lbernick · 2022-04-08T15:04:48Z

pkg/reconciler/pipelinerun/cancel.go

-	// Loop over the TaskRuns in the PipelineRun status.
-	// If a TaskRun is not in the status yet we should not cancel it anyways.
-	for taskRunName := range pr.Status.TaskRuns {
+	trNames, runNames, err := getChildObjectsToCancel(ctx, pr.Status)


this is looking much better! :)

lbernick · 2022-04-08T15:05:47Z

pkg/reconciler/pipelinerun/cancel_test.go

 		pipelineRun: &v1beta1.PipelineRun{
 			ObjectMeta: metav1.ObjectMeta{Name: "test-pipeline-run-cancelled"},
 			Spec: v1beta1.PipelineRunSpec{
 				Status: v1beta1.PipelineRunSpecStatusCancelledDeprecated,
 			},
 		},
+	}, {
+		name:           "child-references",
+		embeddedStatus: config.BothEmbeddedStatus,


could you please add a test case for minimal embedded status?

lbernick · 2022-04-08T15:11:39Z

pkg/reconciler/pipelinerun/pipelinerun.go

-	return nil
+	updatePipelineRunStatusFromChildObjects(ctx, logger, pr, taskRuns, runs)
+
+	return validateChildObjectsInPipelineRunStatus(ctx, pr.Status)


I think this function call should be moved to updatePipelineRunStatusFromChildObjects

lbernick · 2022-04-08T15:19:55Z

pkg/reconciler/pipelinerun/pipelinerun.go


 	return nil
 }

-func updatePipelineRunStatusFromTaskRuns(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, trs []*v1beta1.TaskRun) {
+func updatePipelineRunStatusFromSlices(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, taskRuns []*v1beta1.TaskRun, runs []*v1alpha1.Run) {
+	taskNameByPipelineTask := make(map[string]string)


maybe a better pattern here could be one function for minimal embedded status, one function for full embedded status, and one function for both embedded status which calls both of the first two. I think this way none of them should need to return anything. thoughts?

tekton-robot · 2022-04-08T17:10:48Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	93.2%	3.7
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	86.5%	2.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

abayer · 2022-04-08T17:43:56Z

/retest

tekton-robot · 2022-04-08T18:06:13Z

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/pipeline/v1beta1/pipelinerun_types.go	86.4%	80.9%	-5.5
pkg/reconciler/pipelinerun/cancel.go	89.5%	93.2%	3.7
pkg/reconciler/pipelinerun/pipelinerun.go	84.2%	86.1%	1.9
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.7%	96.6%	-0.0
test/featureflags.go	Do not exist	0.0%

abayer · 2022-04-08T18:38:04Z

/retest

lbernick · 2022-04-08T19:23:21Z

pkg/reconciler/pipelinerun/cancel_test.go

 			d := test.Data{
 				PipelineRuns: []*v1beta1.PipelineRun{tc.pipelineRun},
 				TaskRuns:     tc.taskRuns,
 				Runs:         tc.runs,
 			}
 			ctx, _ := ttesting.SetupFakeContext(t)
+			cfg := config.NewStore(logtesting.TestLogger(t))


what's this for?

Ah, there's a line further down that needs to be removed. We need the context to be populated with the feature flags, hence this bit.

Oh, no, the later line is fine. Yeah, so what I said above. =)

I think you can just do cfg := withCustomTasks(withEmbeddedStatus(newFeatureFlagsConfigMap(), tc.embeddedStatus))

That creates a ConfigMap, but not a *config.Store, which is what we need to call cfg.ToContext(ctx). I tried hand-crafting a context.Context so I could skip the *config.Store in between, but couldn't make it work right for config.FromContextOrDefaults(ctx) to pick it up properly.

lbernick · 2022-04-08T19:28:20Z

test/custom_task_test.go

 		}
-		if r.IsDone() {
-			t.Fatalf("Run unexpectedly done: %v", r.Status.GetCondition(apis.ConditionSucceeded))
+		for rn := range pr.Status.Runs {


I'm a bit confused, what's going on here?

We need the run name later in the test.

oh I see, the slice has length 1-- in that case pr.Status.Runs[0] is more clear

Ah, right, I tried to do that, but pr.Status.Runs is a map, so pr.Status.Runs[0] isn't an option.

lbernick

/lgtm

lbernick · 2022-04-08T20:15:30Z

test/custom_task_test.go

 		}
-		if r.IsDone() {
-			t.Fatalf("Run unexpectedly done: %v", r.Status.GetCondition(apis.ConditionSucceeded))
+		for rn := range pr.Status.Runs {


oh I see, the slice has length 1-- in that case pr.Status.Runs[0] is more clear

lbernick · 2022-04-08T20:18:39Z

pkg/reconciler/pipelinerun/cancel_test.go

 			d := test.Data{
 				PipelineRuns: []*v1beta1.PipelineRun{tc.pipelineRun},
 				TaskRuns:     tc.taskRuns,
 				Runs:         tc.runs,
 			}
 			ctx, _ := ttesting.SetupFakeContext(t)
+			cfg := config.NewStore(logtesting.TestLogger(t))


I think you can just do cfg := withCustomTasks(withEmbeddedStatus(newFeatureFlagsConfigMap(), tc.embeddedStatus))

abayer · 2022-04-14T17:13:03Z

/hold cancel

Ok, hopefully this is the final form. =)

abayer · 2022-04-14T17:38:40Z

/retest

abayer · 2022-04-14T17:48:43Z

/retest

lbernick

/lgtm

abayer · 2022-04-14T17:56:37Z

/retest

abayer · 2022-04-14T18:27:47Z

/retest

lbernick · 2022-04-14T18:32:55Z

If build tests are failing this seems unlikely to be a flake?

abayer · 2022-04-14T18:38:30Z

the pull-tekton-pipeline-build-tests failure was pre-rebase, and it was a job that appears to not have properly started in the first place, getting marked as a failure for timing out. And literally while I was writing this, the current run passed. =)

abayer · 2022-04-14T18:38:48Z

/retest

tekton-robot · 2022-04-14T18:41:19Z

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/cancel.go	89.5%	93.2%	3.7
pkg/reconciler/pipelinerun/pipelinerun.go	85.5%	86.0%	0.6
test/featureflags.go	Do not exist	0.0%

abayer · 2022-04-14T18:57:16Z

cc @tektoncd/core-maintainers - I'd be extremely grateful for a final review/approve (well, assuming it's approval-worthy!), so I can finally get TEP-0100 put to bed for once and for all. =)

tekton-robot · 2022-04-15T07:42:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

JaneSoo · 2022-04-19T10:27:05Z

Hello, thank you all so much for this feature. How do I enable this feature? Should I add the flag in my pipelinerun? Do you have an example?

lbernick · 2022-04-19T13:39:31Z

Hi @JaneSoo, this will be available in the next release, and can be enabled via the feature-flags configmap as described here. Email announcement coming soon!

JaneSoo · 2022-04-19T15:40:39Z

Thanks a lot for the respond @lbernick

JaneSoo · 2022-04-20T16:17:01Z

@lbernick I don't think I am subscribed to an email. Could you tell me when is the release date?

lbernick · 2022-04-20T17:11:37Z

Please make sure you are subscribed to https://groups.google.com/g/tekton-users; we just sent out an email update (thanks for the reminder!) This release is scheduled for the 25th.

With tektoncd#4739 merged and the v0.35.0 release coming up, it's time to update the deprecations table. The plan is to switch the default value for `embedded-status` to `minimal` 9 months after the release of v0.35.0, and then remove the `full` and `both` values in the following release. I opted to use the date for changing the default as the "earliest date of removal" here. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

With #4739 merged and the v0.35.0 release coming up, it's time to update the deprecations table. The plan is to switch the default value for `embedded-status` to `minimal` 9 months after the release of v0.35.0, and then remove the `full` and `both` values in the following release. I opted to use the date for changing the default as the "earliest date of removal" here. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

With tektoncd#4739 merged and the v0.35.0 release coming up, it's time to update the deprecations table. The plan is to switch the default value for `embedded-status` to `minimal` 9 months after the release of v0.35.0, and then remove the `full` and `both` values in the following release. I opted to use the date for changing the default as the "earliest date of removal" here. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

This comes out of discussions on tektoncd#4739 - with the new minimal embedded status changes which will be introduced in that PR, we can see that we're currently using the output of `pipelineRunFacts.State.GetTaskRunsStatus(pr)` and `pipelineRunFacts.State.GetRunsStatus(pr)` for two separate purposes: * To set `pr.Status.TaskRuns` and `pr.Status.Runs` with the full embedded status * To pass to `resources.ApplyTaskResultsToPipelineResults` for populating results It's understandable why `ApplyTaskResultsToPipelineResults` is using the maps from `pr.Status.[TaskRuns|Runs]`, since those maps do contain everything needed for propagating results up from the tasks to the pipeline run, but if you look at the current implementation, you can see that it's shuffling the maps around into a different form that's more suited for what it's doing than the original form. So this PR reworks `ApplyTaskResultsToPipelineResults` to instead take maps in the form the current implementation uses internally, with new functions on `PipelineRunState` to get these new maps without needing to use the `pr.Status.[TaskRuns|Runs]` form as an intermediary. This makes the pre-minimal-embedded-status implementation cleaner, and is particularly helpful in that regard once we do have minimal embedded status in place. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

…d status This builds on tektoncd#4694, tektoncd#4734, and tektoncd#4753. It will feed into a revamped tektoncd#4739, all as part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md and tektoncd#3140. Specifically, this adds functionality to `pkg/reconciler/pipelinerun/resources` in `pipelinerunresolution.go` and `pipelinerunstate.go` which will be needed for the full implementation. These changes won't have any effects in the current situation, because `pr.Status.ChildReferences` is never populated, so can be made independently of the rest of the implementation, thus also shrinking the size of the rest of the implementation PR(s) for easier review. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

With tektoncd#4739 merged and the v0.35.0 release coming up, it's time to update the deprecations table. The plan is to switch the default value for `embedded-status` to `minimal` 9 months after the release of v0.35.0, and then remove the `full` and `both` values in the following release. I opted to use the date for changing the default as the "earliest date of removal" here. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). kind/feature Categorizes issue or PR as related to a new feature. labels Apr 6, 2022

tekton-robot requested review from pritidesai and vdemeester April 6, 2022 18:06

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 6, 2022

tekton-robot assigned jerop and lbernick Apr 6, 2022

lbernick reviewed Apr 6, 2022

View reviewed changes

lbernick reviewed Apr 7, 2022

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2022

lbernick reviewed Apr 8, 2022

View reviewed changes

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

abayer force-pushed the embedded-statuses-impl-mk2 branch from c934e21 to af6055a Compare April 8, 2022 20:26

abayer force-pushed the embedded-statuses-impl-mk2 branch from 7990ce4 to ce12e81 Compare April 14, 2022 17:12

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2022

lbernick reviewed Apr 14, 2022

View reviewed changes

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

vdemeester approved these changes Apr 15, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2022

tekton-robot merged commit 940a7bb into tektoncd:main Apr 15, 2022

abayer mentioned this pull request Apr 20, 2022

[TEP-0100] Update deprecations table #4781

Merged

5 tasks

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

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

Conversation

abayer commented Apr 6, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

abayer commented Apr 6, 2022

tekton-robot commented Apr 6, 2022

abayer commented Apr 6, 2022

tekton-robot commented Apr 6, 2022

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 6, 2022

tekton-robot commented Apr 6, 2022

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 Apr 7, 2022

abayer commented Apr 7, 2022

abayer commented Apr 7, 2022

tekton-robot commented Apr 7, 2022

Choose a reason for hiding this comment

abayer Apr 8, 2022 • edited Loading

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 Apr 8, 2022

abayer commented Apr 8, 2022

tekton-robot commented Apr 8, 2022

abayer commented Apr 8, 2022

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

abayer commented Apr 6, 2022 •

edited

Loading

abayer Apr 8, 2022 •

edited

Loading