Address comments

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>

go.mod

@@ -5,7 +5,6 @@ go 1.20
 require (
 	dario.cat/mergo v1.0.0
 	github.com/bufbuild/buf v1.26.1
-	github.com/evanphx/json-patch/v5 v5.6.0
 	github.com/go-logr/logr v1.2.4
 	github.com/google/go-cmp v0.5.9
 	github.com/spf13/afero v1.9.5
@@ -40,6 +39,7 @@ require (
 	github.com/docker/go-connections v0.4.0 // indirect
 	github.com/docker/go-units v0.5.0 // indirect
 	github.com/emicklei/go-restful/v3 v3.10.2 // indirect
+	github.com/evanphx/json-patch/v5 v5.6.0 // indirect
 	github.com/fatih/color v1.15.0 // indirect
 	github.com/felixge/fgprof v0.9.3 // indirect
 	github.com/fsnotify/fsnotify v1.6.0 // indirect

pkg/logging/logging.go

@@ -37,6 +37,8 @@ limitations under the License.
 package logging
 
 import (
+	"fmt"
+
 	"github.com/go-logr/logr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -98,6 +100,9 @@ func (l logrLogger) WithValues(keysAndValues ...any) Logger {
 func ForResource(object client.Object) []string {
 	ret := make([]string, 0, 10)
 	gvk := object.GetObjectKind().GroupVersionKind()
+	if gvk.Kind == "" {
+		gvk.Kind = fmt.Sprintf("%T", object) // best effort for native Go types
+	}
 	ret = append(ret,
 		"name", object.GetName(),
 		"kind", gvk.Kind,

pkg/resource/api.go

@@ -32,24 +32,30 @@ import (
 // Error strings.
 const (
 	errUpdateObject = "cannot update object"
+
+	// taken from k8s.io/apiserver. Not crucial to match, but for uniformity it
+	// better should.
+	// TODO(sttts): import from k8s.io/apiserver/pkg/registry/generic/registry when
+	//              kube has updated otel dependencies post-1.28.
+	errOptimisticLock = "the object has been modified; please apply your changes to the latest version and try again"
 )
 
 // An APIPatchingApplicator applies changes to an object by either creating or
 // patching it in a Kubernetes API server.
 type APIPatchingApplicator struct {
-	client      client.Client
-	optionalLog logging.Logger // can be nil
+	client client.Client
+	log    logging.Logger
 }
 
 // NewAPIPatchingApplicator returns an Applicator that applies changes to an
 // object by either creating or patching it in a Kubernetes API server.
 func NewAPIPatchingApplicator(c client.Client) *APIPatchingApplicator {
-	return &APIPatchingApplicator{client: c}
+	return &APIPatchingApplicator{client: c, log: logging.NewNopLogger()}
 }
 
 // WithLogger sets the logger on the APIPatchingApplicator.
 func (a *APIPatchingApplicator) WithLogger(l logging.Logger) *APIPatchingApplicator {
-	a.optionalLog = l
+	a.log = l
 	return a
 }
 
@@ -58,17 +64,19 @@ func (a *APIPatchingApplicator) WithLogger(l logging.Logger) *APIPatchingApplica
 // patched if the passed object has the same or an empty resource version.
 func (a *APIPatchingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error { //nolint:gocyclo // the logic here is crucial and deserves to stay in one method
 	if obj.GetName() == "" && obj.GetGenerateName() != "" {
-		return errors.Wrap(a.client.Create(ctx, obj), "cannot create object")
+		log := a.log.WithValues(logging.ForResource(obj))
+		log.Info("creating object")
+		return a.client.Create(ctx, obj)
 	}
 
 	current := obj.DeepCopyObject().(client.Object)
 	err := a.client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, current)
 	if kerrors.IsNotFound(err) {
 		// TODO(negz): Apply ApplyOptions here too?
-		return errors.Wrap(a.client.Create(ctx, obj), "cannot create object")
+		return a.client.Create(ctx, obj)
 	}
 	if err != nil {
-		return errors.Wrap(err, "cannot get object")
+		return err
 	}
 
 	// Note: this check would ideally not be necessary if the Apply signature
@@ -80,21 +88,25 @@ func (a *APIPatchingApplicator) Apply(ctx context.Context, obj client.Object, ao
 		if err != nil {
 			return err
 		}
-		return kerrors.NewConflict(gvr, current.GetName(), errors.New("resource version does not match"))
+		return kerrors.NewConflict(gvr, current.GetName(), errors.New(errOptimisticLock))
 	}
 
 	for _, fn := range ao {
 		if err := fn(ctx, current, obj); err != nil {
-			return err
+			return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj))
 		}
 	}
 
-	if err := LogDiff(a.optionalLog, current, obj); err != nil {
-		return err
+	// log diff
+	patch := client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})
+	patchBytes, err := patch.Data(obj)
+	if err != nil {
+		return errors.Wrapf(err, "failed to diff %s", HumanReadableReference(a.client, obj))
 	}
+	log := a.log.WithValues(logging.ForResource(obj))
+	log.WithValues("diff", string(patchBytes)).Info("patching object")
 
-	// TODO(negz): Allow callers to override the kind of patch used.
-	return errors.Wrap(a.client.Patch(ctx, obj, client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})), "cannot patch object")
+	return a.client.Patch(ctx, obj, client.RawPatch(patch.Type(), patchBytes))
 }
 
 func groupResource(c client.Client, o client.Object) (schema.GroupResource, error) {
@@ -112,8 +124,8 @@ func groupResource(c client.Client, o client.Object) (schema.GroupResource, erro
 // An APIUpdatingApplicator applies changes to an object by either creating or
 // updating it in a Kubernetes API server.
 type APIUpdatingApplicator struct {
-	client      client.Client
-	optionalLog logging.Logger // can be nil
+	client client.Client
+	log    logging.Logger
 }
 
 // NewAPIUpdatingApplicator returns an Applicator that applies changes to an
@@ -122,43 +134,50 @@ type APIUpdatingApplicator struct {
 // Deprecated: Use NewAPIPatchingApplicator instead. The updating applicator
 // can lead to data-loss if the Golang types in this process are not up-to-date.
 func NewAPIUpdatingApplicator(c client.Client) *APIUpdatingApplicator {
-	return &APIUpdatingApplicator{client: c}
+	return &APIUpdatingApplicator{client: c, log: logging.NewNopLogger()}
 }
 
 // WithLogger sets the logger on the APIUpdatingApplicator.
 func (a *APIUpdatingApplicator) WithLogger(l logging.Logger) *APIUpdatingApplicator {
-	a.optionalLog = l
+	a.log = l
 	return a
 }
 
 // Apply changes to the supplied object. The object will be created if it does
 // not exist, or updated if it does.
 func (a *APIUpdatingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error {
 	if obj.GetName() == "" && obj.GetGenerateName() != "" {
-		return errors.Wrap(a.client.Create(ctx, obj), "cannot create object")
+		log := a.log.WithValues(logging.ForResource(obj))
+		log.Info("creating object")
+		return a.client.Create(ctx, obj)
 	}
 
 	current := obj.DeepCopyObject().(client.Object)
 	err := a.client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, current)
 	if kerrors.IsNotFound(err) {
 		// TODO(negz): Apply ApplyOptions here too?
-		return errors.Wrap(a.client.Create(ctx, obj), "cannot create object")
+		return a.client.Create(ctx, obj)
 	}
 	if err != nil {
-		return errors.Wrap(err, "cannot get object")
+		return err
 	}
 
 	for _, fn := range ao {
 		if err := fn(ctx, current, obj); err != nil {
-			return err
+			return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj))
 		}
 	}
 
-	if err := LogDiff(a.optionalLog, current, obj); err != nil {
-		return err
+	// log diff
+	patch := client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})
+	patchBytes, err := patch.Data(obj)
+	if err != nil {
+		return errors.Wrapf(err, "failed to diff %s", HumanReadableReference(a.client, obj))
 	}
+	log := a.log.WithValues(logging.ForResource(obj))
+	log.WithValues("diff", string(patchBytes)).Info("updating object")
 
-	return errors.Wrap(a.client.Update(ctx, obj), "cannot update object")
+	return a.client.Update(ctx, obj)
 }
 
 // An APIFinalizer adds and removes finalizers to and from a resource.