Add broker create, delete and list cmds #894

dsimansk · 2020-06-19T09:16:32Z

Description

Since the Eventing 0.15 multitenant broker is the new default. MT Broker is easier to manage from user's namespace because there're no more requirements for additional permission as majority of broker deployment is installed in knative-eventing namespace. This PR adds commands to manage broker CR.

Changes

Add broker create, broker delete, broker list commands

Note 1: E2E are missing right now, I'm currently working on them.

Note 2: I haven't added update operation since the current impl only creates Broker CR with a name without additional specs.

Reference

Fixes #

knative-prow-robot

@dsimansk: 8 warnings.

In response to this:

Description

Since the Eventing 0.15 multitenant broker is the new default. This PR adds commands to manage broker CR.

Changes

Add broker create, broker delete, broker list commands

Note 1: E2E are missing right now, I'm currently working on them.

Note 2: I haven't added update operation since the current impl only creates Broker CR with a name without additional specs.

Reference

Fixes #

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.

pkg/kn/commands/broker/create.go

pkg/kn/commands/broker/broker.go

pkg/kn/commands/broker/list.go

pkg/kn/commands/broker/delete.go

rhuss

Thanks a lot, looks good so far.

The only bigger question is, whether we should allow a default value for the name. As mentioned in the comments, it is is very unlikely that the user is using multiple brokers ever, then allowing a kn broker create for creating the default broker makes much sense (much like kn trigger create --inject-broker also only creates the default broker).

What are your opinions on this ? // cc: @maximilien @daisy-ycguo @navidshaikh @duglin

rhuss · 2020-06-19T09:29:02Z

docs/cmd/kn_broker_create.md

+Create a broker.
+
+```
+kn broker create NAME


As there is a good default for a broker (named 'default') I wonder whether we can make the name optional for all broker commands.

That would help in creating a default broker like the injection would do

kn broker create Broker 'default' is created

I'd go for requiring the name explicitly as arg for create. Keeping create formula consistent with other resources.

I've decided to keep create consistent with the rest of resource creation flow, but I've had those thoughts myself that "default" might be worth. However, we can consider "default" to be exclusive for annotation created broker, therefore keeping name argument required here.

pkg/eventing/v1beta1/client.go

pkg/kn/commands/broker/broker.go

pkg/kn/commands/broker/broker_test.go

pkg/kn/commands/broker/create.go

rhuss · 2020-06-19T09:35:26Z

pkg/kn/commands/broker/create.go

+			if len(args) != 1 {
+				return errors.New("'broker create' requires the broker name given as single argument")
+			}
+			name := args[0]


we might defaullt to "default" here, but I leave this open for discussion.

pkg/kn/commands/broker/delete.go

rhuss · 2020-06-19T09:37:39Z

pkg/kn/commands/broker/delete.go

+			if len(args) != 1 {
+				return errors.New("'broker delete' requires the broker name given as single argument")
+			}
+			name := args[0]


Here I'm less sure whether we should default to "default", as this could lead to an accidental deletion of a broker.

It really depends how likely the use case to have multiple brokers in a namespace is. If 95% of all usescases only use the default broker, then having a default value is helpful as this supports the user (who might not even now that he can have more than one broker).

pkg/kn/commands/broker/list.go

pkg/kn/root/root.go

Co-authored-by: Roland Huß <[email protected]>

dsimansk · 2020-06-19T14:10:35Z

/lint

knative-prow-robot

@dsimansk: 1 unresolved warnings and 1 new warnings.

In response to this:

/lint

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.

pkg/kn/commands/broker/list.go

maximilien

Some small issues but one "bigger" one in lack of testing in describe command.

Overall, nice work. Thanks for contribution.

pkg/eventing/v1beta1/client.go

maximilien · 2020-06-19T21:50:35Z

pkg/eventing/v1beta1/client_mock.go

+}
+
+// GetBroker records a call for GetBroker with the expected object or error. Either trigger or err should be nil
+func (sr *EventingRecorder) GetBroker(name interface{}, broker *v1beta1.Broker, err error) {


Why name the interface{} parameter name here but broker in line 118?

In 118 it is supposed to be a Broker object normally (but is in an interface{} due to the way the mocking works with expectactions), but here its the name of a Broke (a string normally, again an interface to allow setting expectatiions like Any() ...)

I think its totally fine to mimic the names of the interface here.

maximilien · 2020-06-19T21:51:05Z

pkg/eventing/v1beta1/client_mock.go

+}
+
+// DeleteBroker records a call for DeleteBroker with the expected error (nil if none)
+func (sr *EventingRecorder) DeleteBroker(name interface{}, err error) {


Same comment as before?

see response above, the wording should reflect the wording of the interface.

maximilien · 2020-06-19T22:17:30Z

pkg/kn/commands/broker/describe_test.go

+	"knative.dev/client/pkg/util"
+)
+
+func TestBrokerDescribe(t *testing.T) {


Test is not complete... I can comment out following and still pass the test:

// describeBroker print broker details to the provided output writer func describeBroker(out io.Writer, broker *v1beta1.Broker, printDetails bool) error { dw := printers.NewPrefixWriter(out) commands.WriteMetadata(dw, &broker.ObjectMeta, printDetails) // dw.WriteLine() dw.WriteAttribute("Address", "").WriteAttribute("URL", broker.Status.Address.URL.String()) // dw.WriteLine() // commands.WriteConditions(dw, broker.Status.Conditions, printDetails) if err := dw.Flush(); err != nil { return err } return nil }

Addressed in 25ec5bd, let me know if the test's acceptable now.

dsimansk · 2020-06-21T20:24:43Z

/retest

navidshaikh · 2020-06-22T05:21:55Z

test/e2e/broker_test.go

+	brokerCreate(r, "foo2")
+	verifyBrokerList(r, "foo2")
+	brokerDelete(r, "foo2")
+	verifyBrokerNotfound(r, "foo2")


@dsimansk : seems like the e2e failure

TestBroker: result_collector.go:84: ERROR: Error expected but no error happened

log

is the scenario where the resource being deleted is described before delete completes?

Per our chat discussion, there seems to be race condition between actual completion of broker delete operation out verifyBrokerNotfound. I've addes sync delete in commit e7b5261. Lets see if it helps.

navidshaikh · 2020-06-22T05:25:18Z

/retest

navidshaikh

suggested a few nits

pkg/kn/commands/broker/describe.go

pkg/eventing/v1beta1/client.go

pkg/kn/commands/broker/describe.go

pkg/kn/commands/broker/list.go

Co-authored-by: Navid Shaikh <[email protected]>

pkg/kn/commands/broker/describe.go

knative-metrics-robot · 2020-06-23T09:19:39Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/eventing/v1beta1/client.go	83.1%	87.4%	4.3
pkg/eventing/v1beta1/client_mock.go	90.9%	94.1%	3.2
pkg/kn/commands/broker/broker.go	Do not exist	100.0%
pkg/kn/commands/broker/create.go	Do not exist	83.3%
pkg/kn/commands/broker/delete.go	Do not exist	81.8%
pkg/kn/commands/broker/describe.go	Do not exist	84.0%
pkg/kn/commands/broker/list.go	Do not exist	88.6%

rhuss

I agree now that we should require an explicit name and no implicit "default". If we ever change our minds we can change to a default "default" broker in a backwards-compatible way later. But not vice versa.

knative-prow-robot · 2020-06-23T09:21:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

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:

~~OWNERS~~ [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

navidshaikh · 2020-06-23T09:27:08Z

pkg/kn/commands/broker/delete.go

-  kn broker create mybroker --namespace myproject
-`
+  # Delete a broker 'mybroker' in the 'myproject' namespace
+  kn broker create mybroker --namespace myproject`


rhuss · 2020-06-23T09:29:15Z

/lgtm

Add broker create, delete and list cmds

22e9274

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 19, 2020

knative-prow-robot requested review from mattmoor and rhuss June 19, 2020 09:16

knative-prow-robot reviewed Jun 19, 2020

View reviewed changes

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 19, 2020

mattmoor reviewed Jun 19, 2020

View reviewed changes

dsimansk added 2 commits June 19, 2020 11:25

fix: Fix copyright headers

91abb1c

fix: Fix linter warnings

54befac

rhuss requested changes Jun 19, 2020

View reviewed changes

dsimansk and others added 3 commits June 19, 2020 12:05

Update pkg/kn/commands/broker/create.go

629b73b

Co-authored-by: Roland Huß <[email protected]>

Update pkg/kn/commands/broker/delete.go

10536f2

Co-authored-by: Roland Huß <[email protected]>

Update pkg/kn/commands/broker/list.go

db5a42a

Co-authored-by: Roland Huß <[email protected]>

navidshaikh added this to the v0.16.0 milestone Jun 19, 2020

feat: Add broker describe command

2997489

mattmoor reviewed Jun 19, 2020

View reviewed changes

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 19, 2020

fix: Fix copyright header

0f8a96a

knative-prow-robot reviewed Jun 19, 2020

View reviewed changes

pkg/kn/commands/broker/list.go Show resolved Hide resolved

fix: Fix linter issues

784a912

maximilien suggested changes Jun 19, 2020

View reviewed changes

dsimansk added 5 commits June 21, 2020 20:00

fix: Fix broker describe test

25ec5bd

fix: Fix order of eventing cmds

4e70573

chore: Add e2e tests

9b2901a

fix: Fix e2e tests annotation

d671e82

fix: Reorder e2e tests methods

2e0e657

navidshaikh reviewed Jun 22, 2020

View reviewed changes

feat: Add synchronous broker delete option

e7b5261

navidshaikh requested changes Jun 22, 2020

View reviewed changes

pkg/kn/commands/broker/describe.go Outdated Show resolved Hide resolved

pkg/eventing/v1beta1/client.go Outdated Show resolved Hide resolved

pkg/kn/commands/broker/describe.go Outdated Show resolved Hide resolved

pkg/kn/commands/broker/list.go Outdated Show resolved Hide resolved

dsimansk and others added 5 commits June 22, 2020 13:22

fix: Fix broker describe example

1df31dc

Co-authored-by: Navid Shaikh <[email protected]>

fix: Fix variable name

feacf79

Co-authored-by: Navid Shaikh <[email protected]>

fix: Fix broker list example formatting

a61ef4c

Co-authored-by: Navid Shaikh <[email protected]>

fix: Fix eventing client func name

45abb53

Co-authored-by: Navid Shaikh <[email protected]>

fix: Reflect changes from PR review suggestions

045175e

navidshaikh reviewed Jun 22, 2020

View reviewed changes

pkg/kn/commands/broker/describe.go Outdated Show resolved Hide resolved

fix: Fix formatting of cmd examples

6f2d98c

mattmoor reviewed Jun 22, 2020

View reviewed changes

dsimansk added 3 commits June 22, 2020 13:58

fix: Fix formatting of cmd examples & trailing spaces

c24eba1

fix: Polish eventing client docs

90a0dec

fix: Fix examples alignment

db8f0fb

rhuss approved these changes Jun 23, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020

navidshaikh reviewed Jun 23, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jun 23, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

knative-prow-robot merged commit 1f59f53 into knative:master Jun 23, 2020

dsimansk deleted the broker-crud branch June 23, 2020 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add broker create, delete and list cmds #894

Add broker create, delete and list cmds #894

dsimansk commented Jun 19, 2020 •

edited

Loading

knative-prow-robot left a comment

rhuss left a comment

rhuss Jun 19, 2020

navidshaikh Jun 19, 2020

dsimansk Jun 19, 2020

rhuss Jun 19, 2020

rhuss Jun 19, 2020

dsimansk commented Jun 19, 2020

knative-prow-robot left a comment

maximilien left a comment

maximilien Jun 19, 2020

rhuss Jun 20, 2020

maximilien Jun 19, 2020

rhuss Jun 20, 2020

maximilien Jun 19, 2020

dsimansk Jun 21, 2020

dsimansk commented Jun 21, 2020

navidshaikh Jun 22, 2020

dsimansk Jun 22, 2020

navidshaikh commented Jun 22, 2020

navidshaikh left a comment

knative-metrics-robot commented Jun 23, 2020

rhuss left a comment

knative-prow-robot commented Jun 23, 2020

navidshaikh Jun 23, 2020

rhuss commented Jun 23, 2020

Add broker create, delete and list cmds #894

Add broker create, delete and list cmds #894

Conversation

dsimansk commented Jun 19, 2020 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Jun 19, 2020

knative-prow-robot left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Jun 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh commented Jun 22, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 23, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 23, 2020

Choose a reason for hiding this comment

rhuss commented Jun 23, 2020

dsimansk commented Jun 19, 2020 •

edited

Loading