-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding class option to broker create #1402
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vyasgun: 0 warnings.
In response to this:
Description
Adding --class option to broker create
Changes
- Added the flag to broker create command
- Added unit test
- Added e2e test
Reference
Fixes #1269
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
Codecov Report
@@ Coverage Diff @@
## main #1402 +/- ##
==========================================
+ Coverage 78.33% 78.35% +0.02%
==========================================
Files 160 160
Lines 8270 8279 +9
==========================================
+ Hits 6478 6487 +9
Misses 1103 1103
Partials 689 689
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Thanks for contributions
@@ -32,6 +32,13 @@ func BrokerCreate(r *KnRunResultCollector, name string) { | |||
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "Broker", name, "created", "namespace", r.KnTest().Kn().Namespace())) | |||
} | |||
|
|||
// BrokerCreateWithClass creates a broker with the given name and class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is these not named CreateBroker…
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention in this file is BrokerCreate, BrokerDelete etc so I am just following that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention was already established in the file, not change by this PR. I propose to keep it as it and if we want to change it do it in a separate move.
@@ -59,6 +59,12 @@ func TestBroker(t *testing.T) { | |||
test.BrokerDelete(r, "foo4", true) | |||
verifyBrokerNotfound(r, "foo3") | |||
verifyBrokerNotfound(r, "foo4") | |||
|
|||
t.Log("create broker with class") | |||
test.BrokerCreateWithClass(r, "foo5", "foo-class") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there naming conventions for the class that we should be verifying… e.g., cannot start with numbers or symbols?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be no server side validation for this so I am not sure.
Please re-run |
The following is the coverage report on the affected files.
|
# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka' | ||
kn broker create mybroker --namespace myproject --broker Kafka |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka' | |
kn broker create mybroker --namespace myproject --broker Kafka | |
# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'KafkaBroker' | |
kn broker create mybroker --namespace myproject --broker KafkaBroker |
Sorry my bad again, I think it should be KafkaBroker (so with a trailing Broker suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am following the KNative Kafka Broker documentation and it is just Kafka as per it:
apiVersion: eventing.knative.dev/v1
kind: Broker
metadata:
annotations:
# case-sensitive
eventing.knative.dev/broker.class: Kafka
name: default
namespace: default
spec:
# Configuration specific to this broker.
config:
apiVersion: v1
kind: ConfigMap
name: kafka-broker-config
namespace: knative-eventing
# Optional dead letter sink, you can specify either:
# - deadLetterSink.ref, which is a reference to a Callable
# - deadLetterSink.uri, which is an absolute URI to a Callable (It can potentially be out of the Kubernetes cluster)
delivery:
deadLetterSink:
ref:
apiVersion: serving.knative.dev/v1
kind: Service
name: dlq-service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was referring to https://knative.dev/docs/eventing/broker/example-mtbroker/ where it used as KafkaBroker
. @matzew which is correct now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just Kafka
because I tried creating 2 brokers with the different classes and the Kafka controller only picked up when the annotation was set to Kafka
and not when it was set to KafkaBroker
but we can confirm this once before we decide to push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me open an issue over there at the docs repo, to find out and get confirmation from the experts ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just asked over there to clarify this knative/docs#4090 (would really prefer "Kafka" but then also "MTChannel" instead of "MTChannelBroker" ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhuss I think the current example message is correct. Any objections to proceed and merge this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to merge it.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: itsmurugappan, rhuss, vyasgun 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 |
Description
Adding --class option to broker create
Changes
Reference
Fixes #1269