fix tests and clean up some codes.

Author: yuwenma

applylib/applyset/applyset.go

@@ -94,7 +94,9 @@ func New(options Options) (*ApplySet, error) {
 	parent := options.Parent
 	parentRef := &kubectlapply.ApplySetParentRef{Name: parent.Name(), Namespace: parent.Namespace(), RESTMapping: parent.RESTMapping()}
 	kapplyset := kubectlapply.NewApplySet(parentRef, kubectlapply.ApplySetTooling{Name: options.Tooling}, options.RESTMapper)
-	options.PatchOptions.FieldManager = kapplyset.FieldManager()
+	if options.PatchOptions.FieldManager == "" {
+		options.PatchOptions.FieldManager = kapplyset.FieldManager()
+	}
 	a := &ApplySet{
 		parentClient:  options.ParentClient,
 		client:        options.Client,
@@ -176,7 +178,7 @@ func (a *ApplySet) ApplyOnce(ctx context.Context) (*ApplyResults, error) {
 		}
 	}
 	if err := a.WithParent(ctx, kapplyset); err != nil {
-		return results, fmt.Errorf("unable to update Parent %v", err)
+		return results, fmt.Errorf("unable to update Parent: %w", err)
 	}
 
 	for i := range trackers.items {
@@ -198,11 +200,11 @@ func (a *ApplySet) ApplyOnce(ctx context.Context) (*ApplyResults, error) {
 		if restMapping == nil {
 			// Should be impossible
 			results.applyError(gvk, nn, fmt.Errorf("rest mapping result not found for %v", gvk))
+			continue
 		}
 
 		if err := a.updateManifestLabel(obj, kapplyset.LabelsForMember()); err != nil {
-			klog.Errorf("unable to update label for %v/%v %v: %v", obj.GetName(), obj.GetNamespace(), gvk, err)
-			continue
+			return results, fmt.Errorf("unable to update label for %v/%v %v: %w", obj.GetName(), obj.GetNamespace(), gvk, err)
 		}
 
 		gvr := restMapping.Resource
@@ -252,40 +254,23 @@ func (a *ApplySet) ApplyOnce(ctx context.Context) (*ApplyResults, error) {
 	// We want to be more cautions on pruning and only do it if all manifests are applied.
 	if a.prune && results.applyFailCount == 0 {
 		klog.V(4).Infof("Prune is enabled")
-		err := func() error {
-			pruneObjects, err := kapplyset.FindAllObjectsToPrune(ctx, a.client, visitedUids)
-			if err != nil {
-				return err
-			}
-			if err = a.deleteObjects(ctx, pruneObjects, results); err != nil {
-				return err
-			}
-			return nil
-		}()
+		pruneObjects, err := kapplyset.FindAllObjectsToPrune(ctx, a.client, visitedUids)
 		if err != nil {
-			klog.Errorf("encounter error on pruning %v", err)
-			if e := a.updateParentLabelsAndAnnotations(ctx, kapplyset, "superset"); e != nil {
-				return results, e
-			}
+			return results, err
+		}
+		if err = a.deleteObjects(ctx, pruneObjects, results); err != nil {
+			return results, err
+		}
+		// "latest" mode updates the parent "applyset.kubernetes.io/contains-group-resources" annotations
+		// to only contain the current manifest GVRs.
+		if err := a.updateParentLabelsAndAnnotations(ctx, kapplyset, "latest"); err != nil {
+			klog.Errorf("update parent failed %v", err)
 		}
-	}
-	if err := a.updateParentLabelsAndAnnotations(ctx, kapplyset, "latest"); err != nil {
-		klog.Errorf("update parent failed %v", err)
 	}
 	return results, nil
 }
 
-func mergeMap(from, to map[string]string) map[string]string {
-	if to == nil {
-		to = make(map[string]string)
-	}
-	for k, v := range from {
-		to[k] = v
-	}
-	return to
-}
-
-// updateLabel adds the "applyset.kubernetes.io/part-of: Parent-ID" label to the manifest.
+// updateManifestLabel adds the "applyset.kubernetes.io/part-of: Parent-ID" label to the manifest.
 func (a *ApplySet) updateManifestLabel(obj ApplyableObject, applysetLabels map[string]string) error {
 	u, ok := obj.(*unstructured.Unstructured)
 	if !ok {
@@ -296,14 +281,14 @@ func (a *ApplySet) updateManifestLabel(obj ApplyableObject, applysetLabels map[s
 	if labels == nil {
 		labels = make(map[string]string)
 	}
-	newLabels := mergeMap(applysetLabels, labels)
-	u.SetLabels(newLabels)
+	for k, v := range applysetLabels {
+		labels[k] = v
+	}
+	u.SetLabels(labels)
 	return nil
 }
 
 // updateParentLabelsAndAnnotations updates the parent labels and annotations.
-// This method leverages the kubectlapply to build the parent labels and annotations, but avoid using the
-// `resource.NewHelper` and cmdutil to patch the parent.
 func (a *ApplySet) updateParentLabelsAndAnnotations(ctx context.Context, kapplyset *kubectlapply.ApplySet, mode kubectlapply.ApplySetUpdateMode) error {
 	parent, err := meta.Accessor(a.parent.GetSubject())
 	if err != nil {
@@ -315,26 +300,46 @@ func (a *ApplySet) updateParentLabelsAndAnnotations(ctx context.Context, kapplys
 		return err
 	}
 	partialParent := kapplyset.BuildParentPatch(mode)
-	newAnnotations := mergeMap(partialParent.Annotations, parent.GetAnnotations())
-	parent.SetAnnotations(newAnnotations)
-	newLabels := mergeMap(partialParent.Labels, parent.GetLabels())
-	parent.SetLabels(newLabels)
 
-	return a.updateParent(ctx, original, parent)
+	// update annotation
+	annotations := parent.GetAnnotations()
+	if annotations == nil {
+		annotations = make(map[string]string)
+	}
+	for k, v := range partialParent.Annotations {
+		annotations[k] = v
+	}
+	parent.SetAnnotations(annotations)
+
+	// update labels
+	labels := parent.GetLabels()
+	if labels == nil {
+		labels = make(map[string]string)
+	}
+	for k, v := range partialParent.Labels {
+		labels[k] = v
+	}
+	parent.SetLabels(labels)
+
+	// update parent in the cluster.
+	if !reflect.DeepEqual(original.GetLabels(), parent.GetLabels()) || !reflect.DeepEqual(original.GetAnnotations(), parent.GetAnnotations()) {
+		if err := a.parentClient.Update(ctx, parent.(client.Object)); err != nil {
+			return fmt.Errorf("error updating parent %w", err)
+		}
+	}
+	return nil
 }
 
 func (a *ApplySet) deleteObjects(ctx context.Context, pruneObjects []kubectlapply.PruneObject, results *ApplyResults) error {
 	for i := range pruneObjects {
 		pruneObject := &pruneObjects[i]
-
 		name := pruneObject.Name
 		namespace := pruneObject.Namespace
 		mapping := pruneObject.Mapping
 		gvk := pruneObject.Object.GetObjectKind().GroupVersionKind()
 		nn := types.NamespacedName{Namespace: namespace, Name: name}
 
 		if err := a.client.Resource(mapping.Resource).Namespace(namespace).Delete(ctx, name, a.deleteOptions); err != nil {
-			klog.Error("unable to delete resource ")
 			results.pruneError(gvk, nn, fmt.Errorf("error from delete: %w", err))
 		} else {
 			klog.Infof("pruned resource %v", pruneObject.String())
@@ -344,16 +349,16 @@ func (a *ApplySet) deleteObjects(ctx context.Context, pruneObjects []kubectlappl
 	return nil
 }
 
+// WithParent guarantees the parent has the right applyset labels.
+// It uses "superset" mode to determine the "applyset.kubernetes.io/contains-group-resources" which contains both
+//
+//	previous manifests GVRs and the current manifests GVRs.
 func (a *ApplySet) WithParent(ctx context.Context, kapplyset *kubectlapply.ApplySet) error {
 	parent := a.parent.GetSubject()
 	object, err := meta.Accessor(parent)
 	if err != nil {
 		return err
 	}
-	original, err := meta.Accessor(parent.DeepCopyObject())
-	if err != nil {
-		return err
-	}
 	//kubectlapply requires the tooling and id to exist.
 	{
 		annotations := object.GetAnnotations()
@@ -378,15 +383,5 @@ func (a *ApplySet) WithParent(ctx context.Context, kapplyset *kubectlapply.Apply
 		return err
 	}
 
-	return a.updateParent(ctx, original, object)
-}
-
-func (a *ApplySet) updateParent(ctx context.Context, original, current metav1.Object) error {
-	if !reflect.DeepEqual(original.GetLabels(), current.GetLabels()) || !reflect.DeepEqual(original.GetAnnotations(), current.GetAnnotations()) {
-		if err := a.parentClient.Update(ctx, current.(client.Object)); err != nil {
-			return fmt.Errorf("error updating parent %v", err)
-		}
-	}
-	klog.V(4).Infof("parent labels and annotations are not changed, skip updating")
-	return nil
+	return a.updateParentLabelsAndAnnotations(ctx, kapplyset, "superset")
 }

applylib/applyset/applyset_test.go

@@ -23,7 +23,6 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/applylib/testutils"
 	"sigs.k8s.io/yaml"
@@ -32,7 +31,14 @@ import (
 func TestApplySet(t *testing.T) {
 	h := testutils.NewHarness(t)
 
-	existing := ``
+	// The parent should exist in the cluster
+	existing := `
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+  namespace: default
+`
 
 	apply := `
 apiVersion: v1
@@ -61,16 +67,17 @@ data:
 
 	force := true
 	patchOptions.Force = &force
-
-	parentGVK := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"}
+	parent := h.ParseObjects(existing)[0]
+	parentGVK := parent.GroupVersionKind()
 	restmapping, err := h.RESTMapper().RESTMapping(parentGVK.GroupKind(), parentGVK.Version)
 	if err != nil {
 		h.Fatalf("error building parent restmappaing: %v", err)
 	}
 	s, err := New(Options{
-		Parent:        NewParentRef(parentGVK, "test", "default", restmapping),
+		Parent:        NewParentRef(parent, "test", "default", restmapping),
 		RESTMapper:    h.RESTMapper(),
 		Client:        h.DynamicClient(),
+		ParentClient:  h.Client(),
 		PatchOptions:  patchOptions,
 		DeleteOptions: metav1.DeleteOptions{},
 		Prune:         true,

applylib/applyset/testdata/testapplyset/expected.yaml

@@ -2,7 +2,7 @@ apiVersion: v1
 kind: Namespace
 metadata:
   labels:
-    applyset.kubernetes.io/part-of: applyset-k3TRa7vXhJ2Gw-i5Jd_xbW5nDsiGbtAt-ocL_pzzFEE-v1
+    applyset.kubernetes.io/part-of: applyset-XYWvxXDUlCqMdjmmY1arThcdGiF0cvBW6sAfSMWYUdE-v1
     kubernetes.io/metadata.name: test-applyset
   name: test-applyset
 spec:
@@ -18,6 +18,6 @@ data:
 kind: ConfigMap
 metadata:
   labels:
-    applyset.kubernetes.io/part-of: applyset-k3TRa7vXhJ2Gw-i5Jd_xbW5nDsiGbtAt-ocL_pzzFEE-v1
+    applyset.kubernetes.io/part-of: applyset-XYWvxXDUlCqMdjmmY1arThcdGiF0cvBW6sAfSMWYUdE-v1
   name: foo
   namespace: test-applyset

pkg/patterns/declarative/metrics_test.go

@@ -29,21 +29,32 @@ import (
 	core "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/rest"
 	"k8s.io/klog/v2"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/applylib/applyset"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/applier"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
-	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/restmapper"
 	"sigs.k8s.io/yaml"
 )
 
+func fakeParent() runtime.Object {
+	parent := &unstructured.Unstructured{}
+	parent.SetKind("ConfigMap")
+	parent.SetAPIVersion("v1")
+	parent.SetName("test")
+	parent.SetNamespace("default")
+	return parent
+}
+
 // This test checks gvkString function
 func TestGVKString(t *testing.T) {
 	testCases := []struct {
@@ -263,11 +274,6 @@ func TestAddIfNotPresent(t *testing.T) {
 		Host: addr.String(),
 	}
 
-	restMapper, err := restmapper.NewControllerRESTMapper(restConfig)
-	if err != nil {
-		t.Fatalf("error getting rest mapper: %v", err)
-	}
-
 	// Create manager
 	mgrOpt := manager.Options{
 		MetricsBindAddress: "0", // Don't open unneeded ports
@@ -527,16 +533,19 @@ func TestAddIfNotPresent(t *testing.T) {
 					var options applier.ApplierOptions
 					options.Namespace = st.defaultNamespace
 					options.Objects = objList
-					options.RESTMapper = restMapper
+					options.RESTMapper = mgr.GetRESTMapper()
 					options.RESTConfig = restConfig
-					gvk := schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"}
+					parent := fakeParent()
+					options.Client = fake.NewClientBuilder().WithRuntimeObjects(parent).Build()
+					gvk := parent.GetObjectKind().GroupVersionKind()
 					restmapping, err := options.RESTMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 					if err != nil {
 						t.Fatalf("failed to get restmapping for parent: %v", err)
 					}
-					options.ParentRef = applyset.NewParentRef(gvk, "kdp-test", "default", restmapping)
+					options.ParentRef = applyset.NewParentRef(parent, "kdp-test", "default", restmapping)
 					applier := applier.NewApplySetApplier(
 						metav1.PatchOptions{FieldManager: "kdp-test"}, metav1.DeleteOptions{}, applier.ApplysetOptions{})
+
 					if err := applier.Apply(ctx, options); err != nil {
 						t.Fatalf("failed to apply objects: %v", err)
 					}
@@ -546,7 +555,7 @@ func TestAddIfNotPresent(t *testing.T) {
 						t.Fatalf("error parsing manifest: %v", err)
 					}
 
-					if err := deleteObjects(ctx, restConfig, restMapper, st.defaultNamespace, objects); err != nil {
+					if err := deleteObjects(ctx, restConfig, mgr.GetRESTMapper(), st.defaultNamespace, objects); err != nil {
 						t.Fatalf("error deleting objects: %v", err)
 					}
 				} else {

pkg/patterns/declarative/pkg/applier/applylib.go

@@ -125,11 +125,11 @@ func (a *ApplySetApplier) Apply(ctx context.Context, opt ApplierOptions) error {
 }
 
 // NewParentRef maps a declarative object's information to the ParentRef defined in the applyset library.
-func NewParentRef(restMapper meta.RESTMapper, object runtime.Object, name, namespace string) applyset.Parent {
+func NewParentRef(restMapper meta.RESTMapper, object runtime.Object, name, namespace string) (applyset.Parent, error) {
 	gvk := object.GetObjectKind().GroupVersionKind()
 	restMapping, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
 	if err != nil {
-		return nil
+		return nil, err
 	}
-	return applyset.NewParentRef(object, name, namespace, restMapping)
+	return applyset.NewParentRef(object, name, namespace, restMapping), nil
 }