Review feedback

- Default parent resourceVersion to "999"
- avoid strategic merge
- readme polish

README.md

@@ -125,7 +125,7 @@ The implementor is responsible for:
 
 When a finalizer is defined, the parent resource is patched to add the finalizer before creating the child; it is removed after the child is deleted. If the parent resource is pending deletion, the desired child method is not called, and existing children are deleted.
 
-> Warning: It is crucial that each ChildReconciler that uses a finalizer have a unique and stable finalizer name. Two reconcilers that use the same finalizer, or a reconciler that changed the name of its finalizer may leak child resource when the parent is deleted, or the parent resource may never terminate.
+> Warning: It is crucial that each ChildReconciler using a finalizer have a unique and stable finalizer name. Two reconcilers that use the same finalizer, or a reconciler that changed the name of its finalizer, may leak the child resource when the parent is deleted, or the parent resource may never terminate.
 
 **Example:**
 
@@ -509,13 +509,43 @@ type MyResource struct {
 
 ### Finalizers
 
-[Finalizers](https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/) allow a reconciler to clean up state for a resource that has been deleted by a client, and not yet fully removed. Resources with finalizers will stay in this terminating state until all finalizers are cleared from the resource. While using the [Kubernetes garbage collector](https://kubernetes.io/docs/concepts/architecture/garbage-collection/) is recommended when possible, finalizer are useful for cases when state exists outside of the same cluster, scope, and namespace of the parent resource that needs to be cleaned up when no longer used.
+[Finalizers](https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/) allow a reconciler to clean up state for a resource that has been deleted by a client, and not yet fully removed. Terminating resources have `.metadata.deletionTimestamp` set. Resources with finalizers will stay in this terminating state until all finalizers are cleared from the resource. While using the [Kubernetes garbage collector](https://kubernetes.io/docs/concepts/architecture/garbage-collection/) is recommended when possible, finalizer are useful for cases when state exists outside of the same cluster, scope, and namespace of the parent resource that needs to be cleaned up when no longer used.
 
 Deleting a resource that uses finalizers requires the controller to be running.
 
-The [AddParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#AddParentFinalizer) and [ClearParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#ClearParentFinalizer) functions patch the parent resource to update its finalizers. These method work with [CastParents](#castparent) resources and use the same client the [ParentReconciler](#parentreconciler) used to originally load the parent resource. They can be called inside [SubReconcilers](#subreconciler) that may use a different client.
+The [AddParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#AddParentFinalizer) and [ClearParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#ClearParentFinalizer) functions patch the parent resource to update its finalizers. These methods work with [CastParents](#castparent) resources and use the same client the [ParentReconciler](#parentreconciler) used to originally load the parent resource. They can be called inside [SubReconcilers](#subreconciler) that may use a different client.
 
-When an update is required, only the `.metadata.finalizers` field is patched. The `.metadata.resourceVersion` of the parent is updated based on the value returned from the server. An error from the server will cause the resource reconciliation to err.
+When an update is required, only the `.metadata.finalizers` field is patched. The parent's `.metadata.resourceVersion` is used as an optimistic concurrency lock, and is updated with the value returned from the server. Any error from the server will cause the resource reconciliation to err. When testing with the [SubReconcilerTestSuite](#subreconcilertestsuite), the resource version of the parent defaults to `"999"`, the patch bytes include the resource version and the response increments the parent's resource version. For a parent with the default resource version that patches a finalizer, the expected parent will have a resource version of `"1000"`.
+
+A minimal test case for a sub reconciler that adds a finalizer may look like:
+
+```go
+	...
+	{
+		Name: "add 'test.finalizer' finalizer",
+		Parent: resourceDie,
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resourceDie, scheme, corev1.EventTypeNormal, "FinalizerPatched",
+				`Patched finalizer %q`, "test.finalizer"),
+		},
+		ExpectParent: resourceDie.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.Finalizers("test.finalizer")
+				d.ResourceVersion("1000")
+			}),
+		ExpectPatches: []rtesting.PatchRef{
+			{
+				Group:     "testing.reconciler.runtime",
+				Kind:      "TestResource",
+				Namespace: resourceDie.GetNamespace(),
+				Name:      resourceDie.GetName(),
+				PatchType: types.MergePatchType,
+				Patch:     []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`),
+			},
+		},
+	},
+	...
+```
 
 ## Contributing
 

reconcilers/reconcilers.go

@@ -1217,7 +1217,7 @@ func ensureParentFinalizer(ctx context.Context, parent client.Object, finalizer
 					controllerutil.RemoveFinalizer(desired, finalizer)
 				}
 
-				patch := client.StrategicMergeFrom(current, client.MergeFromWithOptimisticLock{})
+				patch := client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})
 				if err := config.Patch(ctx, desired, patch); err != nil {
 					log.Error(err, "unable to patch parent finalizers", "finalizer", finalizer)
 					config.Recorder.Eventf(current, corev1.EventTypeWarning, "FinalizerPatchFailed",

reconcilers/reconcilers_test.go

@@ -550,7 +550,6 @@ func TestChildReconciler(t *testing.T) {
 			d.Namespace(testNamespace)
 			d.Name(testName)
 			d.CreationTimestamp(metav1.NewTime(time.UnixMilli(1000)))
-			d.ResourceVersion("999")
 		}).
 		StatusDie(func(d *dies.TestResourceStatusDie) {
 			d.ConditionsDie(
@@ -692,7 +691,7 @@ func TestChildReconciler(t *testing.T) {
 		},
 		ExpectParent: resourceReady.
 			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
-				d.Finalizers("test.finalizer")
+				d.Finalizers(testFinalizer)
 				d.ResourceVersion("1000")
 			}).
 			SpecDie(func(d *dies.TestResourceSpecDie) {
@@ -710,7 +709,7 @@ func TestChildReconciler(t *testing.T) {
 				Kind:      "TestResource",
 				Namespace: testNamespace,
 				Name:      testName,
-				PatchType: types.StrategicMergePatchType,
+				PatchType: types.MergePatchType,
 				Patch:     []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`),
 			},
 		},
@@ -841,7 +840,7 @@ func TestChildReconciler(t *testing.T) {
 				Kind:      "TestResource",
 				Namespace: testNamespace,
 				Name:      testName,
-				PatchType: types.StrategicMergePatchType,
+				PatchType: types.MergePatchType,
 				Patch:     []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`),
 			},
 		},
@@ -899,7 +898,7 @@ func TestChildReconciler(t *testing.T) {
 				Kind:      "TestResource",
 				Namespace: testNamespace,
 				Name:      testName,
-				PatchType: types.StrategicMergePatchType,
+				PatchType: types.MergePatchType,
 				Patch:     []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`),
 			},
 		},
@@ -1175,7 +1174,7 @@ func TestChildReconciler(t *testing.T) {
 				Kind:      "TestResource",
 				Namespace: testNamespace,
 				Name:      testName,
-				PatchType: types.StrategicMergePatchType,
+				PatchType: types.MergePatchType,
 				Patch:     []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`),
 			},
 		},
@@ -1214,7 +1213,7 @@ func TestChildReconciler(t *testing.T) {
 				Kind:      "TestResource",
 				Namespace: testNamespace,
 				Name:      testName,
-				PatchType: types.StrategicMergePatchType,
+				PatchType: types.MergePatchType,
 				Patch:     []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`),
 			},
 		},

testing/subreconciler.go

@@ -164,6 +164,10 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto
 
 	ctx = reconcilers.StashParentConfig(ctx, c)
 	parent := tc.Parent.DeepCopyObject().(client.Object)
+	if parent.GetResourceVersion() == "" {
+		// this value is also set by the test client when resource are added as givens
+		parent.SetResourceVersion("999")
+	}
 	ctx = reconcilers.StashParentType(ctx, parent.DeepCopyObject().(client.Object))
 	ctx = reconcilers.StashCastParentType(ctx, parent.DeepCopyObject().(client.Object))
 
@@ -199,6 +203,10 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto
 	if tc.ExpectParent != nil {
 		expectedParent = tc.ExpectParent.DeepCopyObject().(client.Object)
 	}
+	if expectedParent.GetResourceVersion() == "" {
+		// mirror defaulting of the parent
+		expectedParent.SetResourceVersion("999")
+	}
 	if diff := cmp.Diff(expectedParent, parent, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, cmpopts.EquateEmpty()); diff != "" {
 		t.Errorf("Unexpected parent mutations(-expected, +actual): %s", diff)
 	}