Repository: knative-sandbox/eventing-kafka-broker
Pull request #2186 (2022-05-11 13:34:24)

[1.3] Fix or remove some TODOs
Backporting https://github.com/knative-sandbox/eventing-kafka-broker/pull/2052 manually

knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/2186#" title="Author self-approved">aliok</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Feventing-kafka-broker).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/eventing-kafka-broker/blob/release-1.3/OWNERS)~~ [aliok]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
aliok: /cherry-pick release-1.2
knative-prow-robot: @aliok: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

<details>

In response to [this](https://github.com/knative-sandbox/eventing-kafka-broker/pull/2186#issuecomment-1123771135):

>/cherry-pick release-1.2


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/2186?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> :exclamation: No coverage uploaded for pull request base (`release-1.3@319b846`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#section-missing-base-commit).
> The diff coverage is `n/a`.

```diff
@@              Coverage Diff               @@
##             release-1.3    #2186   +/-   ##
==============================================
  Coverage               ?   60.37%           
==============================================
  Files                  ?       69           
  Lines                  ?     4550           
  Branches               ?        0           
==============================================
  Hits                   ?     2747           
  Misses                 ?     1525           
  Partials               ?      278           
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/2186?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/2186?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [319b846...b0d6bfa](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/2186?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).


Patch:
diff --git a/control-plane/cmd/kafka-controller/main.go b/control-plane/cmd/kafka-controller/main.go
index 5447ae63e..5a9624445 100644
--- a/control-plane/cmd/kafka-controller/main.go
+++ b/control-plane/cmd/kafka-controller/main.go
@@ -83,7 +83,7 @@ func main() {
 		injection.NamedControllerConstructor{
 			Name: "channel-controller",
 			ControllerConstructor: func(ctx context.Context, watcher configmap.Watcher) *controller.Impl {
-				return channel.NewController(ctx, watcher, channelEnv)
+				return channel.NewController(ctx, channelEnv)
 			},
 		},
 
diff --git a/control-plane/pkg/reconciler/channel/controller.go b/control-plane/pkg/reconciler/channel/controller.go
index 881023a38..febb5bfc1 100644
--- a/control-plane/pkg/reconciler/channel/controller.go
+++ b/control-plane/pkg/reconciler/channel/controller.go
@@ -36,7 +36,6 @@ import (
 	podinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod"
 	secretinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/secret"
 
-	"knative.dev/pkg/configmap"
 	"knative.dev/pkg/controller"
 	"knative.dev/pkg/logging"
 	"knative.dev/pkg/network"
@@ -47,8 +46,7 @@ import (
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/base"
 )
 
-// TODO: no need for a configmap watcher?
-func NewController(ctx context.Context, watcher configmap.Watcher, configs *config.Env) *controller.Impl {
+func NewController(ctx context.Context, configs *config.Env) *controller.Impl {
 
 	messagingv1beta.RegisterAlternateKafkaChannelConditionSet(base.IngressConditionSet)
 


Test patch:
diff --git a/control-plane/pkg/reconciler/channel/channel_test.go b/control-plane/pkg/reconciler/channel/channel_test.go
index e60521a06..bb40ff545 100644
--- a/control-plane/pkg/reconciler/channel/channel_test.go
+++ b/control-plane/pkg/reconciler/channel/channel_test.go
@@ -74,23 +74,15 @@ var DefaultEnv = &config.Env{
 	DataPlaneConfigMapNamespace: "knative-eventing",
 	DataPlaneConfigMapName:      "kafka-channel-channels-subscriptions",
 	GeneralConfigMapName:        "kafka-channel-config",
-	IngressName:                 "kafka-channel-ingress",
+	IngressName:                 "kafka-channel-dispatcher",
 	SystemNamespace:             "knative-eventing",
 	DataPlaneConfigFormat:       base.Json,
 }
 
-// TODO: compare with inmemorychannel_test.go at /Users/aliok/code/github.com/knative/eventing/pkg/reconciler/inmemorychannel/controller/inmemorychannel_test.go
-// TODO: compare with channel_test.go at /Users/aliok/code/github.com/knative/eventing/pkg/reconciler/channel/channel_test.go
-// TODO: are we setting a InitialOffsetsCommitted status? we gotta set it on the subscription. 1) can we do it? 2) does it make sense?
-// TODO: test if things from channel spec is used properly?
-// TODO: test if things from subscription spec is used properly?
-// TODO: rename test cases consistently
-// TODO: test finalize?
-// TODO: fix the order of status updates
-// TODO: add tests where there are no CM updates
-
 func TestReconcileKind(t *testing.T) {
 
+	t.Setenv("SYSTEM_NAMESPACE", "knative-eventing")
+
 	messagingv1beta.RegisterAlternateKafkaChannelConditionSet(base.IngressConditionSet)
 
 	env := *DefaultEnv
@@ -198,7 +190,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						StatusProbeSucceeded,
 					),
@@ -345,7 +336,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						StatusProbeFailed(prober.StatusNotReady),
 					),
 				},
@@ -414,7 +404,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						StatusProbeFailed(prober.StatusUnknown),
 					),
 				},
@@ -483,7 +472,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						WithSubscribers(Subscriber1(WithFreshSubscriber, WithNoSubscriberURI)),
 					),
 				},
@@ -559,7 +547,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -632,7 +619,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -705,7 +691,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -787,7 +772,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1(), Subscriber2()),
 						StatusProbeSucceeded,
@@ -1058,7 +1042,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						StatusProbeSucceeded,
 					),
@@ -1141,7 +1124,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -1209,7 +1191,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						StatusProbeSucceeded,
 					),
@@ -1274,7 +1255,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						StatusProbeSucceeded,
 					),
diff --git a/control-plane/pkg/reconciler/channel/controller_test.go b/control-plane/pkg/reconciler/channel/controller_test.go
index cc3ca16ce..f7a2e0d08 100644
--- a/control-plane/pkg/reconciler/channel/controller_test.go
+++ b/control-plane/pkg/reconciler/channel/controller_test.go
@@ -29,7 +29,6 @@ import (
 	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake"
 	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake"
 	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/secret/fake"
-	"knative.dev/pkg/configmap"
 	dynamicclient "knative.dev/pkg/injection/clients/dynamicclient/fake"
 	reconcilertesting "knative.dev/pkg/reconciler/testing"
 
@@ -61,11 +60,6 @@ func TestNewController(t *testing.T) {
 
 	controller := NewController(
 		ctx,
-		configmap.NewStaticWatcher(&corev1.ConfigMap{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "cm",
-			},
-		}),
 		configs,
 	)
 	if controller == nil {