Repository: knative-sandbox/eventing-kafka
Pull request #144 (2020-10-27 21:21:21)

Fix KafkaBinding conversion
## Proposed Changes

- Cherrypick https://github.com/knative/eventing-contrib/pull/1651
- No fuzzing tests like KafkaSource. Created https://github.com/knative-sandbox/eventing-kafka/issues/145 for that
-

<!--
If this change has user-visible impact, follow the instructions below.
Examples include:

- 🎁 Add new feature
- 🐛 Fix bug
- 🧽 Update or clean up current behavior
- 🗑️ Remove feature or internal logic

Otherwise delete the rest of this template.
-->

**Release Note**

<!--
🗒️ If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->

```release-note
- 🐛 Fix bug
Fix KafkaBinding conversion
```

**Docs**

<!--
📖 If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.
-->


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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka/pull/144#" title="Author self-approved">aliok</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **vaikas** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @vaikas` 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).

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

- **[OWNERS](https://github.com/knative-sandbox/eventing-kafka/blob/master/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":["vaikas"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=h1) Report
> Merging [#144](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=desc) into [master](https://codecov.io/gh/knative-sandbox/eventing-kafka/commit/a4deee1c39c7e4be572ab8a3b34e93784b39f0b4?el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144/graphs/tree.svg?width=650&height=150&src=pr&token=W765CT93QC)](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   75.23%   75.26%   +0.03%     
==========================================
  Files         111      111              
  Lines        3993     3991       -2     
==========================================
  Hits         3004     3004              
+ Misses        807      805       -2     
  Partials      182      182              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/bindings/v1alpha1/kafka\_conversion.go](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144/diff?src=pr&el=tree#diff-cGtnL2FwaXMvYmluZGluZ3MvdjFhbHBoYTEva2Fma2FfY29udmVyc2lvbi5nbw==) | `90.00% <100.00%> (-0.25%)` | :arrow_down: |
| [...tributed/common/kafka/admin/eventhubcache/cache.go](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144/diff?src=pr&el=tree#diff-cGtnL2NoYW5uZWwvZGlzdHJpYnV0ZWQvY29tbW9uL2thZmthL2FkbWluL2V2ZW50aHViY2FjaGUvY2FjaGUuZ28=) | `67.46% <0.00%> (+2.40%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=footer). Last update [a4deee1...fd9382f](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/144?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-eventing-kafka-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/apis/bindings/v1alpha1/kafka_conversion.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/144/pull-knative-sandbox-eventing-kafka-go-coverage/1321200615212191747/artifacts/line-cov.html#file0) | 86.7% | 85.7% | -1.0

lionelvillard: /lgtm
/approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka/pull/144#" title="Author self-approved">aliok</a>*, *<a href="https://github.com/knative-sandbox/eventing-kafka/pull/144#issuecomment-717559192" title="Approved">lionelvillard</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).

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/blob/master/OWNERS)~~ [lionelvillard]

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

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-tests | fd9382f6ab02a7be10a90e6456ea1a9288bf1d51 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/144/pull-knative-sandbox-eventing-kafka-integration-tests/1321200615212191745) | `/test pull-knative-sandbox-eventing-kafka-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/144). [Your PR dashboard](https://gubernator.knative.dev/pr/aliok).

<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 -->
aliok: /test pull-knative-sandbox-eventing-kafka-integration-tests

Maybe a flake? Event not seen.

Patch:
diff --git a/config/source/resources/kafkabinding.yaml b/config/source/resources/kafkabinding.yaml
index 3d12e01a2..5424d282e 100644
--- a/config/source/resources/kafkabinding.yaml
+++ b/config/source/resources/kafkabinding.yaml
@@ -22,6 +22,15 @@ metadata:
   name: kafkabindings.bindings.knative.dev
 spec:
   group: bindings.knative.dev
+  preserveUnknownFields: false
+  validation:
+    openAPIV3Schema:
+      type: object
+      # this is a work around so we don't need to flush out the
+      # schema for each version at this time
+      #
+      # see issue: https://github.com/knative/serving/issues/912
+      x-kubernetes-preserve-unknown-fields: true
   names:
     categories:
     - all
@@ -33,6 +42,12 @@ spec:
   scope: Namespaced
   subresources:
     status: {}
+  conversion:
+    strategy: Webhook
+    webhookClientConfig:
+      service:
+        name: kafka-source-webhook
+        namespace: knative-sources
   additionalPrinterColumns:
     - name: BootstrapServers
       type: string
diff --git a/pkg/apis/bindings/v1alpha1/kafka_conversion.go b/pkg/apis/bindings/v1alpha1/kafka_conversion.go
index 04128d523..9f4cf718d 100644
--- a/pkg/apis/bindings/v1alpha1/kafka_conversion.go
+++ b/pkg/apis/bindings/v1alpha1/kafka_conversion.go
@@ -39,8 +39,7 @@ func (source *KafkaBinding) ConvertTo(ctx context.Context, obj apis.Convertible)
 			BindingSpec:   source.Spec.BindingSpec,
 			KafkaAuthSpec: kafkaAuthSpec,
 		}
-		sink.Status.Status = source.Status.Status
-		source.Status.Status.ConvertTo(ctx, &sink.Status.Status)
+		source.Status.Status.DeepCopyInto(&sink.Status.Status)
 		return nil
 	default:
 		return fmt.Errorf("Unknown conversion, got: %T", sink)
@@ -62,8 +61,7 @@ func (sink *KafkaBinding) ConvertFrom(ctx context.Context, obj apis.Convertible)
 			BindingSpec:   source.Spec.BindingSpec,
 			KafkaAuthSpec: kafkaAuthSpec,
 		}
-		sink.Status.Status = source.Status.Status
-		source.Status.Status.ConvertTo(ctx, &source.Status.Status)
+		source.Status.Status.DeepCopyInto(&sink.Status.Status)
 		return nil
 	default:
 		return fmt.Errorf("Unknown conversion, got: %T", source)


Test patch:
diff --git a/pkg/apis/bindings/v1alpha1/kafka_conversion_test.go b/pkg/apis/bindings/v1alpha1/kafka_conversion_test.go
index c614f5807..215435191 100644
--- a/pkg/apis/bindings/v1alpha1/kafka_conversion_test.go
+++ b/pkg/apis/bindings/v1alpha1/kafka_conversion_test.go
@@ -147,6 +147,9 @@ func TestKafkaBindingConversionRoundTripV1alpha1(t *testing.T) {
 					Conditions: duckv1.Conditions{{
 						Type:   "Ready",
 						Status: "True",
+					}, {
+						Type:   "DoesNotReallyExistButHereItIsForTestingStatusCopy",
+						Status: "False",
 					}},
 					Annotations: map[string]string{
 						"foo": "bar",
@@ -285,6 +288,9 @@ func TestKafkaBindingConversionRoundTripV1beta1(t *testing.T) {
 					Conditions: duckv1.Conditions{{
 						Type:   "Ready",
 						Status: "True",
+					}, {
+						Type:   "DoesNotReallyExistButHereItIsForTestingStatusCopy",
+						Status: "False",
 					}},
 					Annotations: map[string]string{
 						"foo": "bar",