[TEP-0133] Apply default resolver to finally tasks

Fixes tektoncd#6449. Prior to this commit, the default resolver is only applied to PipelineTasks but not finally PipelineTasks. This commit applies the default resolver to the finally PipelineTasks in the same way as normal PipelineTasks. This commit also refactors the common code to a PipelineTask SetDefaults function. /kind bug [tektoncd#6449]: tektoncd#6449
QuanZhang-William · Mar 31, 2023 · 29b6455 · 29b6455
1 parent b51f08c
commit 29b6455
Show file tree

Hide file tree

Showing 4 changed files with 326 additions and 244 deletions.
diff --git a/pkg/apis/pipeline/v1/pipeline_defaults.go b/pkg/apis/pipeline/v1/pipeline_defaults.go
@@ -32,34 +32,32 @@ func (p *Pipeline) SetDefaults(ctx context.Context) {
 
 // SetDefaults sets default values for the PipelineSpec's Params, Tasks, and Finally
 func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
-	cfg := config.FromContextOrDefaults(ctx)
 	for i := range ps.Params {
 		ps.Params[i].SetDefaults(ctx)
 	}
 
 	for _, pt := range ps.Tasks {
-		if pt.TaskRef != nil {
-			if pt.TaskRef.Kind == "" {
-				pt.TaskRef.Kind = NamespacedTaskKind
-			}
-			if pt.TaskRef.Name == "" && pt.TaskRef.Resolver == "" {
-				pt.TaskRef.Resolver = ResolverName(cfg.Defaults.DefaultResolverType)
-			}
-		}
-		if pt.TaskSpec != nil {
-			pt.TaskSpec.SetDefaults(ctx)
-		}
+		pt.SetDefaults(ctx)
 	}
 
 	for _, ft := range ps.Finally {
 		ctx := ctx // Ensure local scoping per Task
-		if ft.TaskRef != nil {
-			if ft.TaskRef.Kind == "" {
-				ft.TaskRef.Kind = NamespacedTaskKind
-			}
+		ft.SetDefaults(ctx)
+	}
+}
+
+// SetDefaults sets default values for a PipelineTask
+func (pt *PipelineTask) SetDefaults(ctx context.Context) {
+	cfg := config.FromContextOrDefaults(ctx)
+	if pt.TaskRef != nil {
+		if pt.TaskRef.Kind == "" {
+			pt.TaskRef.Kind = NamespacedTaskKind
 		}
-		if ft.TaskSpec != nil {
-			ft.TaskSpec.SetDefaults(ctx)
+		if pt.TaskRef.Name == "" && pt.TaskRef.Resolver == "" {
+			pt.TaskRef.Resolver = ResolverName(cfg.Defaults.DefaultResolverType)
 		}
 	}
+	if pt.TaskSpec != nil {
+		pt.TaskSpec.SetDefaults(ctx)
+	}
 }
diff --git a/pkg/apis/pipeline/v1/pipeline_defaults_test.go b/pkg/apis/pipeline/v1/pipeline_defaults_test.go
@@ -38,38 +38,13 @@ func TestPipeline_SetDefaults(t *testing.T) {
 
 func TestPipelineSpec_SetDefaults(t *testing.T) {
 	cases := []struct {
-		desc     string
-		ps       *v1.PipelineSpec
-		want     *v1.PipelineSpec
-		defaults map[string]string
+		desc string
+		ps   *v1.PipelineSpec
+		want *v1.PipelineSpec
 	}{{
 		desc: "empty pipelineSpec must not change after setting defaults",
 		ps:   &v1.PipelineSpec{},
 		want: &v1.PipelineSpec{},
-	}, {
-		desc: "pipeline task - default task kind must be " + string(v1.NamespacedTaskKind),
-		ps: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo", TaskRef: &v1.TaskRef{Name: "foo-task"},
-			}},
-		},
-		want: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo", TaskRef: &v1.TaskRef{Name: "foo-task", Kind: v1.NamespacedTaskKind},
-			}},
-		},
-	}, {
-		desc: "final pipeline task - default task kind must be " + string(v1.NamespacedTaskKind),
-		ps: &v1.PipelineSpec{
-			Finally: []v1.PipelineTask{{
-				Name: "final-task", TaskRef: &v1.TaskRef{Name: "foo-task"},
-			}},
-		},
-		want: &v1.PipelineSpec{
-			Finally: []v1.PipelineTask{{
-				Name: "final-task", TaskRef: &v1.TaskRef{Name: "foo-task", Kind: v1.NamespacedTaskKind},
-			}},
-		},
 	}, {
 		desc: "param type - default param type must be " + string(v1.ParamTypeString),
 		ps: &v1.PipelineSpec{
@@ -98,102 +73,170 @@ func TestPipelineSpec_SetDefaults(t *testing.T) {
 			}},
 		},
 	}, {
-		desc: "pipeline task with taskSpec - default param type must be " + string(v1.ParamTypeString),
+		desc: "multiple tasks and final tasks",
 		ps: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo", TaskSpec: &v1.EmbeddedTask{
-					TaskSpec: v1.TaskSpec{
-						Params: []v1.ParamSpec{{
-							Name: "string-param",
-						}},
+			Tasks: []v1.PipelineTask{
+				{
+					Name:    "task-with-taskRef",
+					TaskRef: &v1.TaskRef{Name: "bar-task-1"},
+				},
+				{
+					Name: "task-with-taskSpec",
+					TaskSpec: &v1.EmbeddedTask{
+						TaskSpec: v1.TaskSpec{
+							Params: []v1.ParamSpec{{
+								Name: "string-param",
+							}},
+						},
 					},
 				},
-			}},
-		},
-		want: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo", TaskSpec: &v1.EmbeddedTask{
-					TaskSpec: v1.TaskSpec{
-						Params: []v1.ParamSpec{{
-							Name: "string-param",
-							Type: v1.ParamTypeString,
-						}},
+			},
+			Finally: []v1.PipelineTask{
+				{
+					Name:    "final-task-with-taskRef",
+					TaskRef: &v1.TaskRef{Name: "foo-task-1"},
+				},
+				{
+					Name: "final-task-with-taskSpec",
+					TaskSpec: &v1.EmbeddedTask{
+						TaskSpec: v1.TaskSpec{
+							Params: []v1.ParamSpec{{
+								Name: "string-param",
+							}},
+						},
 					},
 				},
-			}},
-		},
-	}, {
-		desc: "pipeline task with taskRef - with default resolver",
-		ps: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name:    "foo",
-				TaskRef: &v1.TaskRef{},
-			}},
+			},
 		},
 		want: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo",
-				TaskRef: &v1.TaskRef{
-					Kind: v1.NamespacedTaskKind,
-					ResolverRef: v1.ResolverRef{
-						Resolver: "git",
+			Tasks: []v1.PipelineTask{
+				{
+					Name:    "task-with-taskRef",
+					TaskRef: &v1.TaskRef{Name: "bar-task-1", Kind: v1.NamespacedTaskKind},
+				},
+				{
+					Name: "task-with-taskSpec",
+					TaskSpec: &v1.EmbeddedTask{
+						TaskSpec: v1.TaskSpec{
+							Params: []v1.ParamSpec{{
+								Name: "string-param",
+								Type: v1.ParamTypeString,
+							}},
+						},
 					},
 				},
-			}},
+			},
+			Finally: []v1.PipelineTask{
+				{
+					Name:    "final-task-with-taskRef",
+					TaskRef: &v1.TaskRef{Name: "foo-task-1", Kind: v1.NamespacedTaskKind},
+				},
+				{
+					Name: "final-task-with-taskSpec",
+					TaskSpec: &v1.EmbeddedTask{
+						TaskSpec: v1.TaskSpec{
+							Params: []v1.ParamSpec{{
+								Name: "string-param",
+								Type: v1.ParamTypeString,
+							}},
+						},
+					},
+				},
+			},
 		},
-		defaults: map[string]string{
-			"default-resolver-type": "git",
+	}}
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx := context.Background()
+			tc.ps.SetDefaults(ctx)
+			if d := cmp.Diff(tc.want, tc.ps); d != "" {
+				t.Errorf("Mismatch of pipelineSpec after setting defaults: %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
+func TestPipelineTask_SetDefaults(t *testing.T) {
+	cases := []struct {
+		desc     string
+		pt       *v1.PipelineTask
+		want     *v1.PipelineTask
+		defaults map[string]string
+	}{{
+		desc: "pipeline task - default task kind must be " + string(v1.NamespacedTaskKind),
+		pt: &v1.PipelineTask{
+			Name:    "foo",
+			TaskRef: &v1.TaskRef{Name: "foo-task"},
+		},
+		want: &v1.PipelineTask{
+			Name: "foo",
+			TaskRef: &v1.TaskRef{
+				Name: "foo-task",
+				Kind: v1.NamespacedTaskKind,
+			},
 		},
 	}, {
-		desc: "pipeline task with taskRef - user-provided resolver overwrites default resolver",
-		ps: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo",
-				TaskRef: &v1.TaskRef{
-					ResolverRef: v1.ResolverRef{
-						Resolver: "custom resolver",
-					},
+		desc: "pipeline task with taskSpec - default param type must be " + string(v1.ParamTypeString),
+		pt: &v1.PipelineTask{
+			Name: "foo",
+			TaskSpec: &v1.EmbeddedTask{
+				TaskSpec: v1.TaskSpec{
+					Params: []v1.ParamSpec{{
+						Name: "string-param",
+					}},
 				},
-			}},
+			},
+		},
+		want: &v1.PipelineTask{
+			Name: "foo",
+			TaskSpec: &v1.EmbeddedTask{
+				TaskSpec: v1.TaskSpec{
+					Params: []v1.ParamSpec{{
+						Name: "string-param",
+						Type: v1.ParamTypeString,
+					}},
+				},
+			},
 		},
-		want: &v1.PipelineSpec{
-			Tasks: []v1.PipelineTask{{
-				Name: "foo",
-				TaskRef: &v1.TaskRef{
-					Kind: v1.NamespacedTaskKind,
-					ResolverRef: v1.ResolverRef{
-						Resolver: "custom resolver",
-					},
+	}, {
+		desc: "pipeline task with taskRef - with default resolver",
+		pt: &v1.PipelineTask{
+			Name:    "foo",
+			TaskRef: &v1.TaskRef{},
+		},
+		want: &v1.PipelineTask{
+			Name: "foo",
+			TaskRef: &v1.TaskRef{
+				Kind: v1.NamespacedTaskKind,
+				ResolverRef: v1.ResolverRef{
+					Resolver: "git",
 				},
-			}},
+			},
 		},
 		defaults: map[string]string{
 			"default-resolver-type": "git",
 		},
 	}, {
-		desc: "final pipeline task with taskSpec - default param type must be " + string(v1.ParamTypeString),
-		ps: &v1.PipelineSpec{
-			Finally: []v1.PipelineTask{{
-				Name: "foo", TaskSpec: &v1.EmbeddedTask{
-					TaskSpec: v1.TaskSpec{
-						Params: []v1.ParamSpec{{
-							Name: "string-param",
-						}},
-					},
+		desc: "pipeline task with taskRef - user-provided resolver overwrites default resolver",
+		pt: &v1.PipelineTask{
+			Name: "foo",
+			TaskRef: &v1.TaskRef{
+				ResolverRef: v1.ResolverRef{
+					Resolver: "custom resolver",
 				},
-			}},
-		},
-		want: &v1.PipelineSpec{
-			Finally: []v1.PipelineTask{{
-				Name: "foo", TaskSpec: &v1.EmbeddedTask{
-					TaskSpec: v1.TaskSpec{
-						Params: []v1.ParamSpec{{
-							Name: "string-param",
-							Type: v1.ParamTypeString,
-						}},
-					},
+			},
+		},
+		want: &v1.PipelineTask{
+			Name: "foo",
+			TaskRef: &v1.TaskRef{
+				Kind: v1.NamespacedTaskKind,
+				ResolverRef: v1.ResolverRef{
+					Resolver: "custom resolver",
 				},
-			}},
+			},
+		},
+		defaults: map[string]string{
+			"default-resolver-type": "git",
 		},
 	}}
 	for _, tc := range cases {
@@ -202,8 +245,8 @@ func TestPipelineSpec_SetDefaults(t *testing.T) {
 			if len(tc.defaults) > 0 {
 				ctx = dfttesting.SetDefaults(context.Background(), t, tc.defaults)
 			}
-			tc.ps.SetDefaults(ctx)
-			if d := cmp.Diff(tc.want, tc.ps); d != "" {
+			tc.pt.SetDefaults(ctx)
+			if d := cmp.Diff(tc.want, tc.pt); d != "" {
 				t.Errorf("Mismatch of pipelineSpec after setting defaults: %s", diff.PrintWantGot(d))
 			}
 		})

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go
@@ -32,34 +32,32 @@ func (p *Pipeline) SetDefaults(ctx context.Context) {
 
 // SetDefaults sets default values for the PipelineSpec's Params, Tasks, and Finally
 func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
-	cfg := config.FromContextOrDefaults(ctx)
 	for i := range ps.Params {
 		ps.Params[i].SetDefaults(ctx)
 	}
 
 	for _, pt := range ps.Tasks {
-		if pt.TaskRef != nil {
-			if pt.TaskRef.Kind == "" {
-				pt.TaskRef.Kind = NamespacedTaskKind
-			}
-			if pt.TaskRef.Name == "" && pt.TaskRef.Resolver == "" {
-				pt.TaskRef.Resolver = ResolverName(cfg.Defaults.DefaultResolverType)
-			}
-		}
-		if pt.TaskSpec != nil {
-			pt.TaskSpec.SetDefaults(ctx)
-		}
+		pt.SetDefaults(ctx)
 	}
 
 	for _, ft := range ps.Finally {
 		ctx := ctx // Ensure local scoping per Task
-		if ft.TaskRef != nil {
-			if ft.TaskRef.Kind == "" {
-				ft.TaskRef.Kind = NamespacedTaskKind
-			}
+		ft.SetDefaults(ctx)
+	}
+}
+
+// SetDefaults sets default values for a PipelineTask
+func (pt *PipelineTask) SetDefaults(ctx context.Context) {
+	cfg := config.FromContextOrDefaults(ctx)
+	if pt.TaskRef != nil {
+		if pt.TaskRef.Kind == "" {
+			pt.TaskRef.Kind = NamespacedTaskKind
 		}
-		if ft.TaskSpec != nil {
-			ft.TaskSpec.SetDefaults(ctx)
+		if pt.TaskRef.Name == "" && pt.TaskRef.Resolver == "" {
+			pt.TaskRef.Resolver = ResolverName(cfg.Defaults.DefaultResolverType)
 		}
 	}
+	if pt.TaskSpec != nil {
+		pt.TaskSpec.SetDefaults(ctx)
+	}
 }