Added --broker-config flag to broker create command #1700

vyasgun · 2022-07-04T13:33:56Z

Description

This flag will be used to set the .spec.config field in the broker object

Changes

Added the flag
The flag has to be used in conjunction with --class flag
Added unit tests and examples

Reference

Release Note

Add --broker-config flag to broker create command

knative-prow

@vyasgun: 5 warnings.

In response to this:

Description

This flag will be used to set the .spec.config field in the broker object

Changes

Added the flag

The flag has to be used in conjunction with --class flag

TODO: Add unit tests and examples

Reference

Fixes #1525

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/config_flags.go

codecov · 2022-07-04T13:38:20Z

Codecov Report

Merging #1700 (2d25e77) into main (458a6b7) will increase coverage by 0.08%.
The diff coverage is 87.65%.

@@            Coverage Diff             @@
##             main    #1700      +/-   ##
==========================================
+ Coverage   79.69%   79.77%   +0.08%     
==========================================
  Files         173      174       +1     
  Lines       13264    13357      +93     
==========================================
+ Hits        10571    10656      +85     
- Misses       1963     1968       +5     
- Partials      730      733       +3

Impacted Files	Coverage Δ
pkg/kn/commands/broker/create.go	`71.01% <78.57%> (+1.37%)`	⬆️
pkg/kn/commands/broker/config_flags.go	`88.88% <88.88%> (ø)`
pkg/eventing/v1/client.go	`91.11% <100.00%> (+0.11%)`	⬆️
pkg/kn/flags/podspec_helper.go	`82.25% <0.00%> (+0.37%)`	⬆️
pkg/kn/commands/version/version.go	`77.02% <0.00%> (+2.70%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 458a6b7...2d25e77. Read the comment docs.

maximilien

First thanks for contribution. Left some comments. However, is this part 1 of the PR. I don't see any tests?

maximilien · 2022-07-06T00:35:49Z

docs/cmd/kn_broker_create.md

@@ -23,6 +23,7 @@ kn broker create NAME
 ```
      --backoff-delay string     The delay before retrying.
      --backoff-policy string    The retry backoff policy (linear, exponential).
+      --broker-config string     Broker config object like ConfigMap or RabbitMQ


Somehow this statement is missing something "Broker config object like ConfigMap or RabbitMQ" can we be more descriptive. Also is "or" the right conjunctive here?

I updated the flag description in my latest commit. Please let me know if it can be improved further

pkg/kn/commands/broker/config_flags.go

maximilien · 2022-07-06T00:40:08Z

pkg/kn/commands/broker/config_flags.go

+			case "apiversion":
+				kRef.APIVersion = v
+			default:
+				return nil, fmt.Errorf("incorrect field %q. Please specify any of the following: Namespace, Group, APIVersion", k)


Since field test is on lower case names perhaps the error message should use lower case in message too?

The argument is being converted to lower case before the check so the user can provide it in any case. I think the error message looks clearer with capitalisation.

vyasgun · 2022-07-07T18:39:57Z

@maximilien Thanks for the review. I updated the PR with tests and more detailed docs.

dsimansk · 2022-07-12T08:42:46Z

pkg/kn/commands/broker/create.go

+  # Create a broker 'mybroker' in the myproject namespace with config referencing configmap named spec-cm in test namespace
+  kn broker create mybroker --namespace myproject --class Kafka --broker-config cm:broker-spec-cm:test


It'd nice to show at least one different example than ConfigMap.

pkg/kn/commands/broker/config_flags.go

dsimansk · 2022-07-12T08:48:24Z

pkg/kn/commands/broker/create.go

@@ -33,6 +34,15 @@ var createExample = `

  # Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka'
  kn broker create mybroker --namespace myproject --class Kafka
+
+  # Create a broker 'mybroker' in the myproject namespace with config referencing configmap named spec-cm
+  kn broker create mybroker --namespace myproject --class Kafka --broker-config cm:spec-cm


A counter proposal to the config k,v format:

--broker-config rabbitmq.com/v1beta1:RabbitmqCluster:<name>:<namespace> # With special case for core v1 resources like ConfigMaps and Secrets --broker-config cm:spec-cm:spec-cm-ns

+1, see my proposal above.

rhuss · 2022-07-12T09:43:16Z

docs/cmd/kn_broker_create.md

+
+  # Create a broker 'mybroker' in the myproject namespace with config referencing configmap named spec-cm in test namespace
+  kn broker create mybroker --namespace myproject --class Kafka --broker-config cm:broker-spec-cm:test
+


The last example seems to duplicate the example before. As @dsimansk mentioned, having an example for a secret and/or a full GVK would be helpful here.

Thanks for the suggestion. I updated the examples accordingly. Hope they look okay now

rhuss · 2022-07-12T09:51:25Z

docs/cmd/kn_broker_create.md

+      --broker-config string     Reference to the configuration that specifies configuration options for this Broker. For example, a pointer to a ConfigMap, Secret, RabbitmqCluster etc.The format for specifying the object is a colon separated string consisting of at most 3 substrings:
+                                 kind:object-name:namespace=?,apiVersion=?,group=?
+                                 The third substring is optional and the following is also acceptable (in case of ConfigMap, Secret, and RabbitmqCluster kinds):
+                                 kind:object-name


Proposal for the wording

Reference pointing to the broker configuration. The value takes the format "<prefix>:<name>:<namespace>", where <prefix> is either "cm" for a ConfigMap, "secret" for a Secret or a GVK in the format "rabbitmq.com/v1beta1/RabbitmqCluster". "name" is the name of the referenced resource and "namespace" the optional namespace of that resource (which by default is the current namespace).

I back @dsimansk opinion that we should not invent another format to provide a full GVK (like in your proposal with additional apiVersion and group), but encode it already in the prefix. We have done that already I think, for the sink: (or at least plan for this). The prefix actually should fully specify the reference type (and we allow some shortcut for well-known types like cm and secret)

Sink is using the same delimiter (':') to get the prefix. So, instead of having the prefix as a part of the first slice and then splitting it by '/', I have added the format as follows:

<apiVersion>:<kind>:<name>:<namespace>

I can change this if this doesn't seem right/consistent.

rhuss · 2022-07-12T09:55:45Z

pkg/kn/commands/broker/config_flags.go

+func getDefaultKReference(kind string) *duckv1.KReference {
+	k := strings.ToLower(kind)
+	switch k {
+	case "cm", "configmap":


could we make that switch more dynamic ? I.e. already add the list of shortcut prefixes to KReferenceMapping and then either iterate of the list to find the entry or build up a map before with the shortcut prefix as key, pointing to the KReferenceMapping entry (i.e. I would create an own Struct for the mapping with a KReference and a string array as member for KRerferenceMapping.

The advantage would be that all the relevant parts (reference + prefixes) are defined in a single place (i.e. where KReferenceMapping is defined)

Yes, sounds good. I'll do that.

dsimansk · 2022-07-12T14:11:56Z

/test all

dsimansk · 2022-07-12T17:27:47Z

/approve
/lgtm

knative-prow · 2022-07-12T17:27:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, 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:

~~OWNERS~~ [dsimansk]

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

dsimansk · 2022-07-13T11:24:52Z

/kind feature

Added --broker-config flag to broker create command

41d2a20

knative-prow bot reviewed Jul 4, 2022

View reviewed changes

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 4, 2022

maximilien reviewed Jul 6, 2022

View reviewed changes

Added unit tests

80dd398

vyasgun force-pushed the pr/broker-config branch 4 times, most recently from 7a2c935 to f88d879 Compare July 7, 2022 18:29

Added comments for exported functions and regenerated docs

dd6b4c4

vyasgun force-pushed the pr/broker-config branch from f88d879 to dd6b4c4 Compare July 8, 2022 09:21

dsimansk reviewed Jul 12, 2022

View reviewed changes

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

dsimansk reviewed Jul 12, 2022

View reviewed changes

rhuss reviewed Jul 12, 2022

View reviewed changes

Changed format for spec config

2d25e77

vyasgun force-pushed the pr/broker-config branch from c8310ac to 2d25e77 Compare July 12, 2022 16:42

knative-prow bot assigned dsimansk Jul 12, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2022

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2022

knative-prow bot merged commit 61579a2 into knative:main Jul 12, 2022

knative-prow bot added the kind/feature New feature or request label Jul 13, 2022

		# Create a broker 'mybroker' in the myproject namespace with config referencing configmap named spec-cm in test namespace
		kn broker create mybroker --namespace myproject --class Kafka --broker-config cm:broker-spec-cm:test

Added --broker-config flag to broker create command #1700

Added --broker-config flag to broker create command #1700

Uh oh!

Conversation

vyasgun commented Jul 4, 2022 • edited by dsimansk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Reference

Uh oh!

knative-prow bot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyasgun commented Jul 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsimansk Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsimansk commented Jul 12, 2022

Uh oh!

dsimansk commented Jul 12, 2022

Uh oh!

knative-prow bot commented Jul 12, 2022

Uh oh!

dsimansk commented Jul 13, 2022

Uh oh!

Uh oh!

vyasgun commented Jul 4, 2022 •

edited by dsimansk

Loading

codecov bot commented Jul 4, 2022 •

edited

Loading

dsimansk Jul 12, 2022 •

edited

Loading