set default build prune limits for group api

pkg/build/apis/build/types.go

@@ -80,11 +80,23 @@ const (
 	BuildCancelledEventReason = "BuildCancelled"
 	// BuildCancelledEventMessage is the message associated with the event registered when build is cancelled.
 	BuildCancelledEventMessage = "Build %s/%s has been cancelled"
+
+	// DefaultSuccessfulBuildsHistoryLimit is the default number of successful builds to retain
+	// if the buildconfig does not specify a value.  This only applies to buildconfigs created
+	// via the new group api resource, not the legacy resource.
+	DefaultSuccessfulBuildsHistoryLimit = int32(5)
+
+	// DefaultFailedBuildsHistoryLimit is the default number of failed builds to retain
+	// if the buildconfig does not specify a value.  This only applies to buildconfigs created
+	// via the new group api resource, not the legacy resource.
+	DefaultFailedBuildsHistoryLimit = int32(5)
 )
 
-// WhitelistEnvVarNames is a list of environment variable keys that are allowed to be set by the
-// user on the build pod.
-var WhitelistEnvVarNames = [2]string{"BUILD_LOGLEVEL", "GIT_SSL_NO_VERIFY"}
+var (
+	// WhitelistEnvVarNames is a list of environment variable keys that are allowed to be set by the
+	// user on the build pod.
+	WhitelistEnvVarNames = [2]string{"BUILD_LOGLEVEL", "GIT_SSL_NO_VERIFY"}
+)
 
 // +genclient=true
 

pkg/build/registry/buildconfig/etcd/etcd.go

@@ -24,9 +24,9 @@ func NewREST(optsGetter restoptions.Getter) (*REST, error) {
 		QualifiedResource: buildapi.Resource("buildconfigs"),
 		PredicateFunc:     buildconfig.Matcher,
 
-		CreateStrategy: buildconfig.Strategy,
-		UpdateStrategy: buildconfig.Strategy,
-		DeleteStrategy: buildconfig.Strategy,
+		CreateStrategy: buildconfig.GroupStrategy,
+		UpdateStrategy: buildconfig.GroupStrategy,
+		DeleteStrategy: buildconfig.GroupStrategy,
 	}
 
 	options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: buildconfig.GetAttrs}

pkg/build/registry/buildconfig/strategy.go

@@ -9,6 +9,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/apiserver/pkg/registry/rest"
 	kstorage "k8s.io/apiserver/pkg/storage"
 	"k8s.io/apiserver/pkg/storage/names"
 	kapi "k8s.io/kubernetes/pkg/api"
@@ -17,15 +18,28 @@ import (
 	"github.com/openshift/origin/pkg/build/apis/build/validation"
 )
 
-// strategy implements behavior for BuildConfig objects
+var (
+	// GroupStrategy is the logic that applies when creating and updating BuildConfig objects
+	// in the Group api.
+	// This differs from the LegacyStrategy in that on create it will set a default build
+	// pruning limit value for both successful and failed builds.  This is new behavior that
+	// can only be introduced to users consuming the new group based api.
+	GroupStrategy = groupStrategy{strategy{kapi.Scheme, names.SimpleNameGenerator}}
+
+	// LegacyStrategy is the default logic that applies when creating BuildConfig objects.
+	// Specifically it will not set the default build pruning limit values because that was not
+	// part of the legacy api.
+	LegacyStrategy = legacyStrategy{strategy{kapi.Scheme, names.SimpleNameGenerator}}
+)
+
+// strategy implements most of the behavior for BuildConfig objects
+// It does not provide a PrepareForCreate implementation, that is expected
+// to be implemented by the child implementation.
 type strategy struct {
 	runtime.ObjectTyper
 	names.NameGenerator
 }
 
-// Strategy is the default logic that applies when creating and updating BuildConfig objects.
-var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
-
 func (strategy) NamespaceScoped() bool {
 	return true
 }
@@ -39,16 +53,17 @@ func (strategy) AllowUnconditionalUpdate() bool {
 	return false
 }
 
+// Canonicalize normalizes the object after validation.
+func (strategy) Canonicalize(obj runtime.Object) {
+}
+
 // PrepareForCreate clears fields that are not allowed to be set by end users on creation.
-func (strategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
+// This is invoked by the Group and Legacy strategies.
+func (s strategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
 	bc := obj.(*buildapi.BuildConfig)
 	dropUnknownTriggers(bc)
 }
 
-// Canonicalize normalizes the object after validation.
-func (strategy) Canonicalize(obj runtime.Object) {
-}
-
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update.
 func (strategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) {
 	newBC := obj.(*buildapi.BuildConfig)
@@ -71,6 +86,45 @@ func (strategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object)
 	return validation.ValidateBuildConfigUpdate(obj.(*buildapi.BuildConfig), old.(*buildapi.BuildConfig))
 }
 
+// groupStrategy implements behavior for BuildConfig objects in the Group api
+type groupStrategy struct {
+	strategy
+}
+
+// PrepareForCreate delegates to the common strategy and sets default pruning limits
+func (s groupStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
+	s.strategy.PrepareForCreate(ctx, obj)
+
+	bc := obj.(*buildapi.BuildConfig)
+	if bc.Spec.SuccessfulBuildsHistoryLimit == nil {
+		v := buildapi.DefaultSuccessfulBuildsHistoryLimit
+		bc.Spec.SuccessfulBuildsHistoryLimit = &v
+	}
+	if bc.Spec.FailedBuildsHistoryLimit == nil {
+		v := buildapi.DefaultFailedBuildsHistoryLimit
+		bc.Spec.FailedBuildsHistoryLimit = &v
+	}
+}
+
+// legacyStrategy implements behavior for BuildConfig objects in the legacy api
+type legacyStrategy struct {
+	strategy
+}
+
+// PrepareForCreate delegates to the common strategy.
+func (s legacyStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
+	s.strategy.PrepareForCreate(ctx, obj)
+
+	// legacy buildconfig api does not apply default pruning values, to maintain
+	// backwards compatibility.
+
+}
+
+// DefaultGarbageCollectionPolicy for legacy buildconfigs will orphan dependents.
+func (s legacyStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
+	return rest.OrphanDependents
+}
+
 // GetAttrs returns labels and fields of a given object for filtering purposes
 func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) {
 	buildConfig, ok := obj.(*buildapi.BuildConfig)

pkg/build/registry/buildconfig/strategy_test.go

@@ -10,12 +10,12 @@ import (
 	buildapi "github.com/openshift/origin/pkg/build/apis/build"
 )
 
-func TestBuildConfigStrategy(t *testing.T) {
+func TestBuildConfigGroupStrategy(t *testing.T) {
 	ctx := apirequest.NewDefaultContext()
-	if !Strategy.NamespaceScoped() {
+	if !GroupStrategy.NamespaceScoped() {
 		t.Errorf("BuildConfig is namespace scoped")
 	}
-	if Strategy.AllowCreateOnUpdate() {
+	if GroupStrategy.AllowCreateOnUpdate() {
 		t.Errorf("BuildConfig should not allow create on update")
 	}
 	buildConfig := &buildapi.BuildConfig{
@@ -88,34 +88,163 @@ func TestBuildConfigStrategy(t *testing.T) {
 			LastVersion: 9,
 		},
 	}
-	Strategy.PrepareForCreate(ctx, buildConfig)
-	errs := Strategy.Validate(ctx, buildConfig)
+	GroupStrategy.PrepareForCreate(ctx, buildConfig)
+	errs := GroupStrategy.Validate(ctx, buildConfig)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected error validating %v", errs)
 	}
+	if buildConfig.Spec.SuccessfulBuildsHistoryLimit == nil {
+		t.Errorf("Expected successful limit %d, got nil", buildapi.DefaultSuccessfulBuildsHistoryLimit)
+	}
+	if *buildConfig.Spec.SuccessfulBuildsHistoryLimit != buildapi.DefaultSuccessfulBuildsHistoryLimit {
+		t.Errorf("Expected successful limit %d, got %d", buildapi.DefaultSuccessfulBuildsHistoryLimit, *buildConfig.Spec.SuccessfulBuildsHistoryLimit)
+	}
+	if buildConfig.Spec.FailedBuildsHistoryLimit == nil {
+		t.Errorf("Expected failed limit %d, got nil", buildapi.DefaultFailedBuildsHistoryLimit)
+	}
+	if *buildConfig.Spec.FailedBuildsHistoryLimit != buildapi.DefaultFailedBuildsHistoryLimit {
+		t.Errorf("Expected failed limit %d, got %d", buildapi.DefaultFailedBuildsHistoryLimit, *buildConfig.Spec.FailedBuildsHistoryLimit)
+	}
+
+	// lastversion cannot go backwards
+	newBC.Status.LastVersion = 9
+	GroupStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
+	if newBC.Status.LastVersion != buildConfig.Status.LastVersion {
+		t.Errorf("Expected version=%d, got %d", buildConfig.Status.LastVersion, newBC.Status.LastVersion)
+	}
+
+	// lastversion can go forwards
+	newBC.Status.LastVersion = 11
+	GroupStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
+	if newBC.Status.LastVersion != 11 {
+		t.Errorf("Expected version=%d, got %d", 11, newBC.Status.LastVersion)
+	}
+
+	GroupStrategy.PrepareForCreate(ctx, buildConfig)
+	errs = GroupStrategy.Validate(ctx, buildConfig)
+	if len(errs) != 0 {
+		t.Errorf("Unexpected error validating %v", errs)
+	}
+
+	invalidBuildConfig := &buildapi.BuildConfig{}
+	errs = GroupStrategy.Validate(ctx, invalidBuildConfig)
+	if len(errs) == 0 {
+		t.Errorf("Expected error validating")
+	}
+}
+
+func TestBuildConfigLegacyStrategy(t *testing.T) {
+	ctx := apirequest.NewDefaultContext()
+	if !LegacyStrategy.NamespaceScoped() {
+		t.Errorf("BuildConfig is namespace scoped")
+	}
+	if LegacyStrategy.AllowCreateOnUpdate() {
+		t.Errorf("BuildConfig should not allow create on update")
+	}
+	buildConfig := &buildapi.BuildConfig{
+		ObjectMeta: metav1.ObjectMeta{Name: "config-id", Namespace: "namespace"},
+		Spec: buildapi.BuildConfigSpec{
+			RunPolicy: buildapi.BuildRunPolicySerial,
+			Triggers: []buildapi.BuildTriggerPolicy{
+				{
+					GitHubWebHook: &buildapi.WebHookTrigger{Secret: "12345"},
+					Type:          buildapi.GitHubWebHookBuildTriggerType,
+				},
+				{
+					Type: "unknown",
+				},
+			},
+			CommonSpec: buildapi.CommonSpec{
+				Source: buildapi.BuildSource{
+					Git: &buildapi.GitBuildSource{
+						URI: "http://github.com/my/repository",
+					},
+					ContextDir: "context",
+				},
+				Strategy: buildapi.BuildStrategy{
+					DockerStrategy: &buildapi.DockerBuildStrategy{},
+				},
+				Output: buildapi.BuildOutput{
+					To: &kapi.ObjectReference{
+						Kind: "DockerImage",
+						Name: "repository/data",
+					},
+				},
+			},
+		},
+		Status: buildapi.BuildConfigStatus{
+			LastVersion: 10,
+		},
+	}
+	newBC := &buildapi.BuildConfig{
+		ObjectMeta: metav1.ObjectMeta{Name: "config-id", Namespace: "namespace"},
+		Spec: buildapi.BuildConfigSpec{
+			RunPolicy: buildapi.BuildRunPolicySerial,
+			Triggers: []buildapi.BuildTriggerPolicy{
+				{
+					GitHubWebHook: &buildapi.WebHookTrigger{Secret: "12345"},
+					Type:          buildapi.GitHubWebHookBuildTriggerType,
+				},
+				{
+					Type: "unknown",
+				},
+			},
+			CommonSpec: buildapi.CommonSpec{
+				Source: buildapi.BuildSource{
+					Git: &buildapi.GitBuildSource{
+						URI: "http://github.com/my/repository",
+					},
+					ContextDir: "context",
+				},
+				Strategy: buildapi.BuildStrategy{
+					DockerStrategy: &buildapi.DockerBuildStrategy{},
+				},
+				Output: buildapi.BuildOutput{
+					To: &kapi.ObjectReference{
+						Kind: "DockerImage",
+						Name: "repository/data",
+					},
+				},
+			},
+		},
+		Status: buildapi.BuildConfigStatus{
+			LastVersion: 9,
+		},
+	}
+	LegacyStrategy.PrepareForCreate(ctx, buildConfig)
+	errs := LegacyStrategy.Validate(ctx, buildConfig)
+	if len(errs) != 0 {
+		t.Errorf("Unexpected error validating %v", errs)
+	}
+	if buildConfig.Spec.SuccessfulBuildsHistoryLimit != nil {
+		t.Errorf("Expected successful limit to be nil, got %d", *buildConfig.Spec.SuccessfulBuildsHistoryLimit)
+	}
+	if buildConfig.Spec.FailedBuildsHistoryLimit != nil {
+		t.Errorf("Expected failed limit to be nil, got %d", *buildConfig.Spec.FailedBuildsHistoryLimit)
+	}
 
 	// lastversion cannot go backwards
 	newBC.Status.LastVersion = 9
-	Strategy.PrepareForUpdate(ctx, newBC, buildConfig)
+	LegacyStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
 	if newBC.Status.LastVersion != buildConfig.Status.LastVersion {
 		t.Errorf("Expected version=%d, got %d", buildConfig.Status.LastVersion, newBC.Status.LastVersion)
 	}
 
 	// lastversion can go forwards
 	newBC.Status.LastVersion = 11
-	Strategy.PrepareForUpdate(ctx, newBC, buildConfig)
+	LegacyStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
 	if newBC.Status.LastVersion != 11 {
 		t.Errorf("Expected version=%d, got %d", 11, newBC.Status.LastVersion)
 	}
 
-	Strategy.PrepareForCreate(ctx, buildConfig)
-	errs = Strategy.Validate(ctx, buildConfig)
+	LegacyStrategy.PrepareForCreate(ctx, buildConfig)
+	errs = LegacyStrategy.Validate(ctx, buildConfig)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected error validating %v", errs)
 	}
 
 	invalidBuildConfig := &buildapi.BuildConfig{}
-	errs = Strategy.Validate(ctx, invalidBuildConfig)
+	errs = LegacyStrategy.Validate(ctx, invalidBuildConfig)
 	if len(errs) == 0 {
 		t.Errorf("Expected error validating")
 	}

pkg/cmd/cli/cmd/exporter.go

@@ -148,7 +148,10 @@ func (e *DefaultExporter) Export(obj runtime.Object, exact bool) error {
 		return deployrest.Strategy.Export(ctx, obj, exact)
 
 	case *buildapi.BuildConfig:
-		buildconfigrest.Strategy.PrepareForCreate(ctx, obj)
+		// Use the legacy strategy to avoid setting prune defaults if
+		// the object wasn't created with them in the first place.
+		// TODO: use the exportstrategy pattern instead.
+		buildconfigrest.LegacyStrategy.PrepareForCreate(ctx, obj)
 		// TODO: should be handled by prepare for create
 		t.Status.LastVersion = 0
 		for i := range t.Spec.Triggers {

pkg/cmd/server/origin/legacy.go

@@ -5,6 +5,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/registry/rest"
 
+	buildconfig "github.com/openshift/origin/pkg/build/registry/buildconfig"
 	buildconfigetcd "github.com/openshift/origin/pkg/build/registry/buildconfig/etcd"
 	deploymentconfigetcd "github.com/openshift/origin/pkg/deploy/registry/deployconfig/etcd"
 )
@@ -197,7 +198,8 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
 				case "buildConfigs":
 					restStorage := s.(*buildconfigetcd.REST)
 					store := *restStorage.Store
-					store.DeleteStrategy = orphanByDefault(store.DeleteStrategy)
+					store.DeleteStrategy = buildconfig.LegacyStrategy
+					store.CreateStrategy = buildconfig.LegacyStrategy
 					legacyStorage[resource] = &buildconfigetcd.REST{Store: &store}
 				case "deploymentConfigs":
 					restStorage := s.(*deploymentconfigetcd.REST)