Cherry pick custom kafka topics changes #729
Conversation
* Added kafka feature for broker and channel topic templates * Added tests for broker and channel features * Updated codegen
…ensions#3162) * Updated broker to use broker topic name template * Updated broker reconciliation to onlyuse topic name template if no existing topic name for broker * Fixed import styling * Updated channel to use topic template * Updated channels to use topic annotations * Fixed import formatting * Added tests for reconciling with custom topic templates * Fixed go imports * use annotated topic names when deleting topics * switched from failing tests to using panics if topic names not setup properly * updated kafka feature yaml files to match channel topic template * updated kafka feature default vars to be private * updated tests * Fixed goimports * Fixed broker finalizer tests * Updated unit tests * Fixed unit tests * Switched templates to not be pointers * Updated broker and channel controllers to watch kafka feature config maps Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>
|
/cc @pierDipi |
|
/hold until openshift/release#40854 is merged so that we can have CI run on this |
|
/remove-hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, pierDipi 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:
Approvers can indicate their approval by writing |
| dispatcher.rate-limiter: "disabled" | ||
| dispatcher.ordered-executor-metrics: "disabled" | ||
| controller.autoscaler: "disabled" | ||
| triggers.consumergroup.template: "knative-trigger-{{ .Namespace }}-{{ .Name }}" | ||
| brokers.topic.template: "knative-broker-{{ .Namespace }}-{{ .Name }}" | ||
| channels.topic.template: "knative-messaging-kafka.{{ .Namespace }}.{{ .Name }}" |
There was a problem hiding this comment.
Hi. Is there a reason why the format is different for brokers and channels? Just noticed that the .Namespace and .Name are separated by - for brokers and by . for channels.
The example at line 44 looks better to me (separation by - and using a shorter name knative-channel instead of knative-messaging-kafka.
There was a problem hiding this comment.
Yeah, this was to match the existing formats for channel and broker names so that the default behaviour would not change
This PR cherry picks the changes from knative-extensions#3157 and knative-extensions#3162
There were some interesting merge conflicts in the broker and channel unit tests because of the presence of TLS in the tests on the upstream repo, so it may be worth looking closely at those files to make sure everything is still correct (but the tests are passing locally for me now)