Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconcile ET properly with no reference #7423

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions pkg/apis/eventing/v1beta1/eventtype_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,4 @@ func (et *EventType) SetDefaults(ctx context.Context) {
}

func (ets *EventTypeSpec) SetDefaults(ctx context.Context) {
if ets.Reference == nil && ets.Broker == "" {
ets.Broker = "default"
}
}
24 changes: 21 additions & 3 deletions pkg/apis/eventing/v1beta1/eventtype_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestEventTypeDefaults(t *testing.T) {
initial: EventType{},
expected: EventType{
Spec: EventTypeSpec{
Broker: "default",
Broker: "",
},
},
},
Expand All @@ -53,7 +53,7 @@ func TestEventTypeDefaults(t *testing.T) {
Spec: EventTypeSpec{
Type: "test-type",
Source: testSource,
Broker: "default",
Broker: "",
Schema: testSchema,
},
},
Expand All @@ -70,7 +70,25 @@ func TestEventTypeDefaults(t *testing.T) {
Spec: EventTypeSpec{
Type: "test-type",
Source: testSource,
Broker: "default",
Broker: "",
Schema: testSchema,
},
},
},
"broker set": {
initial: EventType{
Spec: EventTypeSpec{
Type: "test-type",
Source: testSource,
Schema: testSchema,
Broker: "my-broker",
},
},
expected: EventType{
Spec: EventTypeSpec{
Type: "test-type",
Source: testSource,
Broker: "my-broker",
Schema: testSchema,
},
},
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/eventing/v1beta3/eventtype_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ func (et *EventTypeStatus) MarkReferenceDoesNotExist() {
eventTypeCondSet.Manage(et).MarkFalse(EventTypeConditionReferenceExists, "ResourceDoesNotExist", "Resource in spec.reference does not exist")
}

func (et *EventTypeStatus) MarkReferenceExistsNotSet() {
eventTypeCondSet.Manage(et).MarkTrueWithReason(EventTypeConditionReferenceExists, "ReferenceNotSet", "No reference set on the event type")
}

func (et *EventTypeStatus) MarkReferenceExistsUnknown(reason, messageFormat string, messageA ...interface{}) {
eventTypeCondSet.Manage(et).MarkUnknown(EventTypeConditionReferenceExists, reason, messageFormat, messageA...)
}
4 changes: 2 additions & 2 deletions pkg/reconciler/eventtype/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import (
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"

eventtypeinformer "knative.dev/eventing/pkg/client/injection/informers/eventing/v1beta2/eventtype"
eventtypereconciler "knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1beta2/eventtype"
eventtypeinformer "knative.dev/eventing/pkg/client/injection/informers/eventing/v1beta3/eventtype"
eventtypereconciler "knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1beta3/eventtype"
)

// NewController initializes the controller and is called by the generated code
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/eventtype/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

// Fake injection informers
_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/broker/fake"
_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1beta2/eventtype/fake"
_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1beta3/eventtype/fake"
)

func TestNew(t *testing.T) {
Expand Down
21 changes: 17 additions & 4 deletions pkg/reconciler/eventtype/eventtype.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ import (
"go.uber.org/zap"
apierrs "k8s.io/apimachinery/pkg/api/errors"

"knative.dev/eventing/pkg/apis/eventing/v1beta2"
eventtypereconciler "knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1beta2/eventtype"
"knative.dev/eventing/pkg/apis/eventing/v1beta3"
eventtypereconciler "knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1beta3/eventtype"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/logging"
pkgreconciler "knative.dev/pkg/reconciler"
)
Expand All @@ -38,8 +39,16 @@ type Reconciler struct {
var _ eventtypereconciler.Interface = (*Reconciler)(nil)

// ReconcileKind implements Interface.ReconcileKind.
// 1. Verify the Reference exists.
func (r *Reconciler) ReconcileKind(ctx context.Context, et *v1beta2.EventType) pkgreconciler.Event {
// 1. Check if there is a reference
// a) if not, reconcile to true
// b) if yes, continue reconciling
// 2. Verify the Reference exists.
Comment on lines +42 to +45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like having this comment here. thx

func (r *Reconciler) ReconcileKind(ctx context.Context, et *v1beta3.EventType) pkgreconciler.Event {

if et.Spec.Reference == nil || isEmptyReference(et.Spec.Reference) {
et.Status.MarkReferenceExistsNotSet()
return nil
}

_, err := r.kReferenceResolver.Resolve(ctx, et.Spec.Reference, et)
if err != nil {
Expand All @@ -55,3 +64,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, et *v1beta2.EventType) p
et.Status.MarkReferenceExists()
return nil
}

func isEmptyReference(ref *duckv1.KReference) bool {
return ref.Name == "" && ref.Namespace == "" && ref.APIVersion == "" && ref.Kind == "" && ref.Group == "" && *ref.Address == ""
}
19 changes: 18 additions & 1 deletion pkg/reconciler/eventtype/eventtype_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
clientgotesting "k8s.io/client-go/testing"
fakeeventingclient "knative.dev/eventing/pkg/client/injection/client/fake"
"knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1beta2/eventtype"
"knative.dev/eventing/pkg/client/injection/reconciler/eventing/v1beta3/eventtype"
. "knative.dev/eventing/pkg/reconciler/testing/v1"
"knative.dev/pkg/apis"
"knative.dev/pkg/configmap"
Expand Down Expand Up @@ -99,6 +99,23 @@ func TestReconcile(t *testing.T) {
WantEvents: []string{
Eventf(corev1.EventTypeWarning, "InternalError failed to get object test-namespace/test-broker:", `brokers.eventing.knative.dev "test-broker" not found`),
},
}, {
Name: "No reference set",
Key: testKey,
Objects: []runtime.Object{
NewEventType(eventTypeName, testNS,
WithEventTypeType(eventTypeType),
WithEventTypeSource(eventTypeSource),
),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: NewEventType(eventTypeName, testNS,
WithInitEventTypeConditions,
WithEventTypeType(eventTypeType),
WithEventTypeSource(eventTypeSource),
WithEventTypeReferenceNotSet),
}},
WantErr: false,
}}

logger := logtesting.TestLogger(t)
Expand Down
32 changes: 18 additions & 14 deletions pkg/reconciler/testing/v1/eventtype.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,16 @@ import (
duckv1 "knative.dev/pkg/apis/duck/v1"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/eventing/pkg/apis/eventing/v1beta2"
"knative.dev/eventing/pkg/apis/eventing/v1beta3"
"knative.dev/pkg/apis"
)

// EventTypeOption enables further configuration of an EventType.
type EventTypeOption func(*v1beta2.EventType)
type EventTypeOption func(*v1beta3.EventType)

// NewEventType creates a EventType with EventTypeOptions.
func NewEventType(name, namespace string, o ...EventTypeOption) *v1beta2.EventType {
et := &v1beta2.EventType{
func NewEventType(name, namespace string, o ...EventTypeOption) *v1beta3.EventType {
et := &v1beta3.EventType{
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should land in new package testing/v1beta3. Perhaps the package was supposed to be introduced by v1beta3 PR. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the v1 used to use the "latest".

I think that is OK... but we can (separately) do different?

Any comments, @Cali0707, @dsimansk ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to update this either in this PR or a separate PR if we want, but this file seemed to be using v1beta2 so I had assumed it would be fine to bump to v1beta3 :)

ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: name,
Expand All @@ -46,59 +46,63 @@ func NewEventType(name, namespace string, o ...EventTypeOption) *v1beta2.EventTy
}

// WithInitEventTypeConditions initializes the EventType's conditions.
func WithInitEventTypeConditions(et *v1beta2.EventType) {
func WithInitEventTypeConditions(et *v1beta3.EventType) {
et.Status.InitializeConditions()
}

func WithEventTypeSource(source *apis.URL) EventTypeOption {
return func(et *v1beta2.EventType) {
return func(et *v1beta3.EventType) {
et.Spec.Source = source
}
}

func WithEventTypeType(t string) EventTypeOption {
return func(et *v1beta2.EventType) {
return func(et *v1beta3.EventType) {
et.Spec.Type = t
}
}

func WithEventTypeReference(ref *duckv1.KReference) EventTypeOption {
return func(et *v1beta2.EventType) {
return func(et *v1beta3.EventType) {
et.Spec.Reference = ref
}
}

func WithEventTypeDescription(description string) EventTypeOption {
return func(et *v1beta2.EventType) {
return func(et *v1beta3.EventType) {
et.Spec.Description = description
}
}

func WithEventTypeLabels(labels map[string]string) EventTypeOption {
return func(et *v1beta2.EventType) {
return func(et *v1beta3.EventType) {
et.ObjectMeta.Labels = labels
}
}

func WithEventTypeOwnerReference(ownerRef metav1.OwnerReference) EventTypeOption {
return func(et *v1beta2.EventType) {
return func(et *v1beta3.EventType) {
et.ObjectMeta.OwnerReferences = []metav1.OwnerReference{
ownerRef,
}
}
}

func WithEventTypeDeletionTimestamp(et *v1beta2.EventType) {
func WithEventTypeDeletionTimestamp(et *v1beta3.EventType) {
t := metav1.NewTime(time.Unix(1e9, 0))
et.ObjectMeta.SetDeletionTimestamp(&t)
}

// WithEventTypeResourceDoesNotExist calls .Status.MarkFilterFailed on the EventType.
func WithEventTypeResourceDoesNotExist(et *v1beta2.EventType) {
func WithEventTypeResourceDoesNotExist(et *v1beta3.EventType) {
et.Status.MarkReferenceDoesNotExist()
}

func WithEventTypeReferenceNotSet(et *v1beta3.EventType) {
et.Status.MarkReferenceExistsNotSet()
}

// WithEventTypeResourceExists calls .Status.MarkReferenceExists on the EventType.
func WithEventTypeResourceExists(et *v1beta2.EventType) {
func WithEventTypeResourceExists(et *v1beta3.EventType) {
et.Status.MarkReferenceExists()
}
8 changes: 4 additions & 4 deletions pkg/reconciler/testing/v1/listers.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ import (
"k8s.io/client-go/tools/cache"
eventingduck "knative.dev/eventing/pkg/apis/duck/v1"
eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1"
eventingv1beta2 "knative.dev/eventing/pkg/apis/eventing/v1beta2"
eventingv1beta3 "knative.dev/eventing/pkg/apis/eventing/v1beta3"
flowsv1 "knative.dev/eventing/pkg/apis/flows/v1"
messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1"
sourcesv1 "knative.dev/eventing/pkg/apis/sources/v1"
fakeeventingclientset "knative.dev/eventing/pkg/client/clientset/versioned/fake"
eventinglisters "knative.dev/eventing/pkg/client/listers/eventing/v1"
eventingv1beta2listers "knative.dev/eventing/pkg/client/listers/eventing/v1beta2"
eventingv1beta3listers "knative.dev/eventing/pkg/client/listers/eventing/v1beta3"
flowslisters "knative.dev/eventing/pkg/client/listers/flows/v1"
messaginglisters "knative.dev/eventing/pkg/client/listers/messaging/v1"
sourcelisters "knative.dev/eventing/pkg/client/listers/sources/v1"
Expand Down Expand Up @@ -108,8 +108,8 @@ func (l *Listers) GetAllObjects() []runtime.Object {
return all
}

func (l *Listers) GetEventTypeLister() eventingv1beta2listers.EventTypeLister {
return eventingv1beta2listers.NewEventTypeLister(l.indexerFor(&eventingv1beta2.EventType{}))
func (l *Listers) GetEventTypeLister() eventingv1beta3listers.EventTypeLister {
return eventingv1beta3listers.NewEventTypeLister(l.indexerFor(&eventingv1beta3.EventType{}))
}

func (l *Listers) GetPingSourceLister() sourcelisters.PingSourceLister {
Expand Down
Loading