Skip to content
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

Merged
merged 3 commits into from
Aug 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
|===
| | Description | PR

| 🎁
| Adding --class flag to broker create command
| https://github.com/knative/client/pull/1402[#1402]

| 🎁
| Add an `client.knative.dev/updateTimestamp` annotation to trigger a new revision when required
| https://github.com/knative/client/pull/1364[#1364]
Expand Down
1 change: 1 addition & 0 deletions docs/cmd/kn_broker_create.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ kn broker create NAME
### Options

```
--class string Broker class like 'MTChannelBasedBroker' or 'Kafka' (if available)
-h, --help help for create
-n, --namespace string Specify the namespace to operate in.
```
Expand Down
7 changes: 7 additions & 0 deletions lib/test/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

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…?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

func BrokerCreateWithClass(r *KnRunResultCollector, name, class string) {
out := r.KnTest().Kn().Run("broker", "create", name, "--class", class)
r.AssertNoError(out)
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "Broker", name, "created", "namespace", r.KnTest().Kn().Namespace()))
}

// BrokerDelete deletes a broker with the given name.
func BrokerDelete(r *KnRunResultCollector, name string, wait bool) {
args := []string{"broker", "delete", name}
Expand Down
12 changes: 12 additions & 0 deletions pkg/eventing/v1/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,18 @@ func (b *BrokerBuilder) Namespace(ns string) *BrokerBuilder {
return b
}

// Class for broker builder
func (b *BrokerBuilder) Class(class string) *BrokerBuilder {
if class == "" {
return b
}
if len(b.broker.Annotations) == 0 {
b.broker.Annotations = make(map[string]string)
}
b.broker.Annotations[eventingv1.BrokerClassAnnotationKey] = class
return b
}

// Build to return an instance of broker object
func (b *BrokerBuilder) Build() *eventingv1.Broker {
return b.broker
Expand Down
19 changes: 18 additions & 1 deletion pkg/eventing/v1/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ import (
duckv1 "knative.dev/pkg/apis/duck/v1"
)

var testNamespace = "test-ns"
var (
testNamespace = "test-ns"
testClass = "test-class"
)

func setup() (fakeSvr fake.FakeEventingV1, client KnEventingClient) {
fakeE := fake.FakeEventingV1{Fake: &client_testing.Fake{}}
Expand Down Expand Up @@ -202,6 +205,7 @@ func TestBrokerCreate(t *testing.T) {
server, client := setup()

objNew := newBroker(name)
brokerObjWithClass := newBrokerWithClass(name)

server.AddReactor("create", "brokers",
func(a client_testing.Action) (bool, runtime.Object, error) {
Expand All @@ -219,6 +223,11 @@ func TestBrokerCreate(t *testing.T) {
assert.NilError(t, err)
})

t.Run("create broker with class without error", func(t *testing.T) {
err := client.CreateBroker(context.Background(), brokerObjWithClass)
assert.NilError(t, err)
})

t.Run("create broker with an error returns an error object", func(t *testing.T) {
err := client.CreateBroker(context.Background(), newBroker("unknown"))
assert.ErrorContains(t, err, "unknown")
Expand Down Expand Up @@ -361,6 +370,14 @@ func newTrigger(name string) *eventingv1.Trigger {
func newBroker(name string) *eventingv1.Broker {
return NewBrokerBuilder(name).
Namespace(testNamespace).
Class("").
Build()
}

func newBrokerWithClass(name string) *eventingv1.Broker {
return NewBrokerBuilder(name).
Namespace(testNamespace).
Class(testClass).
Build()
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/kn/commands/broker/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,7 @@ func createBroker(brokerName string) *v1beta1.Broker {
func createBrokerWithNamespace(brokerName, namespace string) *v1beta1.Broker {
return clientv1beta1.NewBrokerBuilder(brokerName).Namespace(namespace).Build()
}

func createBrokerWithClass(brokerName, class string) *v1beta1.Broker {
return clientv1beta1.NewBrokerBuilder(brokerName).Namespace("default").Class(class).Build()
}
6 changes: 5 additions & 1 deletion pkg/kn/commands/broker/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ var createExample = `
// 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",
Expand All @@ -58,7 +60,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 {
Expand All @@ -71,5 +74,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
}
19 changes: 19 additions & 0 deletions pkg/kn/commands/broker/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

var (
brokerName = "foo"
className = "foo-class"
)

func TestBrokerCreate(t *testing.T) {
Expand All @@ -42,6 +43,24 @@ func TestBrokerCreate(t *testing.T) {
eventingRecorder.Validate()
}

func TestBrokerCreateWithClass(t *testing.T) {
eventingClient := clienteventingv1.NewMockKnEventingClient(t)

eventingRecorder := eventingClient.Recorder()
eventingRecorder.CreateBroker(createBrokerWithClass(brokerName, className), nil)

out, err := executeBrokerCommand(eventingClient, "create", brokerName, "--class", className)
assert.NilError(t, err, "Broker should be created")
assert.Assert(t, util.ContainsAll(out, "Broker", brokerName, "created", "namespace", "default"))

eventingRecorder.CreateBroker(createBrokerWithClass(brokerName, ""), nil)
out, err = executeBrokerCommand(eventingClient, "create", brokerName, "--class", "")
assert.NilError(t, err, "Broker should be created")
assert.Assert(t, util.ContainsAll(out, "Broker", brokerName, "created", "namespace", "default"))

eventingRecorder.Validate()
}

func TestBrokerCreateWithError(t *testing.T) {
eventingClient := clienteventingv1.NewMockKnEventingClient(t)

Expand Down
12 changes: 12 additions & 0 deletions test/e2e/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

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?

Copy link
Contributor Author

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.

verifyBrokerList(r, "foo5")
verifyBrokerListOutputName(r, "foo5")
verifyBrokerDescribeContains(r, "foo5", "foo-class")
}

// Private functions
Expand Down Expand Up @@ -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))
}