From 00b55df87e4815ac0647e53fb11f242e28ff2f45 Mon Sep 17 00:00:00 2001 From: Guillaume Rose Date: Tue, 5 Oct 2021 10:02:14 +0200 Subject: [PATCH] Remove upgrade via defaulting 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. --- cmd/webhook/main.go | 5 +- .../v1alpha1/pipelinerun_defaults_test.go | 16 ------ pkg/apis/pipeline/v1alpha1/task_defaults.go | 11 ---- .../pipeline/v1alpha1/taskrun_defaults.go | 12 ----- .../v1alpha1/taskrun_defaults_test.go | 19 ------- .../v1beta1/pipelinerun_defaults_test.go | 17 ------- .../pipeline/v1beta1/taskrun_defaults_test.go | 19 ------- pkg/contexts/contexts.go | 37 -------------- pkg/contexts/contexts_test.go | 50 ------------------- pkg/reconciler/pipelinerun/pipelinerun.go | 9 +--- .../resources/pipelinerunresolution.go | 3 +- pkg/reconciler/taskrun/resources/taskspec.go | 3 +- pkg/reconciler/taskrun/taskrun.go | 7 +-- 13 files changed, 8 insertions(+), 200 deletions(-) delete mode 100644 pkg/contexts/contexts.go delete mode 100644 pkg/contexts/contexts_test.go diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 6c5ee4883eb..816eecfe1eb 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -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" @@ -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. @@ -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. diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go index 3f90c0d5b27..7d22c4b6592 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go @@ -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" @@ -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{ diff --git a/pkg/apis/pipeline/v1alpha1/task_defaults.go b/pkg/apis/pipeline/v1alpha1/task_defaults.go index 7ff64e3f639..bcafc8b6e52 100644 --- a/pkg/apis/pipeline/v1alpha1/task_defaults.go +++ b/pkg/apis/pipeline/v1alpha1/task_defaults.go @@ -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" ) @@ -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) } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go b/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go index 3adc7e175a1..02915f64315 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_defaults.go @@ -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" ) @@ -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 diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go index d6f2b32a6af..86fcee74827 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_defaults_test.go @@ -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" @@ -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{ diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go index 2e40cfe34a1..85e241b7fe4 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go @@ -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" @@ -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{ @@ -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{ diff --git a/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go b/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go index ad779e99170..a00de5c5b80 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go @@ -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" @@ -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{ diff --git a/pkg/contexts/contexts.go b/pkg/contexts/contexts.go deleted file mode 100644 index b721edd3f48..00000000000 --- a/pkg/contexts/contexts.go +++ /dev/null @@ -1,37 +0,0 @@ -/* -Copyright 2019 The Tekton Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package contexts - -import "context" - -// lemonadeKey is used as the key for associating information -// with a context.Context. This variable doesn't really matter, so it's -// a total random name (for history purpose, used lemonade as it was written -// in an hot summer day). -type lemonadeKey struct{} - -// WithUpgradeViaDefaulting notes on the context that we want defaulting to rewrite -// from v1alpha1 pre-defaults to v1alpha1 post-defaults. -func WithUpgradeViaDefaulting(ctx context.Context) context.Context { - return context.WithValue(ctx, lemonadeKey{}, struct{}{}) -} - -// IsUpgradeViaDefaulting checks whether we should be "defaulting" from v1alpha1 pre-defaults to -// the v1alpha1 post-defaults subset. -func IsUpgradeViaDefaulting(ctx context.Context) bool { - return ctx.Value(lemonadeKey{}) != nil -} diff --git a/pkg/contexts/contexts_test.go b/pkg/contexts/contexts_test.go deleted file mode 100644 index 8ce1fe25baf..00000000000 --- a/pkg/contexts/contexts_test.go +++ /dev/null @@ -1,50 +0,0 @@ -/* -Copyright 2019 The Tekton Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package contexts - -import ( - "context" - "testing" -) - -func TestContexts(t *testing.T) { - ctx := context.Background() - tests := []struct { - name string - ctx context.Context - check func(context.Context) bool - want bool - }{{ - name: "are upgrading via defaulting", - ctx: WithUpgradeViaDefaulting(ctx), - check: IsUpgradeViaDefaulting, - want: true, - }, { - name: "aren't upgrading via defaulting", - ctx: ctx, - check: IsUpgradeViaDefaulting, - want: false, - }} - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := tc.check(tc.ctx) - if tc.want != got { - t.Errorf("check() = %v, wanted %v", tc.want, got) - } - }) - } -} diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index f74b8f50abc..26053756d0e 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -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" @@ -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) @@ -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() { diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 6c774290f90..a52af3f760c 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -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" @@ -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) diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index a0b5fe8791f..947661277cd 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -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" ) @@ -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 diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 14d231cb21e..49bfcd6344d 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -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" @@ -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) @@ -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