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

[release-1.2] [1.3] Fix or remove some TODOs
This is an automated cherry-pick of #2186

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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/2188#" title="Author self-approved">knative-prow-robot</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **pierdipi** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @pierdipi` in a comment when ready.

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).

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

- **[OWNERS](https://github.com/knative-sandbox/eventing-kafka-broker/blob/release-1.2/OWNERS)**

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":["pierdipi"]} -->
knative-prow[bot]: @knative-prow-robot: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit-tests_eventing-kafka-broker_release-1.2 | 340b15d1dcde8e76dad9ffbcfc0b0a1b199a6c43 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka-broker/2188/unit-tests_eventing-kafka-broker_release-1.2/1524397475627012096) | true | `/test unit-tests_eventing-kafka-broker_release-1.2`

[Your PR dashboard](https://prow.knative.dev/pr).

<details>

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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
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/2188#pullrequestreview-969550854" title="Approved">aliok</a>*, *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/2188#" title="Author self-approved">knative-prow-robot</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.2/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: /retest

```
[ERROR] Failed to execute goal on project core: Could not resolve dependencies for project dev.knative.eventing.kafka.broker:core:jar:1.0-SNAPSHOT: The following artifacts could not be resolved: dev.knative.eventing.kafka.broker:contract:jar:1.0-SNAPSHOT, org.apache.kafka:kafka-clients:jar:2.8.1, com.github.luben:zstd-jni:jar:1.4.9-1, org.lz4:lz4-java:jar:1.7.1, org.xerial.snappy:snappy-java:jar:1.1.8.1, io.vertx:vertx-kafka-client:jar:4.2.4: Could not find artifact dev.knative.eventing.kafka.broker:contract:jar:1.0-SNAPSHOT in sonatype (https://oss.sonatype.org/content/repositories/snapshots/) -> [Help 1]
```

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 a888eee34..ec3c090cf 100644
--- a/control-plane/pkg/reconciler/channel/channel_test.go
+++ b/control-plane/pkg/reconciler/channel/channel_test.go
@@ -72,23 +72,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,
 					),
@@ -267,7 +258,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						StatusProbeFailed(prober.StatusNotReady),
 					),
 				},
@@ -338,7 +328,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						StatusProbeFailed(prober.StatusUnknown),
 					),
 				},
@@ -409,7 +398,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						WithSubscribers(Subscriber1(WithFreshSubscriber, WithNoSubscriberURI)),
 					),
 				},
@@ -487,7 +475,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -562,7 +549,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -637,7 +623,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -721,7 +706,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1(), Subscriber2()),
 						StatusProbeSucceeded,
@@ -994,7 +978,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						StatusProbeSucceeded,
 					),
@@ -1078,7 +1061,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						WithSubscribers(Subscriber1()),
 						StatusProbeSucceeded,
@@ -1148,7 +1130,6 @@ func TestReconcileKind(t *testing.T) {
 						StatusConfigMapUpdatedReady(&env),
 						StatusTopicReadyWithName(ChannelTopic()),
 						StatusDataPlaneAvailable,
-						//StatusInitialOffsetsCommitted,
 						ChannelAddressable(&env),
 						StatusProbeSucceeded,
 					),
@@ -1215,7 +1196,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 {