search

knative / eventing-contrib

Event Sources
Apache License 2.0
226 stars 225 forks source link

[0.17] Fix KafkaSource conversion #1650

Closed aliok closed 3 years ago

aliok commented 3 years ago

Fixes #1649

Proposed Changes

That conversion had 2 problems:

Conversion tests had roundtrip test cases but they didn't fail. Now with 2 conditions the previous code makes the tests fail.

Release Note

- 🐛 Fix bug
Fix KafkaSource conversion which resulted in reconcilation churn
aliok commented 3 years ago

this is to be ported into 0.18 of eventing-contrib and also into master of knative-sandbox/eventing-kafka

knative-metrics-robot commented 3 years ago

The following is the coverage report on the affected files. Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
kafka/source/pkg/apis/sources/v1alpha1/kafka_conversion.go 94.1% 93.8% -0.4
vaikas commented 3 years ago

/lgtm /approve

Should probably cherrypick to .17, .18, etc.?

knative-prow-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, vaikas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[kafka/OWNERS](https://github.com/knative/eventing-contrib/blob/release-0.17/kafka/OWNERS)~~ [vaikas] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
aliok commented 3 years ago

/hold

aliok commented 3 years ago

Should probably cherrypick to .17, .18, etc.?

this is against 0.17 :) prefer having against master in knative-sandbox/eventing-kafka first?

feel free to unhold if this PR is just OK

aliok commented 3 years ago

cc @vaikas

vaikas commented 3 years ago

for the head I'd like us to use fuzzers because they catch so many things :) /unhold

vaikas commented 3 years ago

Of course it's against .17 :) should cp for .18, yes?

aliok commented 3 years ago

should cp for .18, yes?

yep

aliok commented 3 years ago

This is already fixed on 0.18: https://github.com/knative/eventing-contrib/pull/1645 and on master: https://github.com/knative-sandbox/eventing-kafka/pull/128

No cherry pick needed