Skip to content

Commit

Permalink
Remove upgrade via defaulting
Browse files Browse the repository at this point in the history
Upgrade is currently performed by the conversion webhook. Therefore,
there is no need to do it when defaulting.

Upgrade during defaulting was inspired by Knative. They also removed
it from their codebase.
  • Loading branch information
guillaumerose authored and tekton-robot committed Oct 5, 2021
1 parent 36fd46a commit 00b55df
Show file tree
Hide file tree
Showing 13 changed files with 8 additions and 200 deletions.
5 changes: 2 additions & 3 deletions cmd/webhook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/contexts"
"k8s.io/apimachinery/pkg/runtime/schema"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
Expand Down Expand Up @@ -81,7 +80,7 @@ func newDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher

// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
func(ctx context.Context) context.Context {
return contexts.WithUpgradeViaDefaulting(store.ToContext(ctx))
return store.ToContext(ctx)
},

// Whether to disallow unknown fields.
Expand All @@ -106,7 +105,7 @@ func newValidationAdmissionController(ctx context.Context, cmw configmap.Watcher

// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
func(ctx context.Context) context.Context {
return contexts.WithUpgradeViaDefaulting(store.ToContext(ctx))
return store.ToContext(ctx)
},

// Whether to disallow unknown fields.
Expand Down
16 changes: 0 additions & 16 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/test/diff"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -116,21 +115,6 @@ func TestPipelineRunDefaulting(t *testing.T) {
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
}, {
name: "PipelineRef upgrade context",
in: &v1alpha1.PipelineRun{
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
},
},
want: &v1alpha1.PipelineRun{
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
ServiceAccountName: config.DefaultServiceAccountValue,
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
wc: contexts.WithUpgradeViaDefaulting,
}, {
name: "PipelineRef default config context",
in: &v1alpha1.PipelineRun{
Expand Down
11 changes: 0 additions & 11 deletions pkg/apis/pipeline/v1alpha1/task_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ package v1alpha1
import (
"context"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/contexts"
"knative.dev/pkg/apis"
)

Expand All @@ -32,15 +30,6 @@ func (t *Task) SetDefaults(ctx context.Context) {

// SetDefaults set any defaults for the task spec
func (ts *TaskSpec) SetDefaults(ctx context.Context) {
if contexts.IsUpgradeViaDefaulting(ctx) {
v := v1beta1.TaskSpec{}
if ts.ConvertTo(ctx, &v) == nil {
alpha := TaskSpec{}
if alpha.ConvertFrom(ctx, &v) == nil {
*ts = alpha
}
}
}
for i := range ts.Params {
ts.Params[i].SetDefaults(ctx)
}
Expand Down
12 changes: 0 additions & 12 deletions pkg/apis/pipeline/v1alpha1/taskrun_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ import (
"time"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/contexts"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
)
Expand All @@ -47,16 +45,6 @@ func (tr *TaskRun) SetDefaults(ctx context.Context) {
}

func (trs *TaskRunSpec) SetDefaults(ctx context.Context) {
if contexts.IsUpgradeViaDefaulting(ctx) {
v := v1beta1.TaskRunSpec{}
if trs.ConvertTo(ctx, &v) == nil {
alpha := TaskRunSpec{}
if alpha.ConvertFrom(ctx, &v) == nil {
*trs = alpha
}
}
}

cfg := config.FromContextOrDefaults(ctx)
if trs.TaskRef != nil && trs.TaskRef.Kind == "" {
trs.TaskRef.Kind = NamespacedTaskKind
Expand Down
19 changes: 0 additions & 19 deletions pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/test/diff"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -164,24 +163,6 @@ func TestTaskRunDefaulting(t *testing.T) {
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
}, {
name: "TaskRef upgrade context",
in: &v1alpha1.TaskRun{
Spec: v1alpha1.TaskRunSpec{
TaskRef: &v1alpha1.TaskRef{Name: "foo"},
},
},
want: &v1alpha1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"app.kubernetes.io/managed-by": "tekton-pipelines"},
},
Spec: v1alpha1.TaskRunSpec{
TaskRef: &v1alpha1.TaskRef{Name: "foo", Kind: v1alpha1.NamespacedTaskKind},
ServiceAccountName: config.DefaultServiceAccountValue,
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
wc: contexts.WithUpgradeViaDefaulting,
}, {
name: "TaskRef default config context",
in: &v1alpha1.TaskRun{
Expand Down
17 changes: 0 additions & 17 deletions pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/test/diff"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -215,21 +214,6 @@ func TestPipelineRunDefaulting(t *testing.T) {
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
}, {
name: "PipelineRef upgrade context",
in: &v1beta1.PipelineRun{
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{Name: "foo"},
},
},
want: &v1beta1.PipelineRun{
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{Name: "foo"},
ServiceAccountName: config.DefaultServiceAccountValue,
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
wc: contexts.WithUpgradeViaDefaulting,
}, {
name: "Embedded PipelineSpec default",
in: &v1beta1.PipelineRun{
Expand All @@ -253,7 +237,6 @@ func TestPipelineRunDefaulting(t *testing.T) {
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
wc: contexts.WithUpgradeViaDefaulting,
}, {
name: "PipelineRef default config context",
in: &v1beta1.PipelineRun{
Expand Down
19 changes: 0 additions & 19 deletions pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/test/diff"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -236,24 +235,6 @@ func TestTaskRunDefaulting(t *testing.T) {
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
}, {
name: "TaskRef upgrade context",
in: &v1beta1.TaskRun{
Spec: v1beta1.TaskRunSpec{
TaskRef: &v1beta1.TaskRef{Name: "foo"},
},
},
want: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"app.kubernetes.io/managed-by": "tekton-pipelines"},
},
Spec: v1beta1.TaskRunSpec{
TaskRef: &v1beta1.TaskRef{Name: "foo", Kind: v1beta1.NamespacedTaskKind},
ServiceAccountName: config.DefaultServiceAccountValue,
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
},
},
wc: contexts.WithUpgradeViaDefaulting,
}, {
name: "TaskRef default config context",
in: &v1beta1.TaskRun{
Expand Down
37 changes: 0 additions & 37 deletions pkg/contexts/contexts.go

This file was deleted.

50 changes: 0 additions & 50 deletions pkg/contexts/contexts_test.go

This file was deleted.

9 changes: 2 additions & 7 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
listersv1alpha1 "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1alpha1"
listers "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1beta1"
resourcelisters "github.com/tektoncd/pipeline/pkg/client/resource/listers/resource/v1alpha1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/pkg/pipelinerunmetrics"
tknreconciler "github.com/tektoncd/pipeline/pkg/reconciler"
"github.com/tektoncd/pipeline/pkg/reconciler/events"
Expand Down Expand Up @@ -188,9 +187,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun)
}

if pr.IsDone() {
// We may be reading a version of the object that was stored at an older version
// and may not have had all of the assumed default specified.
pr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
pr.SetDefaults(ctx)

if err := artifacts.CleanupArtifactStorage(ctx, pr, c.KubeClientSet); err != nil {
logger.Errorf("Failed to delete PVC for PipelineRun %s: %v", pr.Name, err)
Expand Down Expand Up @@ -338,9 +335,7 @@ func (c *Reconciler) resolvePipelineState(
func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, getPipelineFunc resources.GetPipeline) error {
logger := logging.FromContext(ctx)
cfg := config.FromContextOrDefaults(ctx)
// We may be reading a version of the object that was stored at an older version
// and may not have had all of the assumed default specified.
pr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
pr.SetDefaults(ctx)

// When pipeline run is pending, return to avoid creating the task
if pr.IsPending() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/pkg/list"
"github.com/tektoncd/pipeline/pkg/names"
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
Expand Down Expand Up @@ -510,7 +509,7 @@ func ResolvePipelineRunTask(
} else {
spec = task.TaskSpec.TaskSpec
}
spec.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
spec.SetDefaults(ctx)
rtr, err := resolvePipelineTaskResources(task, &spec, taskName, kind, providedResources)
if err != nil {
return nil, fmt.Errorf("couldn't match referenced resources with declared resources: %w", err)
Expand Down
3 changes: 1 addition & 2 deletions pkg/reconciler/taskrun/resources/taskspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/contexts"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand All @@ -47,7 +46,7 @@ func GetTaskData(ctx context.Context, taskRun *v1beta1.TaskRun, getTask GetTask)
}
taskMeta = t.TaskMetadata()
taskSpec = t.TaskSpec()
taskSpec.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
taskSpec.SetDefaults(ctx)
case taskRun.Spec.TaskSpec != nil:
taskMeta = taskRun.ObjectMeta
taskSpec = *taskRun.Spec.TaskSpec
Expand Down
7 changes: 2 additions & 5 deletions pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
taskrunreconciler "github.com/tektoncd/pipeline/pkg/client/injection/reconciler/pipeline/v1beta1/taskrun"
listers "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1beta1"
resourcelisters "github.com/tektoncd/pipeline/pkg/client/resource/listers/resource/v1alpha1"
"github.com/tektoncd/pipeline/pkg/contexts"
"github.com/tektoncd/pipeline/pkg/internal/affinityassistant"
"github.com/tektoncd/pipeline/pkg/internal/limitrange"
podconvert "github.com/tektoncd/pipeline/pkg/pod"
Expand Down Expand Up @@ -129,7 +128,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg

// We may be reading a version of the object that was stored at an older version
// and may not have had all of the assumed default specified.
tr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
tr.SetDefaults(ctx)

// Try to send cloud events first
cloudEventErr := cloudevent.SendCloudEvents(tr, c.cloudEventClient, logger)
Expand Down Expand Up @@ -287,9 +286,7 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1
// reconcile (see https://github.com/tektoncd/pipeline/issues/2473).
func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1.TaskSpec, *resources.ResolvedTaskResources, error) {
logger := logging.FromContext(ctx)
// We may be reading a version of the object that was stored at an older version
// and may not have had all of the assumed default specified.
tr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
tr.SetDefaults(ctx)

if c.disableResolution && tr.Status.TaskSpec == nil {
return nil, nil, errResourceNotResolved
Expand Down

0 comments on commit 00b55df

Please sign in to comment.