Skip to content

Commit

Permalink
Extend reconciler name in validation errors
Browse files Browse the repository at this point in the history 
- all reconciler are now named
- include reconciler type/name in field validation that is a nil check

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
  • Loading branch information
@scothis
scothis committed May 9, 2022
1 parent 43c7cb3 commit 11d1639
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 33 deletions.
20 changes: 10 additions & 10 deletions reconcilers/reconcilers.go
View file
Expand Up @@ -690,12 +690,12 @@ func (r *ChildReconciler) validate(ctx context.Context) error {

// validate ChildType value
if r.ChildType == nil {
return fmt.Errorf("ChildType must be defined")
return fmt.Errorf("ChildReconciler %q must define ChildType", r.Name)
}

// validate ChildListType value
if r.ChildListType == nil {
return fmt.Errorf("ChildListType must be defined")
return fmt.Errorf("ChildReconciler %q must define ChildListType", r.Name)
}

// validate DesiredChild function signature:
Expand Down Expand Up @@ -748,7 +748,7 @@ func (r *ChildReconciler) validate(ctx context.Context) error {
if fn.NumIn() != 2 || fn.NumOut() != 0 ||
!reflect.TypeOf(r.ChildType).AssignableTo(fn.In(0)) ||
!reflect.TypeOf(r.ChildType).AssignableTo(fn.In(1)) {
return fmt.Errorf("ChildReconciler %q must implement MergeBeforeUpdate: nil | func(%s, %s), found: %s", r.Name, reflect.TypeOf(r.ChildType), reflect.TypeOf(r.ChildType), fn)
return fmt.Errorf("ChildReconciler %q must implement MergeBeforeUpdate: func(%s, %s), found: %s", r.Name, reflect.TypeOf(r.ChildType), reflect.TypeOf(r.ChildType), fn)
}
}

Expand All @@ -762,7 +762,7 @@ func (r *ChildReconciler) validate(ctx context.Context) error {
!reflect.TypeOf(r.ChildType).AssignableTo(fn.In(0)) ||
!reflect.TypeOf(r.ChildType).AssignableTo(fn.In(1)) ||
fn.Out(0).Kind() != reflect.Bool {
return fmt.Errorf("ChildReconciler %q must implement SemanticEquals: nil | func(%s, %s) bool, found: %s", r.Name, reflect.TypeOf(r.ChildType), reflect.TypeOf(r.ChildType), fn)
return fmt.Errorf("ChildReconciler %q must implement SemanticEquals: func(%s, %s) bool, found: %s", r.Name, reflect.TypeOf(r.ChildType), reflect.TypeOf(r.ChildType), fn)
}
}

Expand Down Expand Up @@ -1164,12 +1164,12 @@ func (r *CastParent) SetupWithManager(ctx context.Context, mgr ctrl.Manager, bld
func (r *CastParent) validate(ctx context.Context) error {
// validate Type value
if r.Type == nil {
return fmt.Errorf("Type must be defined")
return fmt.Errorf("CastParent %q must define Type", r.Name)
}

// validate Reconciler value
if r.Reconciler == nil {
return fmt.Errorf("Reconciler must be defined")
return fmt.Errorf("CastParent %q must define Reconciler", r.Name)
}

return nil
Expand Down Expand Up @@ -1266,12 +1266,12 @@ func (r *WithConfig) SetupWithManager(ctx context.Context, mgr ctrl.Manager, bld
func (r *WithConfig) validate(ctx context.Context) error {
// validate Config value
if r.Config == nil {
return fmt.Errorf("Config must be defined")
return fmt.Errorf("WithConfig %q must define Config", r.Name)
}

// validate Reconciler value
if r.Reconciler == nil {
return fmt.Errorf("Reconciler must be defined")
return fmt.Errorf("WithConfig %q must define Reconciler", r.Name)
}

return nil
Expand Down Expand Up @@ -1333,12 +1333,12 @@ func (r *WithFinalizer) SetupWithManager(ctx context.Context, mgr ctrl.Manager,
func (r *WithFinalizer) validate(ctx context.Context) error {
// validate Finalizer value
if r.Finalizer == "" {
return fmt.Errorf("Finalizer must be defined")
return fmt.Errorf("WithFinalizer %q must define Finalizer", r.Name)
}

// validate Reconciler value
if r.Reconciler == nil {
return fmt.Errorf("Reconciler must be defined")
return fmt.Errorf("WithFinalizer %q must define Reconciler", r.Name)
}

return nil
Expand Down
54 changes: 31 additions & 23 deletions reconcilers/reconcilers_validate_test.go
View file
Expand Up @@ -25,8 +25,8 @@ func TestSyncReconciler_validate(t *testing.T) {
{
name: "empty",
parent: &corev1.ConfigMap{},
reconciler: &SyncReconciler{Name: "empty"},
shouldErr: `SyncReconciler "empty" must implement Sync`,
reconciler: &SyncReconciler{},
shouldErr: `SyncReconciler "" must implement Sync`,
},
{
name: "valid",
Expand Down Expand Up @@ -217,7 +217,7 @@ func TestChildReconciler_validate(t *testing.T) {
name: "empty",
parent: &corev1.ConfigMap{},
reconciler: &ChildReconciler{},
shouldErr: "ChildType must be defined",
shouldErr: `ChildReconciler "" must define ChildType`,
},
{
name: "valid",
Expand All @@ -235,27 +235,29 @@ func TestChildReconciler_validate(t *testing.T) {
name: "ChildType missing",
parent: &corev1.ConfigMap{},
reconciler: &ChildReconciler{
Name: "ChildType missing",
// ChildType: &corev1.Pod{},
ChildListType: &corev1.PodList{},
DesiredChild: func(ctx context.Context, parent *corev1.ConfigMap) (*corev1.Pod, error) { return nil, nil },
ReflectChildStatusOnParent: func(parent *corev1.ConfigMap, child *corev1.Pod, err error) {},
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func(a1, a2 *corev1.Pod) bool { return false },
},
shouldErr: "ChildType must be defined",
shouldErr: `ChildReconciler "ChildType missing" must define ChildType`,
},
{
name: "ChildListType missing",
parent: &corev1.ConfigMap{},
reconciler: &ChildReconciler{
Name: "ChildListType missing",
ChildType: &corev1.Pod{},
// ChildListType: &corev1.PodList{},
DesiredChild: func(ctx context.Context, parent *corev1.ConfigMap) (*corev1.Pod, error) { return nil, nil },
ReflectChildStatusOnParent: func(parent *corev1.ConfigMap, child *corev1.Pod, err error) {},
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func(a1, a2 *corev1.Pod) bool { return false },
},
shouldErr: "ChildListType must be defined",
shouldErr: `ChildReconciler "ChildListType missing" must define ChildListType`,
},
{
name: "DesiredChild missing",
Expand Down Expand Up @@ -465,7 +467,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func() {},
SemanticEquals: func(a1, a2 *corev1.Pod) bool { return false },
},
shouldErr: `ChildReconciler "MergeBeforeUpdate num in" must implement MergeBeforeUpdate: nil | func(*v1.Pod, *v1.Pod), found: func()`,
shouldErr: `ChildReconciler "MergeBeforeUpdate num in" must implement MergeBeforeUpdate: func(*v1.Pod, *v1.Pod), found: func()`,
},
{
name: "MergeBeforeUpdate in 0",
Expand All @@ -479,7 +481,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current *corev1.Secret, desired *corev1.Pod) {},
SemanticEquals: func(a1, a2 *corev1.Pod) bool { return false },
},
shouldErr: `ChildReconciler "MergeBeforeUpdate in 0" must implement MergeBeforeUpdate: nil | func(*v1.Pod, *v1.Pod), found: func(*v1.Secret, *v1.Pod)`,
shouldErr: `ChildReconciler "MergeBeforeUpdate in 0" must implement MergeBeforeUpdate: func(*v1.Pod, *v1.Pod), found: func(*v1.Secret, *v1.Pod)`,
},
{
name: "MergeBeforeUpdate in 1",
Expand All @@ -493,7 +495,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current *corev1.Pod, desired *corev1.Secret) {},
SemanticEquals: func(a1, a2 *corev1.Pod) bool { return false },
},
shouldErr: `ChildReconciler "MergeBeforeUpdate in 1" must implement MergeBeforeUpdate: nil | func(*v1.Pod, *v1.Pod), found: func(*v1.Pod, *v1.Secret)`,
shouldErr: `ChildReconciler "MergeBeforeUpdate in 1" must implement MergeBeforeUpdate: func(*v1.Pod, *v1.Pod), found: func(*v1.Pod, *v1.Secret)`,
},
{
name: "MergeBeforeUpdate num out",
Expand All @@ -507,7 +509,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current, desired *corev1.Pod) error { return nil },
SemanticEquals: func(a1, a2 *corev1.Pod) bool { return false },
},
shouldErr: `ChildReconciler "MergeBeforeUpdate num out" must implement MergeBeforeUpdate: nil | func(*v1.Pod, *v1.Pod), found: func(*v1.Pod, *v1.Pod) error`,
shouldErr: `ChildReconciler "MergeBeforeUpdate num out" must implement MergeBeforeUpdate: func(*v1.Pod, *v1.Pod), found: func(*v1.Pod, *v1.Pod) error`,
},
{
name: "SemanticEquals missing",
Expand Down Expand Up @@ -535,7 +537,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func() bool { return false },
},
shouldErr: `ChildReconciler "SemanticEquals num in" must implement SemanticEquals: nil | func(*v1.Pod, *v1.Pod) bool, found: func() bool`,
shouldErr: `ChildReconciler "SemanticEquals num in" must implement SemanticEquals: func(*v1.Pod, *v1.Pod) bool, found: func() bool`,
},
{
name: "SemanticEquals in 0",
Expand All @@ -549,7 +551,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func(a1 *corev1.Secret, a2 *corev1.Pod) bool { return false },
},
shouldErr: `ChildReconciler "SemanticEquals in 0" must implement SemanticEquals: nil | func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Secret, *v1.Pod) bool`,
shouldErr: `ChildReconciler "SemanticEquals in 0" must implement SemanticEquals: func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Secret, *v1.Pod) bool`,
},
{
name: "SemanticEquals in 1",
Expand All @@ -563,7 +565,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func(a1 *corev1.Pod, a2 *corev1.Secret) bool { return false },
},
shouldErr: `ChildReconciler "SemanticEquals in 1" must implement SemanticEquals: nil | func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Pod, *v1.Secret) bool`,
shouldErr: `ChildReconciler "SemanticEquals in 1" must implement SemanticEquals: func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Pod, *v1.Secret) bool`,
},
{
name: "SemanticEquals num out",
Expand All @@ -577,7 +579,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func(a1, a2 *corev1.Pod) {},
},
shouldErr: `ChildReconciler "SemanticEquals num out" must implement SemanticEquals: nil | func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Pod, *v1.Pod)`,
shouldErr: `ChildReconciler "SemanticEquals num out" must implement SemanticEquals: func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Pod, *v1.Pod)`,
},
{
name: "SemanticEquals out 0",
Expand All @@ -591,7 +593,7 @@ func TestChildReconciler_validate(t *testing.T) {
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SemanticEquals: func(a1, a2 *corev1.Pod) error { return nil },
},
shouldErr: `ChildReconciler "SemanticEquals out 0" must implement SemanticEquals: nil | func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Pod, *v1.Pod) error`,
shouldErr: `ChildReconciler "SemanticEquals out 0" must implement SemanticEquals: func(*v1.Pod, *v1.Pod) bool, found: func(*v1.Pod, *v1.Pod) error`,
},
{
name: "HarmonizeImmutableFields",
Expand Down Expand Up @@ -836,7 +838,7 @@ func TestCastParent_validate(t *testing.T) {
name: "empty",
parent: &corev1.ConfigMap{},
reconciler: &CastParent{},
shouldErr: "Type must be defined",
shouldErr: `CastParent "" must define Type`,
},
{
name: "valid",
Expand All @@ -854,23 +856,25 @@ func TestCastParent_validate(t *testing.T) {
name: "missing type",
parent: &corev1.ConfigMap{},
reconciler: &CastParent{
Name: "missing type",
Type: nil,
Reconciler: &SyncReconciler{
Sync: func(ctx context.Context, parent *corev1.Secret) error {
return nil
},
},
},
shouldErr: "Type must be defined",
shouldErr: `CastParent "missing type" must define Type`,
},
{
name: "missing reconciler",
parent: &corev1.ConfigMap{},
reconciler: &CastParent{
Name: "missing reconciler",
Type: &corev1.Secret{},
Reconciler: nil,
},
shouldErr: "Reconciler must be defined",
shouldErr: `CastParent "missing reconciler" must define Reconciler`,
},
}

Expand Down Expand Up @@ -900,7 +904,7 @@ func TestWithConfig_validate(t *testing.T) {
name: "empty",
parent: &corev1.ConfigMap{},
reconciler: &WithConfig{},
shouldErr: "Config must be defined",
shouldErr: `WithConfig "" must define Config`,
},
{
name: "valid",
Expand All @@ -916,19 +920,21 @@ func TestWithConfig_validate(t *testing.T) {
name: "missing config",
parent: &corev1.ConfigMap{},
reconciler: &WithConfig{
Name: "missing config",
Reconciler: &Sequence{},
},
shouldErr: "Config must be defined",
shouldErr: `WithConfig "missing config" must define Config`,
},
{
name: "missing reconciler",
parent: &corev1.ConfigMap{},
reconciler: &WithConfig{
Name: "missing reconciler",
Config: func(ctx context.Context, c Config) (Config, error) {
return config, nil
},
},
shouldErr: "Reconciler must be defined",
shouldErr: `WithConfig "missing reconciler" must define Reconciler`,
},
}

Expand All @@ -954,7 +960,7 @@ func TestWithFinalizer_validate(t *testing.T) {
name: "empty",
parent: &corev1.ConfigMap{},
reconciler: &WithFinalizer{},
shouldErr: "Finalizer must be defined",
shouldErr: `WithFinalizer "" must define Finalizer`,
},
{
name: "valid",
Expand All @@ -968,17 +974,19 @@ func TestWithFinalizer_validate(t *testing.T) {
name: "missing finalizer",
parent: &corev1.ConfigMap{},
reconciler: &WithFinalizer{
Name: "missing finalizer",
Reconciler: &Sequence{},
},
shouldErr: "Finalizer must be defined",
shouldErr: `WithFinalizer "missing finalizer" must define Finalizer`,
},
{
name: "missing reconciler",
parent: &corev1.ConfigMap{},
reconciler: &WithFinalizer{
Name: "missing reconciler",
Finalizer: "my-finalizer",
},
shouldErr: "Reconciler must be defined",
shouldErr: `WithFinalizer "missing reconciler" must define Reconciler`,
},
}

Expand Down

0 comments on commit 11d1639

Please sign in to comment.