Consolidate EventsRecorder into APIRecorder and document annotation drop

The duplicate EventsRecorder type was identical to the updated APIRecorder;
consolidate to a single type. Document clearly that the events.k8s.io API
has no AnnotatedEventf equivalent, so annotations accumulated via
WithAnnotations are stored but cannot be forwarded to the API server.

Also fix the FilterFn godoc (returning true prevents recording), update
the stale godoc URL to point at the new events package, and tighten tests
to use a single mockKubeRecorder that satisfies events.EventRecorder.

Signed-off-by: Ross Golder <ross@golder.org>

Author: rossigee

pkg/event/event.go

@@ -27,8 +27,8 @@ import (
 // A Type of event.
 type Type string
 
-// Event types. See below for valid types.
-// https://godoc.org/k8s.io/client-go/tools/record#EventRecorder
+// Event types.
+// https://pkg.go.dev/k8s.io/client-go/tools/events#EventRecorder
 const (
 	TypeNormal  Type = "Normal"
 	TypeWarning Type = "Warning"
@@ -77,28 +77,26 @@ type Recorder interface {
 	WithAnnotations(keysAndValues ...string) Recorder
 }
 
+// FilterFn is a function used to filter events. Returning true prevents the
+// event from being recorded.
+type FilterFn func(obj runtime.Object, e Event) bool
+
 // An APIRecorder records Kubernetes events to an API server using the
-// events.k8s.io API.
+// events.k8s.io/v1 API introduced in Kubernetes 1.19.
+//
+// Note: the events.EventRecorder interface does not support per-event
+// annotations (unlike the deprecated record.EventRecorder.AnnotatedEventf).
+// Annotations accumulated via WithAnnotations are stored but cannot be
+// forwarded to the API server. Callers that relied on annotation propagation
+// should encode that metadata into the event message instead.
 type APIRecorder struct {
 	kube        events.EventRecorder
-	annotations map[string]string
+	annotations map[string]string // stored but not forwarded; see type doc.
 	filterFns   []FilterFn
 }
 
-// An EventsRecorder records Kubernetes events to an API server using the
-// new events.k8s.io API.
-type EventsRecorder struct {
-	kube        events.EventRecorder
-	annotations map[string]string
-	filterFns   []FilterFn
-}
-
-// FilterFn is a function used to filter events.
-// It should return true when events should not be sent.
-type FilterFn func(obj runtime.Object, e Event) bool
-
 // NewAPIRecorder returns an APIRecorder that records Kubernetes events to an
-// APIServer using the supplied EventRecorder.
+// API server using the supplied EventRecorder.
 func NewAPIRecorder(r events.EventRecorder, fns ...FilterFn) *APIRecorder {
 	return &APIRecorder{kube: r, annotations: map[string]string{}, filterFns: fns}
 }
@@ -115,7 +113,10 @@ func (r *APIRecorder) Event(obj runtime.Object, e Event) {
 }
 
 // WithAnnotations returns a new *APIRecorder that includes the supplied
-// annotations with all recorded events.
+// annotations. Note: the events.k8s.io API does not support per-event
+// annotations, so accumulated annotations are stored but not propagated to
+// the API server. Encode annotation data in the event message if it must
+// appear in the emitted event.
 func (r *APIRecorder) WithAnnotations(keysAndValues ...string) Recorder {
 	ar := NewAPIRecorder(r.kube, r.filterFns...)
 	maps.Copy(ar.annotations, r.annotations)
@@ -125,34 +126,6 @@ func (r *APIRecorder) WithAnnotations(keysAndValues ...string) Recorder {
 	return ar
 }
 
-// NewEventsRecorder returns an EventsRecorder that records Kubernetes events to an
-// APIServer using the new events.k8s.io API.
-func NewEventsRecorder(r events.EventRecorder, fns ...FilterFn) *EventsRecorder {
-	return &EventsRecorder{kube: r, annotations: map[string]string{}, filterFns: fns}
-}
-
-// Event records the supplied event using the new events API.
-func (r *EventsRecorder) Event(obj runtime.Object, e Event) {
-	for _, filter := range r.filterFns {
-		if filter(obj, e) {
-			return
-		}
-	}
-
-	r.kube.Eventf(obj, nil, string(e.Type), string(e.Reason), "", "%s", e.Message)
-}
-
-// WithAnnotations returns a new *EventsRecorder that includes the supplied
-// annotations with all recorded events.
-func (r *EventsRecorder) WithAnnotations(keysAndValues ...string) Recorder {
-	ar := NewEventsRecorder(r.kube, r.filterFns...)
-	maps.Copy(ar.annotations, r.annotations)
-
-	sliceMap(keysAndValues, ar.annotations)
-
-	return ar
-}
-
 func sliceMap(from []string, to map[string]string) {
 	for i := 0; i+1 < len(from); i += 2 {
 		k, v := from[i], from[i+1]

pkg/event/event_test.go

@@ -17,43 +17,30 @@ limitations under the License.
 package event
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
-type mockRecordRecorder struct {
+// mockKubeRecorder satisfies events.EventRecorder.
+type mockKubeRecorder struct {
 	events []mockEvent
 }
 
 type mockEvent struct {
 	obj     runtime.Object
-	annots  map[string]string
 	typeStr string
 	reason  string
 	msg     string
 }
 
-func (m *mockRecordRecorder) Event(obj runtime.Object, eventtype, reason, message string) {
-	m.events = append(m.events, mockEvent{obj: obj, typeStr: eventtype, reason: reason, msg: message})
-}
-
-func (m *mockRecordRecorder) Eventf(obj runtime.Object, related runtime.Object, eventtype, reason, action, note string, args ...interface{}) {
-	m.events = append(m.events, mockEvent{obj: obj, typeStr: eventtype, reason: reason, msg: args[0].(string)})
-}
-
-func (m *mockRecordRecorder) AnnotatedEventf(obj runtime.Object, annots map[string]string, typeStr, reason, msg string, args ...interface{}) {
-	m.events = append(m.events, mockEvent{obj: obj, annots: annots, typeStr: typeStr, reason: reason, msg: args[0].(string)})
-}
-
-type mockEventsRecorder struct {
-	events []mockEvent
-}
-
-func (m *mockEventsRecorder) Eventf(obj runtime.Object, related runtime.Object, eventtype, reason, action, note string, args ...interface{}) {
-	m.events = append(m.events, mockEvent{obj: obj, typeStr: eventtype, reason: reason, msg: args[0].(string)})
+func (m *mockKubeRecorder) Eventf(obj runtime.Object, _ runtime.Object, eventtype, reason, _, note string, args ...interface{}) {
+	msg := fmt.Sprintf(note, args...)
+	m.events = append(m.events, mockEvent{obj: obj, typeStr: eventtype, reason: reason, msg: msg})
 }
 
 type mockObj struct{}
@@ -128,73 +115,56 @@ func TestSliceMap(t *testing.T) {
 	}
 }
 
-func TestAPIRecorderWithAnnotationsFilterFns(t *testing.T) {
-	filterCalled := false
-	filter := func(obj runtime.Object, e Event) bool {
-		filterCalled = true
-		return false
+func TestAPIRecorderEvent(t *testing.T) {
+	mr := &mockKubeRecorder{}
+	rec := NewAPIRecorder(mr)
+
+	rec.Event(&mockObj{}, Normal("testReason", "test message"))
+
+	want := mockEvent{typeStr: "Normal", reason: "testReason", msg: "test message"}
+	if diff := cmp.Diff(want, mr.events[0], cmp.AllowUnexported(mockEvent{}), cmpopts.IgnoreFields(mockEvent{}, "obj")); diff != "" {
+		t.Errorf("unexpected event: -want, +got:\n%s", diff)
 	}
+}
 
-	mr := &mockRecordRecorder{}
+func TestAPIRecorderFilter(t *testing.T) {
+	mr := &mockKubeRecorder{}
+	filter := func(_ runtime.Object, _ Event) bool { return true }
 	rec := NewAPIRecorder(mr, filter)
-	_ = rec.WithAnnotations("key", "val")
 
-	rec.Event(&mockObj{}, Normal("test", "msg"))
+	rec.Event(&mockObj{}, Normal("testReason", "test message"))
 
-	if !filterCalled {
-		t.Error("filter function was not preserved after WithAnnotations")
+	if diff := cmp.Diff(0, len(mr.events)); diff != "" {
+		t.Errorf("expected no events, got %d: %s", len(mr.events), diff)
 	}
 }
 
-func TestEventsRecorderWithAnnotationsFilterFns(t *testing.T) {
+func TestAPIRecorderWithAnnotationsPreservesFilterFns(t *testing.T) {
 	filterCalled := false
-	filter := func(obj runtime.Object, e Event) bool {
+	filter := func(_ runtime.Object, _ Event) bool {
 		filterCalled = true
 		return false
 	}
 
-	mr := &mockEventsRecorder{}
-	rec := NewEventsRecorder(mr, filter)
-	_ = rec.WithAnnotations("key", "val")
-
-	rec.Event(&mockObj{}, Normal("test", "msg"))
-
-	if !filterCalled {
-		t.Error("filter function was not preserved after WithAnnotations")
-	}
-}
-
-func TestEventsRecorderEvent(t *testing.T) {
-	mr := &mockEventsRecorder{}
-	rec := NewEventsRecorder(mr)
-
-	rec.Event(&mockObj{}, Normal("testReason", "test message"))
+	mr := &mockKubeRecorder{}
+	rec := NewAPIRecorder(mr, filter)
+	derived := rec.WithAnnotations("key", "val")
 
-	if len(mr.events) != 1 {
-		t.Fatalf("expected 1 event, got %d", len(mr.events))
-	}
+	derived.Event(&mockObj{}, Normal("test", "msg"))
 
-	if mr.events[0].reason != "testReason" {
-		t.Errorf("expected reason 'testReason', got %q", mr.events[0].reason)
-	}
-	if mr.events[0].msg != "test message" {
-		t.Errorf("expected message 'test message', got %q", mr.events[0].msg)
-	}
-	if mr.events[0].typeStr != "Normal" {
-		t.Errorf("expected type 'Normal', got %q", mr.events[0].typeStr)
+	if diff := cmp.Diff(true, filterCalled); diff != "" {
+		t.Errorf("filter function was not preserved after WithAnnotations: %s", diff)
 	}
 }
 
-func TestEventsRecorderFilter(t *testing.T) {
-	mr := &mockEventsRecorder{}
-	filter := func(obj runtime.Object, e Event) bool {
-		return true
-	}
-	rec := NewEventsRecorder(mr, filter)
-
-	rec.Event(&mockObj{}, Normal("testReason", "test message"))
+func TestAPIRecorderWithAnnotationsPreservesExistingAnnotations(t *testing.T) {
+	mr := &mockKubeRecorder{}
+	rec := NewAPIRecorder(mr)
+	r1 := rec.WithAnnotations("k1", "v1").(*APIRecorder)
+	r2 := r1.WithAnnotations("k2", "v2").(*APIRecorder)
 
-	if len(mr.events) != 0 {
-		t.Errorf("expected event to be filtered, got %d events", len(mr.events))
+	want := map[string]string{"k1": "v1", "k2": "v2"}
+	if diff := cmp.Diff(want, r2.annotations); diff != "" {
+		t.Errorf("annotations not preserved correctly: -want, +got:\n%s", diff)
 	}
 }