Set default for DeploymentConfigSpec.RevisionHistoryLimit in apps/v1 to 10

Author: tnozicka

api/protobuf-spec/github_com_openshift_origin_pkg_deploy_apis_apps_v1.proto

@@ -26392,7 +26392,7 @@
      "revisionHistoryLimit": {
       "type": "integer",
       "format": "int32",
-      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified."
+      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group api resource, not the legacy resource.)"
      },
      "test": {
       "type": "boolean",

api/swagger-spec/oapi-v1.json

@@ -96022,7 +96022,7 @@
       "format": "int32"
      },
      "revisionHistoryLimit": {
-      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.",
+      "description": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)",
       "type": "integer",
       "format": "int32"
      },

api/swagger-spec/openshift-openapi-spec.json

@@ -7,6 +7,7 @@ import (
 
 	buildconfig "github.com/openshift/origin/pkg/build/registry/buildconfig"
 	buildconfigetcd "github.com/openshift/origin/pkg/build/registry/buildconfig/etcd"
+	"github.com/openshift/origin/pkg/deploy/registry/deployconfig"
 	deploymentconfigetcd "github.com/openshift/origin/pkg/deploy/registry/deployconfig/etcd"
 	routeregistry "github.com/openshift/origin/pkg/route/registry/route"
 	routeetcd "github.com/openshift/origin/pkg/route/registry/route/etcd"
@@ -206,7 +207,8 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
 				case "deploymentConfigs":
 					restStorage := s.(*deploymentconfigetcd.REST)
 					store := *restStorage.Store
-					restStorage.DeleteStrategy = orphanByDefault(store.DeleteStrategy)
+					store.CreateStrategy = deployconfig.LegacyStrategy
+					store.DeleteStrategy = deployconfig.LegacyStrategy
 					legacyStorage[resource] = &deploymentconfigetcd.REST{Store: &store}
 				case "routes":
 					restStorage := s.(*routeetcd.REST)
@@ -223,15 +225,3 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
 	}
 	return legacyStorage
 }
-
-type orphaningDeleter struct {
-	rest.RESTDeleteStrategy
-}
-
-func (o orphaningDeleter) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
-	return rest.OrphanDependents
-}
-
-func orphanByDefault(in rest.RESTDeleteStrategy) rest.RESTDeleteStrategy {
-	return orphaningDeleter{in}
-}

pkg/cmd/server/origin/legacy.go

@@ -20,6 +20,9 @@ const (
 	// This is set as the default value for ActiveDeadlineSeconds for the deployer pod.
 	// Currently set to 6 hours.
 	MaxDeploymentDurationSeconds int64 = 21600
+	// DefaultRevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks.
+	// This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.
+	DefaultRevisionHistoryLimit int32 = 10
 )
 
 // These constants represent keys used for correlating objects related to deployments.
@@ -156,6 +159,7 @@ type DeploymentConfigSpec struct {
 
 	// RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks.
 	// This field is a pointer to allow for differentiation between an explicit zero and not specified.
+	// Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)
 	RevisionHistoryLimit *int32
 
 	// Test ensures that this deployment config will have zero replicas except while a deployment is running. This allows the

pkg/deploy/apis/apps/types.go

@@ -101,7 +101,7 @@ var map_DeploymentConfigSpec = map[string]string{
 	"minReadySeconds":      "MinReadySeconds is the minimum number of seconds for which a newly created pod should be ready without any of its container crashing, for it to be considered available. Defaults to 0 (pod will be considered available as soon as it is ready)",
 	"triggers":             "Triggers determine how updates to a DeploymentConfig result in new deployments. If no triggers are defined, a new deployment can only occur as a result of an explicit client update to the DeploymentConfig with a new LatestVersion. If null, defaults to having a config change trigger.",
 	"replicas":             "Replicas is the number of desired replicas.",
-	"revisionHistoryLimit": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.",
+	"revisionHistoryLimit": "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)",
 	"test":                 "Test ensures that this deployment config will have zero replicas except while a deployment is running. This allows the deployment config to be used as a continuous deployment test - triggering on images, running the deployment, and then succeeding or failing. Post strategy hooks and After actions can be used to integrate successful deployment with an action.",
 	"paused":               "Paused indicates that the deployment config is paused resulting in no new deployments on template changes or changes in the template caused by other triggers.",
 	"selector":             "Selector is a label query over pods that should match the Replicas count.",

pkg/deploy/apis/apps/v1/generated.proto

@@ -51,6 +51,7 @@ type DeploymentConfigSpec struct {
 
 	// RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks.
 	// This field is a pointer to allow for differentiation between an explicit zero and not specified.
+	// Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)
 	RevisionHistoryLimit *int32 `json:"revisionHistoryLimit,omitempty" protobuf:"varint,4,opt,name=revisionHistoryLimit"`
 
 	// Test ensures that this deployment config will have zero replicas except while a deployment is running. This allows the

pkg/deploy/apis/apps/v1/swagger_doc.go

@@ -37,9 +37,9 @@ func NewREST(optsGetter restoptions.Getter) (*REST, *StatusREST, *ScaleREST, err
 		PredicateFunc:     deployconfig.Matcher,
 		QualifiedResource: deployapi.Resource("deploymentconfigs"),
 
-		CreateStrategy: deployconfig.Strategy,
-		UpdateStrategy: deployconfig.Strategy,
-		DeleteStrategy: deployconfig.Strategy,
+		CreateStrategy: deployconfig.GroupStrategy,
+		UpdateStrategy: deployconfig.GroupStrategy,
+		DeleteStrategy: deployconfig.GroupStrategy,
 	}
 
 	options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: deployconfig.GetAttrs}

pkg/deploy/apis/apps/v1/types.go

@@ -10,6 +10,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"
@@ -24,8 +25,16 @@ type strategy struct {
 	names.NameGenerator
 }
 
-// Strategy is the default logic that applies when creating and updating DeploymentConfig objects.
-var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
+// CommonStrategy is the default logic that applies when creating and updating DeploymentConfig objects.
+var CommonStrategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
+
+// LegacyStrategy is the logic that applies when creating and updating DeploymentConfig objects in the legacy API.
+// An example would be setting different defaults depending on API group
+var LegacyStrategy = legacyStrategy{CommonStrategy}
+
+// GroupStrategy is the logic that applies when creating and updating DeploymentConfig objects in the group API.
+// An example would be setting different defaults depending on API group
+var GroupStrategy = groupStrategy{CommonStrategy}
 
 // NamespaceScoped is true for DeploymentConfig objects.
 func (strategy) NamespaceScoped() bool {
@@ -105,12 +114,44 @@ func (strategy) CheckGracefulDelete(obj runtime.Object, options *metav1.DeleteOp
 	return false
 }
 
+// legacyStrategy implements behavior for DeploymentConfig 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)
+}
+
+// DefaultGarbageCollectionPolicy for legacy DeploymentConfigs will orphan dependents.
+func (s legacyStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
+	return rest.OrphanDependents
+}
+
+// groupStrategy implements behavior for DeploymentConfig objects in the Group API
+type groupStrategy struct {
+	strategy
+}
+
+// PrepareForCreate delegates to the common strategy and sets defaults applicable only to Group API
+func (s groupStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
+	s.strategy.PrepareForCreate(ctx, obj)
+
+	dc := obj.(*deployapi.DeploymentConfig)
+
+	if dc.Spec.RevisionHistoryLimit == nil {
+		v := deployapi.DefaultRevisionHistoryLimit
+		dc.Spec.RevisionHistoryLimit = &v
+	}
+}
+
 // statusStrategy implements behavior for DeploymentConfig status updates.
 type statusStrategy struct {
 	strategy
 }
 
-var StatusStrategy = statusStrategy{Strategy}
+var StatusStrategy = statusStrategy{CommonStrategy}
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update of status.
 func (statusStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) {

pkg/deploy/registry/deployconfig/etcd/etcd.go

@@ -6,46 +6,56 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/diff"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/apiserver/pkg/registry/rest"
 
 	deployapi "github.com/openshift/origin/pkg/deploy/apis/apps"
 	deploytest "github.com/openshift/origin/pkg/deploy/apis/apps/test"
 )
 
+var (
+	nonDefaultRevisionHistoryLimit = deployapi.DefaultRevisionHistoryLimit + 42
+)
+
+func int32ptr(v int32) *int32 {
+	return &v
+}
+
 func TestDeploymentConfigStrategy(t *testing.T) {
 	ctx := apirequest.NewDefaultContext()
-	if !Strategy.NamespaceScoped() {
+	if !CommonStrategy.NamespaceScoped() {
 		t.Errorf("DeploymentConfig is namespace scoped")
 	}
-	if Strategy.AllowCreateOnUpdate() {
+	if CommonStrategy.AllowCreateOnUpdate() {
 		t.Errorf("DeploymentConfig should not allow create on update")
 	}
 	deploymentConfig := &deployapi.DeploymentConfig{
 		ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
 		Spec:       deploytest.OkDeploymentConfigSpec(),
 	}
-	Strategy.PrepareForCreate(ctx, deploymentConfig)
-	errs := Strategy.Validate(ctx, deploymentConfig)
+	CommonStrategy.PrepareForCreate(ctx, deploymentConfig)
+	errs := CommonStrategy.Validate(ctx, deploymentConfig)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected error validating %v", errs)
 	}
 	updatedDeploymentConfig := &deployapi.DeploymentConfig{
 		ObjectMeta: metav1.ObjectMeta{Name: "bar", Namespace: "default", Generation: 1},
 		Spec:       deploytest.OkDeploymentConfigSpec(),
 	}
-	errs = Strategy.ValidateUpdate(ctx, updatedDeploymentConfig, deploymentConfig)
+	errs = CommonStrategy.ValidateUpdate(ctx, updatedDeploymentConfig, deploymentConfig)
 	if len(errs) == 0 {
 		t.Errorf("Expected error validating")
 	}
 	// name must match, and resource version must be provided
 	updatedDeploymentConfig.Name = "foo"
 	updatedDeploymentConfig.ResourceVersion = "1"
-	errs = Strategy.ValidateUpdate(ctx, updatedDeploymentConfig, deploymentConfig)
+	errs = CommonStrategy.ValidateUpdate(ctx, updatedDeploymentConfig, deploymentConfig)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected error validating %v", errs)
 	}
 	invalidDeploymentConfig := &deployapi.DeploymentConfig{}
-	errs = Strategy.Validate(ctx, invalidDeploymentConfig)
+	errs = CommonStrategy.Validate(ctx, invalidDeploymentConfig)
 	if len(errs) == 0 {
 		t.Errorf("Expected error validating")
 	}
@@ -119,3 +129,89 @@ func expectedAfterVersionBump() *deployapi.DeploymentConfig {
 	dc.Generation++
 	return dc
 }
+
+func setRevisionHistoryLimit(v *int32, dc *deployapi.DeploymentConfig) *deployapi.DeploymentConfig {
+	dc.Spec.RevisionHistoryLimit = v
+	return dc
+}
+
+func okDeploymentConfig(generation int64) *deployapi.DeploymentConfig {
+	dc := deploytest.OkDeploymentConfig(0)
+	dc.ObjectMeta.Generation = generation
+	return dc
+}
+
+func TestLegacyStrategy_PrepareForCreate(t *testing.T) {
+	nonDefaultRevisionHistoryLimit := deployapi.DefaultRevisionHistoryLimit + 42
+	tt := []struct {
+		obj      *deployapi.DeploymentConfig
+		expected *deployapi.DeploymentConfig
+	}{
+		{
+			obj: setRevisionHistoryLimit(nil, okDeploymentConfig(0)),
+			// Legacy API shall not default RevisionHistoryLimit to maintain backwards compatibility
+			expected: setRevisionHistoryLimit(nil, okDeploymentConfig(1)),
+		},
+		{
+			obj:      setRevisionHistoryLimit(&nonDefaultRevisionHistoryLimit, okDeploymentConfig(0)),
+			expected: setRevisionHistoryLimit(&nonDefaultRevisionHistoryLimit, okDeploymentConfig(1)),
+		},
+	}
+
+	ctx := apirequest.NewDefaultContext()
+
+	for _, tc := range tt {
+		t.Run("", func(t *testing.T) {
+			LegacyStrategy.PrepareForCreate(ctx, tc.obj)
+			if !reflect.DeepEqual(tc.obj, tc.expected) {
+				t.Fatalf("LegacyStrategy.PrepareForCreate failed:%s", diff.ObjectReflectDiff(tc.obj, tc.expected))
+			}
+
+			errs := LegacyStrategy.Validate(ctx, tc.obj)
+			if len(errs) != 0 {
+				t.Errorf("Unexpected error validating DeploymentConfig: %v", errs)
+			}
+		})
+	}
+}
+
+func TestLegacyStrategy_DefaultGarbageCollectionPolicy(t *testing.T) {
+	expected := rest.OrphanDependents
+	got := LegacyStrategy.DefaultGarbageCollectionPolicy()
+	if got != expected {
+		t.Fatalf("Default garbage collection policy for DeploymentConfigs should be %q (not %q)", expected, got)
+	}
+}
+
+func TestGroupStrategy_PrepareForCreate(t *testing.T) {
+	tt := []struct {
+		obj      *deployapi.DeploymentConfig
+		expected *deployapi.DeploymentConfig
+	}{
+		{
+			obj: setRevisionHistoryLimit(nil, okDeploymentConfig(0)),
+			// Group API should default RevisionHistoryLimit
+			expected: setRevisionHistoryLimit(int32ptr(deployapi.DefaultRevisionHistoryLimit), okDeploymentConfig(1)),
+		},
+		{
+			obj:      setRevisionHistoryLimit(&nonDefaultRevisionHistoryLimit, okDeploymentConfig(0)),
+			expected: setRevisionHistoryLimit(&nonDefaultRevisionHistoryLimit, okDeploymentConfig(1)),
+		},
+	}
+
+	ctx := apirequest.NewDefaultContext()
+
+	for _, tc := range tt {
+		t.Run("", func(t *testing.T) {
+			GroupStrategy.PrepareForCreate(ctx, tc.obj)
+			if !reflect.DeepEqual(tc.obj, tc.expected) {
+				t.Fatalf("GroupStrategy.PrepareForCreate failed:%s", diff.ObjectReflectDiff(tc.obj, tc.expected))
+			}
+
+			errs := GroupStrategy.Validate(ctx, tc.obj)
+			if len(errs) != 0 {
+				t.Errorf("Unexpected error validating DeploymentConfig: %v", errs)
+			}
+		})
+	}
+}

pkg/deploy/registry/deployconfig/strategy.go

@@ -146,7 +146,7 @@ func (e *DefaultExporter) Export(obj runtime.Object, exact bool) error {
 		t.Secrets = newMountableSecrets
 
 	case *deployapi.DeploymentConfig:
-		return deployrest.Strategy.Export(ctx, obj, exact)
+		return deployrest.CommonStrategy.Export(ctx, obj, exact)
 
 	case *buildapi.BuildConfig:
 		// Use the legacy strategy to avoid setting prune defaults if

pkg/deploy/registry/deployconfig/strategy_test.go

@@ -4669,7 +4669,7 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope
 						},
 						"revisionHistoryLimit": {
 							SchemaProps: spec.SchemaProps{
-								Description: "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified.",
+								Description: "RevisionHistoryLimit is the number of old ReplicationControllers to retain to allow for rollbacks. This field is a pointer to allow for differentiation between an explicit zero and not specified. Defaults to 10. (This only applies to DeploymentConfigs created via the new group API resource, not the legacy resource.)",
 								Type:        []string{"integer"},
 								Format:      "int32",
 							},