Skip to content

Commit 583c3bd

Browse files
smarterclaytonsmarterclayton
authored
Merge pull request #14845 from bparees/default_pruning
set default build prune limits for group api
2 parents 98c15cb + 17a2e33 commit 583c3bd

File tree

12 files changed

+366
-73
lines changed

12 files changed

+366
-73
lines changed

‎pkg/build/apis/build/types.go

+15-3
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,23 @@ const (
8080
BuildCancelledEventReason = "BuildCancelled"
8181
// BuildCancelledEventMessage is the message associated with the event registered when build is cancelled.
8282
BuildCancelledEventMessage = "Build %s/%s has been cancelled"
83+
84+
// DefaultSuccessfulBuildsHistoryLimit is the default number of successful builds to retain
85+
// if the buildconfig does not specify a value. This only applies to buildconfigs created
86+
// via the new group api resource, not the legacy resource.
87+
DefaultSuccessfulBuildsHistoryLimit = int32(5)
88+
89+
// DefaultFailedBuildsHistoryLimit is the default number of failed builds to retain
90+
// if the buildconfig does not specify a value. This only applies to buildconfigs created
91+
// via the new group api resource, not the legacy resource.
92+
DefaultFailedBuildsHistoryLimit = int32(5)
8393
)
8494

85-
// WhitelistEnvVarNames is a list of environment variable keys that are allowed to be set by the
86-
// user on the build pod.
87-
var WhitelistEnvVarNames = [2]string{"BUILD_LOGLEVEL", "GIT_SSL_NO_VERIFY"}
95+
var (
96+
// WhitelistEnvVarNames is a list of environment variable keys that are allowed to be set by the
97+
// user on the build pod.
98+
WhitelistEnvVarNames = [2]string{"BUILD_LOGLEVEL", "GIT_SSL_NO_VERIFY"}
99+
)
88100

89101
// +genclient=true
90102

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ func NewREST(optsGetter restoptions.Getter) (*REST, error) {
2424
QualifiedResource: buildapi.Resource("buildconfigs"),
2525
PredicateFunc: buildconfig.Matcher,
2626

27-
CreateStrategy: buildconfig.Strategy,
28-
UpdateStrategy: buildconfig.Strategy,
29-
DeleteStrategy: buildconfig.Strategy,
27+
CreateStrategy: buildconfig.GroupStrategy,
28+
UpdateStrategy: buildconfig.GroupStrategy,
29+
DeleteStrategy: buildconfig.GroupStrategy,
3030
}
3131

3232
options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: buildconfig.GetAttrs}

‎pkg/build/registry/buildconfig/strategy.go

+63-9
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"k8s.io/apimachinery/pkg/runtime"
1010
"k8s.io/apimachinery/pkg/util/validation/field"
1111
apirequest "k8s.io/apiserver/pkg/endpoints/request"
12+
"k8s.io/apiserver/pkg/registry/rest"
1213
kstorage "k8s.io/apiserver/pkg/storage"
1314
"k8s.io/apiserver/pkg/storage/names"
1415
kapi "k8s.io/kubernetes/pkg/api"
@@ -17,15 +18,28 @@ import (
1718
"github.com/openshift/origin/pkg/build/apis/build/validation"
1819
)
1920

20-
// strategy implements behavior for BuildConfig objects
21+
var (
22+
// GroupStrategy is the logic that applies when creating and updating BuildConfig objects
23+
// in the Group api.
24+
// This differs from the LegacyStrategy in that on create it will set a default build
25+
// pruning limit value for both successful and failed builds. This is new behavior that
26+
// can only be introduced to users consuming the new group based api.
27+
GroupStrategy = groupStrategy{strategy{kapi.Scheme, names.SimpleNameGenerator}}
28+
29+
// LegacyStrategy is the default logic that applies when creating BuildConfig objects.
30+
// Specifically it will not set the default build pruning limit values because that was not
31+
// part of the legacy api.
32+
LegacyStrategy = legacyStrategy{strategy{kapi.Scheme, names.SimpleNameGenerator}}
33+
)
34+
35+
// strategy implements most of the behavior for BuildConfig objects
36+
// It does not provide a PrepareForCreate implementation, that is expected
37+
// to be implemented by the child implementation.
2138
type strategy struct {
2239
runtime.ObjectTyper
2340
names.NameGenerator
2441
}
2542

26-
// Strategy is the default logic that applies when creating and updating BuildConfig objects.
27-
var Strategy = strategy{kapi.Scheme, names.SimpleNameGenerator}
28-
2943
func (strategy) NamespaceScoped() bool {
3044
return true
3145
}
@@ -39,16 +53,17 @@ func (strategy) AllowUnconditionalUpdate() bool {
3953
return false
4054
}
4155

56+
// Canonicalize normalizes the object after validation.
57+
func (strategy) Canonicalize(obj runtime.Object) {
58+
}
59+
4260
// PrepareForCreate clears fields that are not allowed to be set by end users on creation.
43-
func (strategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
61+
// This is invoked by the Group and Legacy strategies.
62+
func (s strategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
4463
bc := obj.(*buildapi.BuildConfig)
4564
dropUnknownTriggers(bc)
4665
}
4766

48-
// Canonicalize normalizes the object after validation.
49-
func (strategy) Canonicalize(obj runtime.Object) {
50-
}
51-
5267
// PrepareForUpdate clears fields that are not allowed to be set by end users on update.
5368
func (strategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) {
5469
newBC := obj.(*buildapi.BuildConfig)
@@ -71,6 +86,45 @@ func (strategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object)
7186
return validation.ValidateBuildConfigUpdate(obj.(*buildapi.BuildConfig), old.(*buildapi.BuildConfig))
7287
}
7388

89+
// groupStrategy implements behavior for BuildConfig objects in the Group api
90+
type groupStrategy struct {
91+
strategy
92+
}
93+
94+
// PrepareForCreate delegates to the common strategy and sets default pruning limits
95+
func (s groupStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
96+
s.strategy.PrepareForCreate(ctx, obj)
97+
98+
bc := obj.(*buildapi.BuildConfig)
99+
if bc.Spec.SuccessfulBuildsHistoryLimit == nil {
100+
v := buildapi.DefaultSuccessfulBuildsHistoryLimit
101+
bc.Spec.SuccessfulBuildsHistoryLimit = &v
102+
}
103+
if bc.Spec.FailedBuildsHistoryLimit == nil {
104+
v := buildapi.DefaultFailedBuildsHistoryLimit
105+
bc.Spec.FailedBuildsHistoryLimit = &v
106+
}
107+
}
108+
109+
// legacyStrategy implements behavior for BuildConfig objects in the legacy api
110+
type legacyStrategy struct {
111+
strategy
112+
}
113+
114+
// PrepareForCreate delegates to the common strategy.
115+
func (s legacyStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {
116+
s.strategy.PrepareForCreate(ctx, obj)
117+
118+
// legacy buildconfig api does not apply default pruning values, to maintain
119+
// backwards compatibility.
120+
121+
}
122+
123+
// DefaultGarbageCollectionPolicy for legacy buildconfigs will orphan dependents.
124+
func (s legacyStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
125+
return rest.OrphanDependents
126+
}
127+
74128
// GetAttrs returns labels and fields of a given object for filtering purposes
75129
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) {
76130
buildConfig, ok := obj.(*buildapi.BuildConfig)

‎pkg/build/registry/buildconfig/strategy_test.go

+139-10
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ import (
1010
buildapi "github.com/openshift/origin/pkg/build/apis/build"
1111
)
1212

13-
func TestBuildConfigStrategy(t *testing.T) {
13+
func TestBuildConfigGroupStrategy(t *testing.T) {
1414
ctx := apirequest.NewDefaultContext()
15-
if !Strategy.NamespaceScoped() {
15+
if !GroupStrategy.NamespaceScoped() {
1616
t.Errorf("BuildConfig is namespace scoped")
1717
}
18-
if Strategy.AllowCreateOnUpdate() {
18+
if GroupStrategy.AllowCreateOnUpdate() {
1919
t.Errorf("BuildConfig should not allow create on update")
2020
}
2121
buildConfig := &buildapi.BuildConfig{
@@ -88,34 +88,163 @@ func TestBuildConfigStrategy(t *testing.T) {
8888
LastVersion: 9,
8989
},
9090
}
91-
Strategy.PrepareForCreate(ctx, buildConfig)
92-
errs := Strategy.Validate(ctx, buildConfig)
91+
GroupStrategy.PrepareForCreate(ctx, buildConfig)
92+
errs := GroupStrategy.Validate(ctx, buildConfig)
9393
if len(errs) != 0 {
9494
t.Errorf("Unexpected error validating %v", errs)
9595
}
96+
if buildConfig.Spec.SuccessfulBuildsHistoryLimit == nil {
97+
t.Errorf("Expected successful limit %d, got nil", buildapi.DefaultSuccessfulBuildsHistoryLimit)
98+
}
99+
if *buildConfig.Spec.SuccessfulBuildsHistoryLimit != buildapi.DefaultSuccessfulBuildsHistoryLimit {
100+
t.Errorf("Expected successful limit %d, got %d", buildapi.DefaultSuccessfulBuildsHistoryLimit, *buildConfig.Spec.SuccessfulBuildsHistoryLimit)
101+
}
102+
if buildConfig.Spec.FailedBuildsHistoryLimit == nil {
103+
t.Errorf("Expected failed limit %d, got nil", buildapi.DefaultFailedBuildsHistoryLimit)
104+
}
105+
if *buildConfig.Spec.FailedBuildsHistoryLimit != buildapi.DefaultFailedBuildsHistoryLimit {
106+
t.Errorf("Expected failed limit %d, got %d", buildapi.DefaultFailedBuildsHistoryLimit, *buildConfig.Spec.FailedBuildsHistoryLimit)
107+
}
108+
109+
// lastversion cannot go backwards
110+
newBC.Status.LastVersion = 9
111+
GroupStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
112+
if newBC.Status.LastVersion != buildConfig.Status.LastVersion {
113+
t.Errorf("Expected version=%d, got %d", buildConfig.Status.LastVersion, newBC.Status.LastVersion)
114+
}
115+
116+
// lastversion can go forwards
117+
newBC.Status.LastVersion = 11
118+
GroupStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
119+
if newBC.Status.LastVersion != 11 {
120+
t.Errorf("Expected version=%d, got %d", 11, newBC.Status.LastVersion)
121+
}
122+
123+
GroupStrategy.PrepareForCreate(ctx, buildConfig)
124+
errs = GroupStrategy.Validate(ctx, buildConfig)
125+
if len(errs) != 0 {
126+
t.Errorf("Unexpected error validating %v", errs)
127+
}
128+
129+
invalidBuildConfig := &buildapi.BuildConfig{}
130+
errs = GroupStrategy.Validate(ctx, invalidBuildConfig)
131+
if len(errs) == 0 {
132+
t.Errorf("Expected error validating")
133+
}
134+
}
135+
136+
func TestBuildConfigLegacyStrategy(t *testing.T) {
137+
ctx := apirequest.NewDefaultContext()
138+
if !LegacyStrategy.NamespaceScoped() {
139+
t.Errorf("BuildConfig is namespace scoped")
140+
}
141+
if LegacyStrategy.AllowCreateOnUpdate() {
142+
t.Errorf("BuildConfig should not allow create on update")
143+
}
144+
buildConfig := &buildapi.BuildConfig{
145+
ObjectMeta: metav1.ObjectMeta{Name: "config-id", Namespace: "namespace"},
146+
Spec: buildapi.BuildConfigSpec{
147+
RunPolicy: buildapi.BuildRunPolicySerial,
148+
Triggers: []buildapi.BuildTriggerPolicy{
149+
{
150+
GitHubWebHook: &buildapi.WebHookTrigger{Secret: "12345"},
151+
Type: buildapi.GitHubWebHookBuildTriggerType,
152+
},
153+
{
154+
Type: "unknown",
155+
},
156+
},
157+
CommonSpec: buildapi.CommonSpec{
158+
Source: buildapi.BuildSource{
159+
Git: &buildapi.GitBuildSource{
160+
URI: "http://github.com/my/repository",
161+
},
162+
ContextDir: "context",
163+
},
164+
Strategy: buildapi.BuildStrategy{
165+
DockerStrategy: &buildapi.DockerBuildStrategy{},
166+
},
167+
Output: buildapi.BuildOutput{
168+
To: &kapi.ObjectReference{
169+
Kind: "DockerImage",
170+
Name: "repository/data",
171+
},
172+
},
173+
},
174+
},
175+
Status: buildapi.BuildConfigStatus{
176+
LastVersion: 10,
177+
},
178+
}
179+
newBC := &buildapi.BuildConfig{
180+
ObjectMeta: metav1.ObjectMeta{Name: "config-id", Namespace: "namespace"},
181+
Spec: buildapi.BuildConfigSpec{
182+
RunPolicy: buildapi.BuildRunPolicySerial,
183+
Triggers: []buildapi.BuildTriggerPolicy{
184+
{
185+
GitHubWebHook: &buildapi.WebHookTrigger{Secret: "12345"},
186+
Type: buildapi.GitHubWebHookBuildTriggerType,
187+
},
188+
{
189+
Type: "unknown",
190+
},
191+
},
192+
CommonSpec: buildapi.CommonSpec{
193+
Source: buildapi.BuildSource{
194+
Git: &buildapi.GitBuildSource{
195+
URI: "http://github.com/my/repository",
196+
},
197+
ContextDir: "context",
198+
},
199+
Strategy: buildapi.BuildStrategy{
200+
DockerStrategy: &buildapi.DockerBuildStrategy{},
201+
},
202+
Output: buildapi.BuildOutput{
203+
To: &kapi.ObjectReference{
204+
Kind: "DockerImage",
205+
Name: "repository/data",
206+
},
207+
},
208+
},
209+
},
210+
Status: buildapi.BuildConfigStatus{
211+
LastVersion: 9,
212+
},
213+
}
214+
LegacyStrategy.PrepareForCreate(ctx, buildConfig)
215+
errs := LegacyStrategy.Validate(ctx, buildConfig)
216+
if len(errs) != 0 {
217+
t.Errorf("Unexpected error validating %v", errs)
218+
}
219+
if buildConfig.Spec.SuccessfulBuildsHistoryLimit != nil {
220+
t.Errorf("Expected successful limit to be nil, got %d", *buildConfig.Spec.SuccessfulBuildsHistoryLimit)
221+
}
222+
if buildConfig.Spec.FailedBuildsHistoryLimit != nil {
223+
t.Errorf("Expected failed limit to be nil, got %d", *buildConfig.Spec.FailedBuildsHistoryLimit)
224+
}
96225

97226
// lastversion cannot go backwards
98227
newBC.Status.LastVersion = 9
99-
Strategy.PrepareForUpdate(ctx, newBC, buildConfig)
228+
LegacyStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
100229
if newBC.Status.LastVersion != buildConfig.Status.LastVersion {
101230
t.Errorf("Expected version=%d, got %d", buildConfig.Status.LastVersion, newBC.Status.LastVersion)
102231
}
103232

104233
// lastversion can go forwards
105234
newBC.Status.LastVersion = 11
106-
Strategy.PrepareForUpdate(ctx, newBC, buildConfig)
235+
LegacyStrategy.PrepareForUpdate(ctx, newBC, buildConfig)
107236
if newBC.Status.LastVersion != 11 {
108237
t.Errorf("Expected version=%d, got %d", 11, newBC.Status.LastVersion)
109238
}
110239

111-
Strategy.PrepareForCreate(ctx, buildConfig)
112-
errs = Strategy.Validate(ctx, buildConfig)
240+
LegacyStrategy.PrepareForCreate(ctx, buildConfig)
241+
errs = LegacyStrategy.Validate(ctx, buildConfig)
113242
if len(errs) != 0 {
114243
t.Errorf("Unexpected error validating %v", errs)
115244
}
116245

117246
invalidBuildConfig := &buildapi.BuildConfig{}
118-
errs = Strategy.Validate(ctx, invalidBuildConfig)
247+
errs = LegacyStrategy.Validate(ctx, invalidBuildConfig)
119248
if len(errs) == 0 {
120249
t.Errorf("Expected error validating")
121250
}

‎pkg/cmd/cli/cmd/exporter.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,10 @@ func (e *DefaultExporter) Export(obj runtime.Object, exact bool) error {
148148
return deployrest.Strategy.Export(ctx, obj, exact)
149149

150150
case *buildapi.BuildConfig:
151-
buildconfigrest.Strategy.PrepareForCreate(ctx, obj)
151+
// Use the legacy strategy to avoid setting prune defaults if
152+
// the object wasn't created with them in the first place.
153+
// TODO: use the exportstrategy pattern instead.
154+
buildconfigrest.LegacyStrategy.PrepareForCreate(ctx, obj)
152155
// TODO: should be handled by prepare for create
153156
t.Status.LastVersion = 0
154157
for i := range t.Spec.Triggers {

‎pkg/cmd/server/origin/legacy.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"k8s.io/apimachinery/pkg/util/sets"
66
"k8s.io/apiserver/pkg/registry/rest"
77

8+
buildconfig "github.com/openshift/origin/pkg/build/registry/buildconfig"
89
buildconfigetcd "github.com/openshift/origin/pkg/build/registry/buildconfig/etcd"
910
deploymentconfigetcd "github.com/openshift/origin/pkg/deploy/registry/deployconfig/etcd"
1011
routeregistry "github.com/openshift/origin/pkg/route/registry/route"
@@ -199,7 +200,8 @@ func LegacyStorage(storage map[schema.GroupVersion]map[string]rest.Storage) map[
199200
case "buildConfigs":
200201
restStorage := s.(*buildconfigetcd.REST)
201202
store := *restStorage.Store
202-
store.DeleteStrategy = orphanByDefault(store.DeleteStrategy)
203+
store.DeleteStrategy = buildconfig.LegacyStrategy
204+
store.CreateStrategy = buildconfig.LegacyStrategy
203205
legacyStorage[resource] = &buildconfigetcd.REST{Store: &store}
204206
case "deploymentConfigs":
205207
restStorage := s.(*deploymentconfigetcd.REST)

0 commit comments

Comments
 (0)