Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Require ListOptions when SkipOwnerReference is true #493

Merged
merged 1 commit into from Mar 7, 2024
Merged

Require ListOptions when SkipOwnerReference is true #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion reconcilers/child.go
View file
Open in desktop
Expand Up @@ -121,13 +121,19 @@ type ChildReconciler[Type, ChildType client.Object, ChildListType client.ObjectL
MergeBeforeUpdate func(current, desired ChildType)

// ListOptions allows custom options to be use when listing potential child resources. Each
// resource retrieved as part of the listing is confirmed via OurChild.
// resource retrieved as part of the listing is confirmed via OurChild. There is a performance
// benefit to limiting the number of resource return for each List operation, however,
// excluding an actual child will orphan that resource.
//
// Defaults to filtering by the reconciled resource's namespace:
// []client.ListOption{
// client.InNamespace(resource.GetNamespace()),
// }
//
// ListOptions is required when a Finalizer is defined or SkipOwnerReference is true. An empty
// list is often sufficient although it may incur a performance penalty, especially when
// querying the API sever instead of an informer cache.
//
// +optional
ListOptions func(ctx context.Context, resource Type) []client.ListOption

Expand Down Expand Up @@ -229,6 +235,11 @@ func (r *ChildReconciler[T, CT, CLT]) validate(ctx context.Context) error {
return fmt.Errorf("ChildReconciler %q must implement OurChild since owner references are not used", r.Name)
}

if r.ListOptions == nil && r.SkipOwnerReference {
// ListOptions is required when SkipOwnerReference is true
return fmt.Errorf("ChildReconciler %q must implement ListOptions since owner references are not used", r.Name)
}

// require MergeBeforeUpdate
if r.MergeBeforeUpdate == nil {
return fmt.Errorf("ChildReconciler %q must implement MergeBeforeUpdate", r.Name)
Expand Down
13 changes: 12 additions & 1 deletion reconcilers/childset.go
View file
Open in desktop
Expand Up @@ -116,13 +116,19 @@ type ChildSetReconciler[Type, ChildType client.Object, ChildListType client.Obje
MergeBeforeUpdate func(current, desired ChildType)

// ListOptions allows custom options to be use when listing potential child resources. Each
// resource retrieved as part of the listing is confirmed via OurChild.
// resource retrieved as part of the listing is confirmed via OurChild. There is a performance
// benefit to limiting the number of resource return for each List operation, however,
// excluding an actual child will orphan that resource.
//
// Defaults to filtering by the reconciled resource's namespace:
// []client.ListOption{
// client.InNamespace(resource.GetNamespace()),
// }
//
// ListOptions is required when a Finalizer is defined or SkipOwnerReference is true. An empty
// list is often sufficient although it may incur a performance penalty, especially when
// querying the API sever instead of an informer cache.
//
// +optional
ListOptions func(ctx context.Context, resource Type) []client.ListOption

Expand Down Expand Up @@ -257,6 +263,11 @@ func (r *ChildSetReconciler[T, CT, CLT]) validate(ctx context.Context) error {
return fmt.Errorf("ChildSetReconciler %q must implement OurChild since owner references are not used", r.Name)
}

if r.ListOptions == nil && r.SkipOwnerReference {
// ListOptions is required when SkipOwnerReference is true
return fmt.Errorf("ChildSetReconciler %q must implement ListOptions since owner references are not used", r.Name)
}

// require IdentifyChild
if r.IdentifyChild == nil {
return fmt.Errorf("ChildSetReconciler %q must implement IdentifyChild", r.Name)
Expand Down
33 changes: 33 additions & 0 deletions reconcilers/validate_test.go
View file
Open in desktop
Expand Up @@ -463,6 +463,22 @@ func TestChildReconciler_validate(t *testing.T) {
ListOptions: func(ctx context.Context, parent *corev1.ConfigMap) []client.ListOption { return []client.ListOption{} },
},
},
{
name: "ListOptions missing",
parent: &corev1.ConfigMap{},
reconciler: &ChildReconciler[*corev1.ConfigMap, *corev1.Pod, *corev1.PodList]{
Name: "ListOptions missing",
ChildType: &corev1.Pod{},
ChildListType: &corev1.PodList{},
DesiredChild: func(ctx context.Context, parent *corev1.ConfigMap) (*corev1.Pod, error) { return nil, nil },
ReflectChildStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, child *corev1.Pod, err error) {},
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SkipOwnerReference: true,
// ListOptions: func(ctx context.Context, parent *corev1.ConfigMap) []client.ListOption { return []client.ListOption{} },
OurChild: func(resource *corev1.ConfigMap, child *corev1.Pod) bool { return true },
},
shouldErr: `ChildReconciler "ListOptions missing" must implement ListOptions since owner references are not used`,
},
{
name: "Finalizer without OurChild",
parent: &corev1.ConfigMap{},
Expand Down Expand Up @@ -622,6 +638,23 @@ func TestChildSetReconciler_validate(t *testing.T) {
IdentifyChild: func(child *corev1.Pod) string { return "" },
},
},
{
name: "ListOptions missing",
parent: &corev1.ConfigMap{},
reconciler: &ChildSetReconciler[*corev1.ConfigMap, *corev1.Pod, *corev1.PodList]{
Name: "ListOptions missing",
ChildType: &corev1.Pod{},
ChildListType: &corev1.PodList{},
DesiredChildren: func(ctx context.Context, parent *corev1.ConfigMap) ([]*corev1.Pod, error) { return nil, nil },
ReflectChildrenStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, result ChildSetResult[*corev1.Pod]) {},
MergeBeforeUpdate: func(current, desired *corev1.Pod) {},
SkipOwnerReference: true,
// ListOptions: func(ctx context.Context, parent *corev1.ConfigMap) []client.ListOption { return []client.ListOption{} },
OurChild: func(resource *corev1.ConfigMap, child *corev1.Pod) bool { return true },
IdentifyChild: func(child *corev1.Pod) string { return "" },
},
shouldErr: `ChildSetReconciler "ListOptions missing" must implement ListOptions since owner references are not used`,
},
{
name: "Finalizer without OurChild",
parent: &corev1.ConfigMap{},
Expand Down