Repository: knative-sandbox/eventing-kafka
Pull request #376 (2021-02-10 14:58:34)

Allow v1beta1 stkafkasource topics & bootstrapservers to be mutable
Change the validation of the kafkasource api to allow for changing the
topics/servers after creation

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/376#" title="Author self-approved">lberk</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)~~ [lberk]

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":[]} -->
matzew: /lgtm
I am fine for just doing this for `v1beta1`.

/hold


/assign @lionelvillard for another set of review 
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/sources/v1beta1/kafka_validation.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/376/pull-knative-sandbox-eventing-kafka-go-coverage/1359517129015037952/artifacts/line-cov.html#file0) | 85.7% | 87.5% | 1.8

codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/376?src=pr&el=h1) Report
> Merging [#376](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/376?src=pr&el=desc) (b7c86b3) into [master](https://codecov.io/gh/knative-sandbox/eventing-kafka/commit/80b4e4e884b33c338b8be0ec57297721f6974827?el=desc) (80b4e4e) will **increase** coverage by `0.00%`.
> The diff coverage is `50.00%`.

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

```diff
@@           Coverage Diff           @@
##           master     #376   +/-   ##
=======================================
  Coverage   75.26%   75.27%           
=======================================
  Files         124      124           
  Lines        4791     4792    +1     
=======================================
+ Hits         3606     3607    +1     
  Misses        962      962           
  Partials      223      223           
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/376?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/sources/v1beta1/kafka\_validation.go](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/376/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc291cmNlcy92MWJldGExL2thZmthX3ZhbGlkYXRpb24uZ28=) | `57.14% <50.00%> (+3.29%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/376?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/376?src=pr&el=footer). Last update [80b4e4e...bcd1914](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/376?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lionelvillard: /lgtm

It'd be nice to have e2e tests stressing this new feature (a follow up PR is fine)
Can you add a release note? (even though there is no release notes, maybe time to change this bad behavior?)
The doc should be updated as well.


I let @lberk unhold it.
lberk: Thanks @lionelvillard

I've got a follow-up for a couple other fields that'll (hopefully)
make our validation a bit more consistent with the norms of other
sources (instead of just blanket immutability for all the fields).

I'll add a release note, e2e tests, docs with the follow up PR so we can be
sure to encompass all the changes.

/unhold

Patch:
diff --git a/pkg/apis/sources/v1beta1/kafka_validation.go b/pkg/apis/sources/v1beta1/kafka_validation.go
index d7270b6a5..573fd321b 100644
--- a/pkg/apis/sources/v1beta1/kafka_validation.go
+++ b/pkg/apis/sources/v1beta1/kafka_validation.go
@@ -19,6 +19,8 @@ package v1beta1
 import (
 	"context"
 
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/kmp"
 )
@@ -27,7 +29,9 @@ import (
 func (r *KafkaSource) Validate(ctx context.Context) *apis.FieldError {
 	if apis.IsInUpdate(ctx) {
 		original := apis.GetBaseline(ctx).(*KafkaSource)
-		if diff, err := kmp.ShortDiff(original.Spec, r.Spec); err != nil {
+		// Ignore diffs in bootstrapServers and topics
+		ignoreArgs := cmpopts.IgnoreFields(KafkaSourceSpec{}, "Topics", "BootstrapServers")
+		if diff, err := kmp.ShortDiff(original.Spec, r.Spec, ignoreArgs); err != nil {
 			return &apis.FieldError{
 				Message: "Failed to diff KafkaSource",
 				Paths:   []string{"spec"},


Test patch:
diff --git a/pkg/apis/sources/v1beta1/kafka_validation_test.go b/pkg/apis/sources/v1beta1/kafka_validation_test.go
index 66f6c17dd..1c47cd5d7 100644
--- a/pkg/apis/sources/v1beta1/kafka_validation_test.go
+++ b/pkg/apis/sources/v1beta1/kafka_validation_test.go
@@ -57,10 +57,11 @@ func TestKafkaSourceCheckImmutableFields(t *testing.T) {
 		"Topic changed": {
 			orig: &fullSpec,
 			updated: KafkaSourceSpec{
-				Topics:     []string{"some-other-topic"},
-				SourceSpec: fullSpec.SourceSpec,
+				Topics:        []string{"some-other-topic"},
+				ConsumerGroup: fullSpec.ConsumerGroup,
+				SourceSpec:    fullSpec.SourceSpec,
 			},
-			allowed: false,
+			allowed: true,
 		},
 		"Bootstrap servers changed": {
 			orig: &fullSpec,
@@ -68,9 +69,10 @@ func TestKafkaSourceCheckImmutableFields(t *testing.T) {
 				KafkaAuthSpec: bindingsv1beta1.KafkaAuthSpec{
 					BootstrapServers: []string{"server1,server2"},
 				},
-				SourceSpec: fullSpec.SourceSpec,
+				ConsumerGroup: fullSpec.ConsumerGroup,
+				SourceSpec:    fullSpec.SourceSpec,
 			},
-			allowed: false,
+			allowed: true,
 		},
 		"Sink.APIVersion changed": {
 			orig: &fullSpec,