From b888d941cb1cec03b1756f3b3a56ba50f6ae2d1f Mon Sep 17 00:00:00 2001
From: Justin Kulikauskas <44813129+JustinKuli@users.noreply.github.com>
Date: Sat, 4 May 2024 19:03:28 +0000
Subject: [PATCH] Improve some documentation

Signed-off-by: Justin Kulikauskas <44813129+JustinKuli@users.noreply.github.com>
---
 api/v1alpha1/clientObjectFakes_test.go        | 10 +++---
 api/v1alpha1/reflectiveResourceList.go        |  7 +++-
 api/v1beta1/policycore_types.go               |  5 +++
 api/v1beta1/target.go                         | 34 ++++++++++++++++---
 .../controllers/fakepolicy_controller.go      |  3 ++
 5 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/api/v1alpha1/clientObjectFakes_test.go b/api/v1alpha1/clientObjectFakes_test.go
index 2bcef26..55e9c82 100644
--- a/api/v1alpha1/clientObjectFakes_test.go
+++ b/api/v1alpha1/clientObjectFakes_test.go
@@ -8,10 +8,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// fakeObjList implements client.ObjectList
+// fakeObjList minimally implements client.ObjectList. It is only to be used for tests.
 type fakeObjList string
 
-var _ client.ObjectList = fakeObjList("")
+// ensure fakeObjList implements client.ObjectList
+var _ client.ObjectList = (*fakeObjList)(nil)
 
 func (l fakeObjList) GetResourceVersion() string {
 	return string(l)
@@ -49,10 +50,11 @@ func (l fakeObjList) DeepCopyObject() runtime.Object {
 	return l
 }
 
-// fakeObjList implements client.Object
+// fakeObjList minimally implements client.Object. It is only to be used for tests.
 type fakeObj string
 
-var _ client.Object = fakeObj("")
+// ensure fakeObj implements client.Object
+var _ client.Object = (*fakeObj)(nil)
 
 func (o fakeObj) GetNamespace() string {
 	return string(o)
diff --git a/api/v1alpha1/reflectiveResourceList.go b/api/v1alpha1/reflectiveResourceList.go
index 1058613..a524c92 100644
--- a/api/v1alpha1/reflectiveResourceList.go
+++ b/api/v1alpha1/reflectiveResourceList.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"reflect"
 
+	"open-cluster-management.io/governance-policy-nucleus/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -18,8 +19,12 @@ type ReflectiveResourceList struct {
 	ClientList client.ObjectList
 }
 
+// ensure ReflectiveResourceList implements ResourceList
+var _ v1beta1.ResourceList = (*ReflectiveResourceList)(nil)
+
 // Items returns the list of items in the list. Since this implementation uses reflection, it may
-// have errors or not perform as well as a bespoke implementation for the underlying type.
+// have errors or not perform as well as a bespoke implementation for the underlying type. The
+// returned Objects are in the same order that they are in the list.
 func (l *ReflectiveResourceList) Items() ([]client.Object, error) {
 	value := reflect.ValueOf(l.ClientList)
 	if value.Kind() == reflect.Pointer {
diff --git a/api/v1beta1/policycore_types.go b/api/v1beta1/policycore_types.go
index 8a9cee7..5c0e47e 100644
--- a/api/v1beta1/policycore_types.go
+++ b/api/v1beta1/policycore_types.go
@@ -104,6 +104,8 @@ func (sel NamespaceSelector) MarshalJSON() ([]byte, error) {
 // namespaces that match the NamespaceSelector. The client.Reader needs access
 // for viewing namespaces, like the access given by this kubebuilder tag:
 // `//+kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch`
+//
+// NOTE: unlike Target, an empty NamespaceSelector will match zero namespaces
 func (sel NamespaceSelector) GetNamespaces(ctx context.Context, r client.Reader) ([]string, error) {
 	if len(sel.Include) == 0 && sel.LabelSelector == nil {
 		// A somewhat special case of no matches.
@@ -133,6 +135,9 @@ type namespaceResList struct {
 	corev1.NamespaceList
 }
 
+// ensure namespaceResList implements ResourceList
+var _ ResourceList = (*namespaceResList)(nil)
+
 func (l *namespaceResList) Items() ([]client.Object, error) {
 	items := make([]client.Object, len(l.NamespaceList.Items))
 	for i := range l.NamespaceList.Items {
diff --git a/api/v1beta1/target.go b/api/v1beta1/target.go
index a88a27a..eb0265b 100644
--- a/api/v1beta1/target.go
+++ b/api/v1beta1/target.go
@@ -30,7 +30,28 @@ type Target struct {
 //+kubebuilder:object:generate=false
 
 // ResourceList is meant to wrap a concrete implementation of a client.ObjectList, giving access
-// to the items in the list. The methods should be implemented on pointer types.
+// to the items in the list. The methods should be implemented on pointer types. For example, an
+// implementation of this interface for ConfigMaps might look like:
+//
+//	import corev1 "k8s.io/api/core/v1"
+//	import "sigs.k8s.io/controller-runtime/pkg/client"
+//
+//	type configMapResList struct {
+//		corev1.ConfigMapList
+//	}
+//
+//	func (l *configMapResList) Items() ([]client.Object, error) {
+//		items := make([]client.Object, len(l.ConfigMapList.Items))
+//		for i := range l.ConfigMapList.Items {
+//			items[i] = &l.ConfigMapList.Items[i]
+//		}
+//
+//		return items, nil
+//	}
+//
+//	func (l *configMapResList) ObjectList() client.ObjectList {
+//		return &l.ConfigMapList
+//	}
 type ResourceList interface {
 	ObjectList() client.ObjectList
 	Items() ([]client.Object, error)
@@ -39,11 +60,13 @@ type ResourceList interface {
 // GetMatches returns a list of resources on the cluster, matched by the Target. The provided
 // ResourceList should be backed by a client.ObjectList type which must registered in the scheme of
 // the client.Reader. The items in the provided ResourceList after this method is called will not
-// necessarily equal the items matched by the Target.
+// necessarily equal the items matched by the Target. The items returned here will be in relatively
+// the same order as they were in the list returned by the API.
 //
 // This method should be used preferentially to `GetMatchesDynamic` because it can leverage the
-// Reader's cache. NOTE: unlike the NamespaceSelector, an empty Target will match *all* resources on
-// the cluster.
+// Reader's cache.
+//
+// NOTE: unlike the NamespaceSelector, an empty Target will match *all* resources on the cluster.
 func (t Target) GetMatches(ctx context.Context, r client.Reader, list ResourceList) ([]client.Object, error) {
 	nonNilSel := t.LabelSelector
 	if nonNilSel == nil { // override it to be empty if it is nil
@@ -76,7 +99,8 @@ func (t Target) GetMatches(ctx context.Context, r client.Reader, list ResourceLi
 // the resources is configured by the provided dynamic.ResourceInterface. If the Target specifies a
 // namespace, this method will limit the namespace of the provided Interface if possible. If the
 // provided Interface is already namespaced, the namespace of the Interface will be used (possibly
-// overriding the namespace specified in the Target).
+// overriding the namespace specified in the Target). The items returned here will be in relatively
+// the same order as they were in the list returned by the API.
 //
 // NOTE: unlike the NamespaceSelector, an empty Target will match *all* resources on the cluster.
 func (t Target) GetMatchesDynamic(
diff --git a/test/fakepolicy/controllers/fakepolicy_controller.go b/test/fakepolicy/controllers/fakepolicy_controller.go
index 783e783..9574ec2 100644
--- a/test/fakepolicy/controllers/fakepolicy_controller.go
+++ b/test/fakepolicy/controllers/fakepolicy_controller.go
@@ -119,6 +119,9 @@ type configMapResList struct {
 	corev1.ConfigMapList
 }
 
+// ensure configMapResList implements ResourceList
+var _ nucleusv1beta1.ResourceList = (*configMapResList)(nil)
+
 func (l *configMapResList) Items() ([]client.Object, error) {
 	items := make([]client.Object, len(l.ConfigMapList.Items))
 	for i := range l.ConfigMapList.Items {