Refactoring "config-defaults" to use Defaultable (for timeout)

Each type of tektoncd/pipeline implements `Defaultable` (from
`knative/pkg`), which is meant to be used to set defaults value.

This changes uses this mechanisms to set the default timeout for
`TaskRun`s, instead of putting this mechanisms in the
`timeout_handler` code.

Signed-off-by: Vincent Demeester <[email protected]>

cmd/controller/main.go

@@ -23,7 +23,6 @@ import (
 
 	"github.com/knative/pkg/logging"
 
-	apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
 	tklogging "github.com/tektoncd/pipeline/pkg/logging"
 	corev1 "k8s.io/api/core/v1"
 	kubeinformers "k8s.io/client-go/informers"
@@ -118,10 +117,7 @@ func main() {
 	pipelineInformer := pipelineInformerFactory.Tekton().V1alpha1().Pipelines()
 	pipelineRunInformer := pipelineInformerFactory.Tekton().V1alpha1().PipelineRuns()
 
-	store := apisconfig.NewStore(logger.Named("config-store"))
-	store.WatchConfigs(configMapWatcher)
-
-	timeoutHandler := reconciler.NewTimeoutHandler(stopCh, logger, store)
+	timeoutHandler := reconciler.NewTimeoutHandler(stopCh, logger)
 
 	trc := taskrun.NewController(opt,
 		taskRunInformer,
@@ -131,7 +127,6 @@ func main() {
 		podInformer,
 		nil, //entrypoint cache will be initialized by controller if not provided
 		timeoutHandler,
-		store,
 	)
 	prc := pipelinerun.NewController(opt,
 		pipelineRunInformer,

cmd/webhook/main.go

@@ -17,21 +17,20 @@ limitations under the License.
 package main
 
 import (
+	"context"
 	"flag"
 	"log"
 
-	"github.com/knative/pkg/logging"
-	tklogging "github.com/tektoncd/pipeline/pkg/logging"
-
-	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
-
-	"go.uber.org/zap"
-
 	"github.com/knative/pkg/configmap"
+	"github.com/knative/pkg/logging"
 	"github.com/knative/pkg/logging/logkey"
 	"github.com/knative/pkg/signals"
 	"github.com/knative/pkg/webhook"
+	apiconfig "github.com/tektoncd/pipeline/pkg/apis/config"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
+	tklogging "github.com/tektoncd/pipeline/pkg/logging"
 	"github.com/tektoncd/pipeline/pkg/system"
+	"go.uber.org/zap"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
@@ -71,6 +70,10 @@ func main() {
 	// Watch the logging config map and dynamically update logging levels.
 	configMapWatcher := configmap.NewInformedWatcher(kubeClient, system.GetNamespace())
 	configMapWatcher.Watch(tklogging.ConfigName, logging.UpdateLevelFromConfigMap(logger, atomicLevel, WebhookLogKey))
+
+	store := apiconfig.NewStore(logger.Named("config-store"))
+	store.WatchConfigs(configMapWatcher)
+
 	if err = configMapWatcher.Start(stopCh); err != nil {
 		logger.Fatalf("failed to start configuration manager: %v", err)
 	}
@@ -95,6 +98,11 @@ func main() {
 			v1alpha1.SchemeGroupVersion.WithKind("PipelineRun"):      &v1alpha1.PipelineRun{},
 		},
 		Logger: logger,
+
+		// Decorate contexts with the current state of the config.
+		WithContext: func(ctx context.Context) context.Context {
+			return v1alpha1.WithDefaultConfigurationName(store.ToContext(ctx))
+		},
 	}
 
 	if err := controller.Run(stopCh); err != nil {

pkg/apis/config/default.go

@@ -30,20 +30,20 @@ const (
 	defaultTimeoutMinutesKey = "default-timeout-minutes"
 )
 
-// ConfigDefault holds the default configurations
+// Defaults holds the default configurations
 // +k8s:deepcopy-gen=true
-type ConfigDefault struct {
+type Defaults struct {
 	DefaultTimeoutMinutes int
 }
 
 // Equals returns true if two Configs are identical
-func (cfg *ConfigDefault) Equals(other *ConfigDefault) bool {
+func (cfg *Defaults) Equals(other *Defaults) bool {
 	return other.DefaultTimeoutMinutes == cfg.DefaultTimeoutMinutes
 }
 
-// NewConfigDefaultFromMap returns a Config given a map corresponding to a ConfigMap
-func NewConfigDefaultFromMap(cfgMap map[string]string) (*ConfigDefault, error) {
-	tc := ConfigDefault{
+// NewDefaultsFromMap returns a Config given a map corresponding to a ConfigMap
+func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) {
+	tc := Defaults{
 		DefaultTimeoutMinutes: DefaultTimeoutMinutes,
 	}
 	if defaultTimeoutMin, ok := cfgMap[defaultTimeoutMinutesKey]; ok {
@@ -57,7 +57,7 @@ func NewConfigDefaultFromMap(cfgMap map[string]string) (*ConfigDefault, error) {
 	return &tc, nil
 }
 
-// NewConfigDefaultFromConfigMap returns a Config for the given configmap
-func NewConfigDefaultFromConfigMap(config *corev1.ConfigMap) (*ConfigDefault, error) {
-	return NewConfigDefaultFromMap(config.Data)
+// NewDefaultsFromConfigMap returns a Config for the given configmap
+func NewDefaultsFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) {
+	return NewDefaultsFromMap(config.Data)
 }

pkg/apis/config/default_test.go

@@ -17,33 +17,34 @@ limitations under the License.
 package config
 
 import (
+	"testing"
+
 	"github.com/google/go-cmp/cmp"
 	test "github.com/tektoncd/pipeline/pkg/reconciler/testing"
-	"testing"
 )
 
-func TestNewConfigDefaultFromConfigMap(t *testing.T) {
-	expectedConfig := &ConfigDefault{
+func TestNewDefaultsFromConfigMap(t *testing.T) {
+	expectedConfig := &Defaults{
 		DefaultTimeoutMinutes: 50,
 	}
 	verifyConfigFileWithExpectedConfig(t, DefaultsConfigName, expectedConfig)
 }
 
-func TestNewConfigDefaultFromEmptyConfigMap(t *testing.T) {
+func TestNewDefaultsFromEmptyConfigMap(t *testing.T) {
 	DefaultsConfigEmptyName := "config-defaults-empty"
-	expectedConfig := &ConfigDefault{
+	expectedConfig := &Defaults{
 		DefaultTimeoutMinutes: 60,
 	}
 	verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyName, expectedConfig)
 }
 
-func verifyConfigFileWithExpectedConfig(t *testing.T, fileName string, expectedConfig *ConfigDefault) {
+func verifyConfigFileWithExpectedConfig(t *testing.T, fileName string, expectedConfig *Defaults) {
 	cm := test.ConfigMapFromTestFile(t, fileName)
-	if configDefault, err := NewConfigDefaultFromConfigMap(cm); err == nil {
-		if d := cmp.Diff(configDefault, expectedConfig); d != "" {
+	if Defaults, err := NewDefaultsFromConfigMap(cm); err == nil {
+		if d := cmp.Diff(Defaults, expectedConfig); d != "" {
 			t.Errorf("Diff:\n%s", d)
 		}
 	} else {
-		t.Errorf("NewConfigDefaultFromConfigMap(actual) = %v", err)
+		t.Errorf("NewDefaultsFromConfigMap(actual) = %v", err)
 	}
 }

pkg/apis/config/store.go

@@ -18,6 +18,7 @@ package config
 
 import (
 	"context"
+
 	"github.com/knative/pkg/configmap"
 )
 
@@ -26,7 +27,7 @@ type cfgKey struct{}
 // Config holds the collection of configurations that we attach to contexts.
 // +k8s:deepcopy-gen=false
 type Config struct {
-	ConfigDefault *ConfigDefault
+	Defaults *Defaults
 }
 
 // FromContext extracts a Config from the provided context.
@@ -44,9 +45,9 @@ func FromContextOrDefaults(ctx context.Context) *Config {
 	if cfg := FromContext(ctx); cfg != nil {
 		return cfg
 	}
-	defaults, _ := NewConfigDefaultFromMap(map[string]string{})
+	defaults, _ := NewDefaultsFromMap(map[string]string{})
 	return &Config{
-		ConfigDefault: defaults,
+		Defaults: defaults,
 	}
 }
 
@@ -69,7 +70,7 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i
 			"defaults",
 			logger,
 			configmap.Constructors{
-				DefaultsConfigName: NewConfigDefaultFromConfigMap,
+				DefaultsConfigName: NewDefaultsFromConfigMap,
 			},
 			onAfterStore...,
 		),
@@ -86,6 +87,6 @@ func (s *Store) ToContext(ctx context.Context) context.Context {
 // Load creates a Config from the current config state of the Store.
 func (s *Store) Load() *Config {
 	return &Config{
-		ConfigDefault: s.UntypedLoad(DefaultsConfigName).(*ConfigDefault).DeepCopy(),
+		Defaults: s.UntypedLoad(DefaultsConfigName).(*Defaults).DeepCopy(),
 	}
 }

pkg/apis/config/store_test.go

@@ -18,10 +18,11 @@ package config
 
 import (
 	"context"
+	"testing"
+
 	"github.com/google/go-cmp/cmp"
 	logtesting "github.com/knative/pkg/logging/testing"
 	test "github.com/tektoncd/pipeline/pkg/reconciler/testing"
-	"testing"
 )
 
 func TestStoreLoadWithContext(t *testing.T) {
@@ -31,8 +32,8 @@ func TestStoreLoadWithContext(t *testing.T) {
 
 	config := FromContext(store.ToContext(context.Background()))
 
-	expected, _ := NewConfigDefaultFromConfigMap(defaultConfig)
-	if diff := cmp.Diff(config.ConfigDefault, expected); diff != "" {
+	expected, _ := NewDefaultsFromConfigMap(defaultConfig)
+	if diff := cmp.Diff(config.Defaults, expected); diff != "" {
 		t.Errorf("Unexpected default config (-want, +got): %v", diff)
 	}
 }

pkg/apis/pipeline/v1alpha1/contexts.go

@@ -0,0 +1,30 @@
+/*
+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 v1alpha1
+
+import "context"
+
+// hdcnKey is used as the key for associating information
+// with a context.Context.
+type hdcnKey struct{}
+
+// WithDefaultConfigurationName notes on the context for nested validation
+// that there is a default configuration name, which affects how an empty
+// configurationName is validated.
+func WithDefaultConfigurationName(ctx context.Context) context.Context {
+	return context.WithValue(ctx, hdcnKey{}, struct{}{})
+}

pkg/apis/pipeline/v1alpha1/pipelinerun_defaults.go

@@ -0,0 +1,36 @@
+/*
+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 v1alpha1
+
+import (
+	"context"
+	"time"
+
+	"github.com/tektoncd/pipeline/pkg/apis/config"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func (pr *PipelineRun) SetDefaults(ctx context.Context) {
+	pr.Spec.SetDefaults(ctx)
+}
+
+func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) {
+	cfg := config.FromContextOrDefaults(ctx)
+	if prs.Timeout == nil {
+		prs.Timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute}
+	}
+}

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

@@ -17,7 +17,6 @@ limitations under the License.
 package v1alpha1
 
 import (
-	"context"
 	"fmt"
 	"time"
 
@@ -37,6 +36,10 @@ var (
 	}
 )
 
+// Check that TaskRun may be validated and defaulted.
+var _ apis.Validatable = (*PipelineRun)(nil)
+var _ apis.Defaultable = (*PipelineRun)(nil)
+
 // PipelineRunSpec defines the desired state of PipelineRun
 type PipelineRunSpec struct {
 	PipelineRef PipelineRef `json:"pipelineRef"`
@@ -211,9 +214,6 @@ func (pr *PipelineRun) GetTaskRunRef() corev1.ObjectReference {
 	}
 }
 
-// SetDefaults for pipelinerun
-func (pr *PipelineRun) SetDefaults(ctx context.Context) {}
-
 // GetOwnerReference gets the pipeline run as owner reference for any related objects
 func (pr *PipelineRun) GetOwnerReference() []metav1.OwnerReference {
 	return []metav1.OwnerReference{

pkg/apis/pipeline/v1alpha1/taskrun_defaults.go

@@ -16,7 +16,13 @@ limitations under the License.
 
 package v1alpha1
 
-import "context"
+import (
+	"context"
+	"time"
+
+	"github.com/tektoncd/pipeline/pkg/apis/config"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
 
 func (tr *TaskRun) SetDefaults(ctx context.Context) {
 	tr.Spec.SetDefaults(ctx)
@@ -26,4 +32,8 @@ func (trs *TaskRunSpec) SetDefaults(ctx context.Context) {
 	if trs.TaskRef != nil && trs.TaskRef.Kind == "" {
 		trs.TaskRef.Kind = NamespacedTaskKind
 	}
+	cfg := config.FromContextOrDefaults(ctx)
+	if trs.Timeout == nil {
+		trs.Timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute}
+	}
 }