-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,12 +30,15 @@ var createExample = ` | |||||||||
# Create a broker 'mybroker' in the current namespace | ||||||||||
kn broker create mybroker | ||||||||||
|
||||||||||
# Create a broker 'mybroker' in the 'myproject' namespace | ||||||||||
kn broker create mybroker --namespace myproject` | ||||||||||
# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka' | ||||||||||
kn broker create mybroker --namespace myproject --broker Kafka | ||||||||||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's just There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. +1 to merge it. |
||||||||||
` | ||||||||||
|
||||||||||
// NewBrokerCreateCommand represents command to create new broker instance | ||||||||||
func NewBrokerCreateCommand(p *commands.KnParams) *cobra.Command { | ||||||||||
|
||||||||||
var className string | ||||||||||
vyasgun marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
cmd := &cobra.Command{ | ||||||||||
Use: "create NAME", | ||||||||||
Short: "Create a broker", | ||||||||||
|
@@ -58,7 +61,8 @@ func NewBrokerCreateCommand(p *commands.KnParams) *cobra.Command { | |||||||||
|
||||||||||
brokerBuilder := clientv1beta1. | ||||||||||
NewBrokerBuilder(name). | ||||||||||
Namespace(namespace) | ||||||||||
Namespace(namespace). | ||||||||||
Class(className) | ||||||||||
|
||||||||||
err = eventingClient.CreateBroker(cmd.Context(), brokerBuilder.Build()) | ||||||||||
if err != nil { | ||||||||||
|
@@ -71,5 +75,6 @@ func NewBrokerCreateCommand(p *commands.KnParams) *cobra.Command { | |||||||||
}, | ||||||||||
} | ||||||||||
commands.AddNamespaceFlags(cmd.Flags(), false) | ||||||||||
cmd.Flags().StringVar(&className, "class", "", "Broker class like 'MTChannelBasedBroker' or 'Kafka' (if available)") | ||||||||||
vyasgun marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
return cmd | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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. |
||
verifyBrokerList(r, "foo5") | ||
verifyBrokerListOutputName(r, "foo5") | ||
verifyBrokerDescribeContains(r, "foo5", "foo-class") | ||
} | ||
|
||
// Private functions | ||
|
@@ -86,3 +92,9 @@ func verifyBrokerNotfound(r *test.KnRunResultCollector, name string) { | |
r.AssertError(out) | ||
assert.Check(r.T(), util.ContainsAll(out.Stderr, name, "not found")) | ||
} | ||
|
||
func verifyBrokerDescribeContains(r *test.KnRunResultCollector, name, str string) { | ||
out := r.KnTest().Kn().Run("broker", "describe", name) | ||
r.AssertNoError(out) | ||
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, name, str)) | ||
} |
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.