Skip to content

Commit 5efb878

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #148 from tmshort/ocpbugs-63617
OCPBUGS-63617: Update both disable and enable lists when enabling/disabling features
2 parents 39ffd1b + ada8eee commit 5efb878

File tree

7 files changed

+543
-28
lines changed

7 files changed

+543
-28
lines changed

‎internal/featuregates/mapper.go‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,38 +38,46 @@ type Mapper struct {
3838
featureGates gateMapFunc
3939
}
4040

41+
func enableFeature(v *helmvalues.HelmValues, addList, removeList, feature string) error {
42+
var errs []error
43+
errs = append(errs, v.RemoveListValue(removeList, feature))
44+
errs = append(errs, v.AddListValue(addList, feature))
45+
return errors.Join(errs...)
46+
}
47+
func enableCatalogdFeature(v *helmvalues.HelmValues, enabled bool, feature string) error {
48+
if enabled {
49+
return enableFeature(v, helmvalues.EnableCatalogd, helmvalues.DisableCatalogd, feature)
50+
}
51+
return enableFeature(v, helmvalues.DisableCatalogd, helmvalues.EnableCatalogd, feature)
52+
}
53+
54+
func enableOperatorControllerFeature(v *helmvalues.HelmValues, enabled bool, feature string) error {
55+
if enabled {
56+
return enableFeature(v, helmvalues.EnableOperatorController, helmvalues.DisableOperatorController, feature)
57+
}
58+
return enableFeature(v, helmvalues.DisableOperatorController, helmvalues.EnableOperatorController, feature)
59+
}
60+
4161
func NewMapper() *Mapper {
4262
// Add your downstream to upstream mapping here
4363

4464
featureGates := gateMapFunc{
4565
// features.FeatureGateNewOLMMyDownstreamFeature: functon that returns a list of enabled and disabled gates
4666
features.FeatureGateNewOLMPreflightPermissionChecks: func(v *helmvalues.HelmValues, enabled bool) error {
47-
if enabled {
48-
return v.AddListValue(helmvalues.EnableOperatorController, PreflightPermissions)
49-
}
50-
return v.AddListValue(helmvalues.DisableOperatorController, PreflightPermissions)
67+
return enableOperatorControllerFeature(v, enabled, PreflightPermissions)
5168
},
5269
features.FeatureGateNewOLMOwnSingleNamespace: func(v *helmvalues.HelmValues, enabled bool) error {
53-
if enabled {
54-
return v.AddListValue(helmvalues.EnableOperatorController, SingleOwnNamespaceInstallSupport)
55-
}
56-
return v.AddListValue(helmvalues.DisableOperatorController, SingleOwnNamespaceInstallSupport)
70+
return enableOperatorControllerFeature(v, enabled, SingleOwnNamespaceInstallSupport)
5771
},
5872
features.FeatureGateNewOLMWebhookProviderOpenshiftServiceCA: func(v *helmvalues.HelmValues, enabled bool) error {
5973
var errs []error
60-
if enabled {
61-
errs = append(errs, v.AddListValue(helmvalues.EnableOperatorController, WebhookProviderOpenshiftServiceCA))
62-
} else {
63-
errs = append(errs, v.AddListValue(helmvalues.DisableOperatorController, WebhookProviderOpenshiftServiceCA))
64-
}
65-
errs = append(errs, v.AddListValue(helmvalues.DisableOperatorController, WebhookProviderCertManager))
74+
errs = append(errs, enableOperatorControllerFeature(v, enabled, WebhookProviderOpenshiftServiceCA))
75+
// Always disable WebhookProviderCertManager
76+
errs = append(errs, enableOperatorControllerFeature(v, false, WebhookProviderCertManager))
6677
return errors.Join(errs...)
6778
},
6879
features.FeatureGateNewOLMCatalogdAPIV1Metas: func(v *helmvalues.HelmValues, enabled bool) error {
69-
if enabled {
70-
return v.AddListValue(helmvalues.EnableCatalogd, APIV1MetasHandler)
71-
}
72-
return v.AddListValue(helmvalues.DisableCatalogd, APIV1MetasHandler)
80+
return enableCatalogdFeature(v, enabled, APIV1MetasHandler)
7381
},
7482
}
7583

‎internal/featuregates/mapper_test.go‎

Lines changed: 314 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,3 +298,317 @@ func TestMapper_Constants(t *testing.T) {
298298
t.Error("APIV1MetasHandler and PreflightPermissions should be different")
299299
}
300300
}
301+
302+
func TestEnableFeature(t *testing.T) {
303+
tests := []struct {
304+
name string
305+
addList string
306+
removeList string
307+
feature string
308+
initialVals map[string]interface{}
309+
expectedVals map[string]interface{}
310+
}{
311+
{
312+
name: "enable feature in empty values",
313+
addList: helmvalues.EnableOperatorController,
314+
removeList: helmvalues.DisableOperatorController,
315+
feature: PreflightPermissions,
316+
initialVals: make(map[string]interface{}),
317+
expectedVals: map[string]interface{}{
318+
"options": map[string]interface{}{
319+
"operatorController": map[string]interface{}{
320+
"features": map[string]interface{}{
321+
"enabled": []interface{}{PreflightPermissions},
322+
},
323+
},
324+
},
325+
},
326+
},
327+
{
328+
name: "enable feature and remove from disabled list",
329+
addList: helmvalues.EnableOperatorController,
330+
removeList: helmvalues.DisableOperatorController,
331+
feature: PreflightPermissions,
332+
initialVals: map[string]interface{}{
333+
"options": map[string]interface{}{
334+
"operatorController": map[string]interface{}{
335+
"features": map[string]interface{}{
336+
"disabled": []interface{}{PreflightPermissions},
337+
},
338+
},
339+
},
340+
},
341+
expectedVals: map[string]interface{}{
342+
"options": map[string]interface{}{
343+
"operatorController": map[string]interface{}{
344+
"features": map[string]interface{}{
345+
"disabled": []interface{}{},
346+
"enabled": []interface{}{PreflightPermissions},
347+
},
348+
},
349+
},
350+
},
351+
},
352+
{
353+
name: "enable catalogd feature",
354+
addList: helmvalues.EnableCatalogd,
355+
removeList: helmvalues.DisableCatalogd,
356+
feature: APIV1MetasHandler,
357+
initialVals: map[string]interface{}{
358+
"options": map[string]interface{}{
359+
"catalogd": map[string]interface{}{
360+
"features": map[string]interface{}{
361+
"disabled": []interface{}{APIV1MetasHandler},
362+
},
363+
},
364+
},
365+
},
366+
expectedVals: map[string]interface{}{
367+
"options": map[string]interface{}{
368+
"catalogd": map[string]interface{}{
369+
"features": map[string]interface{}{
370+
"disabled": []interface{}{},
371+
"enabled": []interface{}{APIV1MetasHandler},
372+
},
373+
},
374+
},
375+
},
376+
},
377+
}
378+
379+
for _, tt := range tests {
380+
t.Run(tt.name, func(t *testing.T) {
381+
hv := helmvalues.NewHelmValues()
382+
hv.GetValues()
383+
for k, v := range tt.initialVals {
384+
hv.GetValues()[k] = v
385+
}
386+
387+
err := enableFeature(hv, tt.addList, tt.removeList, tt.feature)
388+
if err != nil {
389+
t.Fatalf("Unexpected error: %v", err)
390+
}
391+
392+
actual := hv.GetValues()
393+
if !reflect.DeepEqual(actual, tt.expectedVals) {
394+
t.Errorf("Expected values %v, got %v", tt.expectedVals, actual)
395+
}
396+
})
397+
}
398+
}
399+
400+
func TestEnableOperatorControllerFeature(t *testing.T) {
401+
tests := []struct {
402+
name string
403+
enabled bool
404+
feature string
405+
initialVals map[string]interface{}
406+
expectedVals map[string]interface{}
407+
}{
408+
{
409+
name: "enable feature",
410+
enabled: true,
411+
feature: PreflightPermissions,
412+
initialVals: make(map[string]interface{}),
413+
expectedVals: map[string]interface{}{
414+
"options": map[string]interface{}{
415+
"operatorController": map[string]interface{}{
416+
"features": map[string]interface{}{
417+
"enabled": []interface{}{PreflightPermissions},
418+
},
419+
},
420+
},
421+
},
422+
},
423+
{
424+
name: "disable feature",
425+
enabled: false,
426+
feature: PreflightPermissions,
427+
initialVals: make(map[string]interface{}),
428+
expectedVals: map[string]interface{}{
429+
"options": map[string]interface{}{
430+
"operatorController": map[string]interface{}{
431+
"features": map[string]interface{}{
432+
"disabled": []interface{}{PreflightPermissions},
433+
},
434+
},
435+
},
436+
},
437+
},
438+
{
439+
name: "enable feature removes from disabled",
440+
enabled: true,
441+
feature: SingleOwnNamespaceInstallSupport,
442+
initialVals: map[string]interface{}{
443+
"options": map[string]interface{}{
444+
"operatorController": map[string]interface{}{
445+
"features": map[string]interface{}{
446+
"disabled": []interface{}{SingleOwnNamespaceInstallSupport},
447+
},
448+
},
449+
},
450+
},
451+
expectedVals: map[string]interface{}{
452+
"options": map[string]interface{}{
453+
"operatorController": map[string]interface{}{
454+
"features": map[string]interface{}{
455+
"disabled": []interface{}{},
456+
"enabled": []interface{}{SingleOwnNamespaceInstallSupport},
457+
},
458+
},
459+
},
460+
},
461+
},
462+
{
463+
name: "disable feature removes from enabled",
464+
enabled: false,
465+
feature: SingleOwnNamespaceInstallSupport,
466+
initialVals: map[string]interface{}{
467+
"options": map[string]interface{}{
468+
"operatorController": map[string]interface{}{
469+
"features": map[string]interface{}{
470+
"enabled": []interface{}{SingleOwnNamespaceInstallSupport},
471+
},
472+
},
473+
},
474+
},
475+
expectedVals: map[string]interface{}{
476+
"options": map[string]interface{}{
477+
"operatorController": map[string]interface{}{
478+
"features": map[string]interface{}{
479+
"enabled": []interface{}{},
480+
"disabled": []interface{}{SingleOwnNamespaceInstallSupport},
481+
},
482+
},
483+
},
484+
},
485+
},
486+
}
487+
488+
for _, tt := range tests {
489+
t.Run(tt.name, func(t *testing.T) {
490+
hv := helmvalues.NewHelmValues()
491+
for k, v := range tt.initialVals {
492+
hv.GetValues()[k] = v
493+
}
494+
495+
err := enableOperatorControllerFeature(hv, tt.enabled, tt.feature)
496+
if err != nil {
497+
t.Fatalf("Unexpected error: %v", err)
498+
}
499+
500+
actual := hv.GetValues()
501+
if !reflect.DeepEqual(actual, tt.expectedVals) {
502+
t.Errorf("Expected values %v, got %v", tt.expectedVals, actual)
503+
}
504+
})
505+
}
506+
}
507+
508+
func TestEnableCatalogdFeature(t *testing.T) {
509+
tests := []struct {
510+
name string
511+
enabled bool
512+
feature string
513+
initialVals map[string]interface{}
514+
expectedVals map[string]interface{}
515+
}{
516+
{
517+
name: "enable catalogd feature",
518+
enabled: true,
519+
feature: APIV1MetasHandler,
520+
initialVals: make(map[string]interface{}),
521+
expectedVals: map[string]interface{}{
522+
"options": map[string]interface{}{
523+
"catalogd": map[string]interface{}{
524+
"features": map[string]interface{}{
525+
"enabled": []interface{}{APIV1MetasHandler},
526+
},
527+
},
528+
},
529+
},
530+
},
531+
{
532+
name: "disable catalogd feature",
533+
enabled: false,
534+
feature: APIV1MetasHandler,
535+
initialVals: make(map[string]interface{}),
536+
expectedVals: map[string]interface{}{
537+
"options": map[string]interface{}{
538+
"catalogd": map[string]interface{}{
539+
"features": map[string]interface{}{
540+
"disabled": []interface{}{APIV1MetasHandler},
541+
},
542+
},
543+
},
544+
},
545+
},
546+
{
547+
name: "enable catalogd feature removes from disabled",
548+
enabled: true,
549+
feature: APIV1MetasHandler,
550+
initialVals: map[string]interface{}{
551+
"options": map[string]interface{}{
552+
"catalogd": map[string]interface{}{
553+
"features": map[string]interface{}{
554+
"disabled": []interface{}{APIV1MetasHandler},
555+
},
556+
},
557+
},
558+
},
559+
expectedVals: map[string]interface{}{
560+
"options": map[string]interface{}{
561+
"catalogd": map[string]interface{}{
562+
"features": map[string]interface{}{
563+
"disabled": []interface{}{},
564+
"enabled": []interface{}{APIV1MetasHandler},
565+
},
566+
},
567+
},
568+
},
569+
},
570+
{
571+
name: "disable catalogd feature removes from enabled",
572+
enabled: false,
573+
feature: APIV1MetasHandler,
574+
initialVals: map[string]interface{}{
575+
"options": map[string]interface{}{
576+
"catalogd": map[string]interface{}{
577+
"features": map[string]interface{}{
578+
"enabled": []interface{}{APIV1MetasHandler},
579+
},
580+
},
581+
},
582+
},
583+
expectedVals: map[string]interface{}{
584+
"options": map[string]interface{}{
585+
"catalogd": map[string]interface{}{
586+
"features": map[string]interface{}{
587+
"enabled": []interface{}{},
588+
"disabled": []interface{}{APIV1MetasHandler},
589+
},
590+
},
591+
},
592+
},
593+
},
594+
}
595+
596+
for _, tt := range tests {
597+
t.Run(tt.name, func(t *testing.T) {
598+
hv := helmvalues.NewHelmValues()
599+
for k, v := range tt.initialVals {
600+
hv.GetValues()[k] = v
601+
}
602+
603+
err := enableCatalogdFeature(hv, tt.enabled, tt.feature)
604+
if err != nil {
605+
t.Fatalf("Unexpected error: %v", err)
606+
}
607+
608+
actual := hv.GetValues()
609+
if !reflect.DeepEqual(actual, tt.expectedVals) {
610+
t.Errorf("Expected values %v, got %v", tt.expectedVals, actual)
611+
}
612+
})
613+
}
614+
}

‎pkg/controller/featuregates_hook.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,12 @@ import (
1111
// upstreamFeatureGates builds a set of helm values for the downsteeam feature-gates that are
1212
// mapped to upstream feature-gates
1313
func upstreamFeatureGates(
14+
values *helmvalues.HelmValues,
1415
clusterGatesConfig featuregates.FeatureGate,
1516
downstreamGates []configv1.FeatureGateName,
1617
downstreamToUpstreamFunc func(configv1.FeatureGateName) func(*helmvalues.HelmValues, bool) error,
1718
) (*helmvalues.HelmValues, error) {
1819
errs := make([]error, 0, len(downstreamGates))
19-
values := helmvalues.NewHelmValues()
2020

2121
for _, downstreamGate := range downstreamGates {
2222
f := downstreamToUpstreamFunc(downstreamGate)

0 commit comments

Comments
 (0)