Allow configuration sink prefixes #676

daisy-ycguo · 2020-02-18T07:12:51Z

Fixes #571

Proposed Changes

Allow user to config customized sink prefix in kn config file, e.g. below lines in config.yaml defines two prefixes test and hello, which map to broker in eventing.knative.dev/v1alpha1 and service in serving.knative.dev/v1 accordingly.

sink:
- prefix: test
  group: eventing.knative.dev
  resource: brokers
  version: v1alpha1
- prefix: hello
  group: serving.knative.dev
  resource: services
  version: v1

rhuss

Thanks Daisy ! One remark wrt/ error handling and I wonder where this method is supposed to be used ? I would add the usage of ConfigSinkPrefixes also to this PR

rhuss · 2020-02-18T11:11:30Z

pkg/kn/core/root.go

+
+	// set the Cfg.SinkPrefixes from viper if sink is configured
+	if viper.IsSet("sink") {
+		viper.UnmarshalKey("sink", &commands.Cfg.SinkPrefixes)


what happens with errors, e.g. when the user misconfigured it ? I think we should propagat an error message which as much context as posisble so that its easy to fix.

I added error handling in the new version.

daisy-ycguo · 2020-02-19T07:46:01Z

Thanks Daisy ! One remark wrt/ error handling and I wonder where this method is supposed to be used ? I would add the usage of ConfigSinkPrefixes also to this PR

Thank you for your review. I added the usage of sink prefix configuration in docs/README.md.

daisy-ycguo · 2020-02-19T08:47:51Z

/test pull-knative-client-integration-tests

daisy-ycguo · 2020-02-19T09:56:42Z

/test pull-knative-client-integration-tests

daisy-ycguo · 2020-02-20T07:39:27Z

/test pull-knative-client-integration-tests

daisy-ycguo · 2020-02-21T01:01:08Z

/test pull-knative-client-integration-tests

rhuss

@daisy-ycguo looks good to me with some minor request to move the error upwards and not just printing it.

Also, could you please add to CHANGELOG.adoc ?

@navidshaikh as I will be on PTO next week, could you please take over this PR ? Its ok from my side except for the issue mentioned above.. thx !

rhuss · 2020-02-21T14:23:10Z

pkg/kn/core/root.go

+	if viper.IsSet("sink") {
+		err := viper.UnmarshalKey("sink", &commands.Cfg.SinkPrefixes)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "Unable to parse sink prefixes configuration: %v .\n", err)


can we propagate this up to the top level (i.e. let this method return an error), and then stop ? Otherwise it would just continue, but I think we should deal this misconfiguration as an error. If possible, you should also add the configuration file (could maybe be obtained from viper) and the cake on the ice would be the line number (if it's easy to detect).

raise an error in the new version. Line number is not easy to detected. But I include the error message returned from viper.UnmarshalKey in the final error message, together with the config file information.

daisy-ycguo · 2020-02-25T12:47:58Z

@navidshaikh I upload a new version based on Roland's feedback. Will you help to continue the review when you are available ?

navidshaikh

build tests failing,

Added some suggestions and requested for adding a note about predefined prefixes in kn can be overridden by values in configuration file.

IMO, we should add some e2e tests here (we've plugin e2e tests setup here test/e2e/plugins_test.go )

navidshaikh · 2020-02-25T13:16:24Z

docs/README.md


 1. `pluginsDir` which is the same as the persistent flag `--plugins-dir` and specifies the kn plugins directory. It defaults to: `~/.kn/plugins`. By using the persistent flag (when you issue a command) or by specifying the value in the `kn` config, a user can select which directory to find `kn` plugins. It can be any directory that is visible to the user.

 2. `lookupPluginsInPath` which is the same as the persistent flag `--lookup-plugins-in-path` and specficies if `kn` should look for plugins anywhere in the specified `PATH` environment variable. This is a boolean configuration option and the default value is `false`.

-For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`.
+3. `sink` defines your prefix to refer to Kubernetes resources which are described by APIGroup, Version and KIND. Then a `sink` object could be described as `<prefix>:<object name>` in `kn` command lines.


Suggested change

3. `sink` defines your prefix to refer to Kubernetes resources which are described by APIGroup, Version and KIND. Then a `sink` object could be described as `<prefix>:<object name>` in `kn` command lines.

3. `sink` defines your prefix to refer to Kubernetes addressable resources. To configure a sink prefix, define following in the config file:

navidshaikh · 2020-02-25T13:19:21Z

docs/README.md


 1. `pluginsDir` which is the same as the persistent flag `--plugins-dir` and specifies the kn plugins directory. It defaults to: `~/.kn/plugins`. By using the persistent flag (when you issue a command) or by specifying the value in the `kn` config, a user can select which directory to find `kn` plugins. It can be any directory that is visible to the user.

 2. `lookupPluginsInPath` which is the same as the persistent flag `--lookup-plugins-in-path` and specficies if `kn` should look for plugins anywhere in the specified `PATH` environment variable. This is a boolean configuration option and the default value is `false`.

-For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`.
+3. `sink` defines your prefix to refer to Kubernetes resources which are described by APIGroup, Version and KIND. Then a `sink` object could be described as `<prefix>:<object name>` in `kn` command lines.
+    1. `prefix` is the prefix you want to describe your sink objects. `svc`, `service`, and `broker` are predefined prefixes in `kn`.


Suggested change

1. `prefix` is the prefix you want to describe your sink objects. `svc`, `service`, and `broker` are predefined prefixes in `kn`.

1. `prefix`: Prefix you want to describe your sink as. `service` or `svc` (`serving.knative.dev/v1`) and `broker` (`eventing.knative.dev/v1alpha1`) are predefined prefixes in `kn`. These predefined prefixes can be overridden by values in configuration file.

navidshaikh · 2020-02-25T13:20:07Z

docs/README.md

-For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`.
+3. `sink` defines your prefix to refer to Kubernetes resources which are described by APIGroup, Version and KIND. Then a `sink` object could be described as `<prefix>:<object name>` in `kn` command lines.
+    1. `prefix` is the prefix you want to describe your sink objects. `svc`, `service`, and `broker` are predefined prefixes in `kn`.
+    2. `group` is the APIGroup of Kubernetes resources.


Suggested change

2. `group` is the APIGroup of Kubernetes resources.

2. `group`: The APIGroup of Kubernetes resource.

navidshaikh · 2020-02-25T13:20:28Z

docs/README.md

+3. `sink` defines your prefix to refer to Kubernetes resources which are described by APIGroup, Version and KIND. Then a `sink` object could be described as `<prefix>:<object name>` in `kn` command lines.
+    1. `prefix` is the prefix you want to describe your sink objects. `svc`, `service`, and `broker` are predefined prefixes in `kn`.
+    2. `group` is the APIGroup of Kubernetes resources.
+    3. `version` is the version of Kubernetes resources.


Suggested change

3. `version` is the version of Kubernetes resources.

3. `version`: The version of Kubernetes resources.

navidshaikh · 2020-02-25T13:21:04Z

docs/README.md

+    1. `prefix` is the prefix you want to describe your sink objects. `svc`, `service`, and `broker` are predefined prefixes in `kn`.
+    2. `group` is the APIGroup of Kubernetes resources.
+    3. `version` is the version of Kubernetes resources.
+    4. `resource` is the plural name of Kubernetes resources. 


Suggested change

4. `resource` is the plural name of Kubernetes resources.

4. `resource`: The plural name of Kubernetes resources (for example: services).

navidshaikh · 2020-02-25T13:25:14Z

docs/README.md

+    3. `version` is the version of Kubernetes resources.
+    4. `resource` is the plural name of Kubernetes resources. 
+
+For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`. It also defines a sink prefix `myprefix` which refer to `brokers` in `eventing.knative.dev/v1alpha1`. With this configuration, you could use `myprefix:default` to describe Broker `default` in `kn` command lines.


Suggested change

For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`. It also defines a sink prefix `myprefix` which refer to `brokers` in `eventing.knative.dev/v1alpha1`. With this configuration, you could use `myprefix:default` to describe Broker `default` in `kn` command lines.

For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`.

It also defines a sink prefix `myprefix` which refers to `brokers` in `eventing.knative.dev/v1alpha1`. With this configuration, you can use `myprefix:default` to describe a Broker `default` in `kn` command line.

thanks. fixed in the new version.

navidshaikh · 2020-02-25T15:31:03Z

build tests failing on unrelated change for MD linting here

docs/README.md\u0026#xA;	ERROR	https://docs.openshift.com/container-platform/4.1/cli_reference/administrator-cli-commands.html#create-kubeconfig\u0026#xA;		Bad Request (HTTP error 400)\u0026#xA;

daisy-ycguo · 2020-02-26T07:11:40Z

/retest

navidshaikh · 2020-02-26T09:20:49Z

/retest

panic: test timed out after 30m0s

knative-metrics-robot · 2020-02-28T06:11:25Z

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/kn/commands/flags/sink.go	61.3%	57.6%	-3.7
pkg/kn/core/root.go	53.8%	50.4%	-3.4

daisy-ycguo · 2020-02-28T08:41:29Z

@navidshaikh tests pass. please have a second review. Thank you.

navidshaikh

/lgtm
/approve

Thanks!

navidshaikh · 2020-02-28T10:01:02Z

test/e2e/sinkprefix_test.go

+	test.cronJobSourceDelete(t, r, "testcronjobsource0")
+}
+
+func (test *e2eTest) cronJobSourceCreateWithConfig(t *testing.T, r *KnRunResultCollector, sourceName string, schedule string, data string, sink string, config string) {


Since CronJob source is deprecated and will be removed, I'd defer to use it for any new work/tests. As we'll need to re-work this later. Fine for now, but for any subsequent tests, lets use other sources.

knative-prow-robot · 2020-02-28T10:02:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, navidshaikh

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~~ [navidshaikh]

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

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2020

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 18, 2020

knative-prow-robot requested review from mattmoor and rhuss February 18, 2020 07:13

daisy-ycguo force-pushed the configsink branch 2 times, most recently from 1e8f0f3 to f8cff8a Compare February 18, 2020 10:02

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 18, 2020

daisy-ycguo changed the title ~~[WIP] Allow configuration sink prefixes~~ Allow configuration sink prefixes Feb 18, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2020

rhuss requested changes Feb 18, 2020

View reviewed changes

daisy-ycguo force-pushed the configsink branch from f8cff8a to 31fdff4 Compare February 19, 2020 07:44

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 19, 2020

daisy-ycguo force-pushed the configsink branch from 31fdff4 to dc301ff Compare February 19, 2020 07:49

rhuss requested changes Feb 21, 2020

View reviewed changes

daisy-ycguo force-pushed the configsink branch 2 times, most recently from 1f21538 to c503c4b Compare February 25, 2020 12:46

navidshaikh requested changes Feb 25, 2020

View reviewed changes

daisy-ycguo force-pushed the configsink branch from c503c4b to f44b920 Compare February 25, 2020 14:39

navidshaikh mentioned this pull request Feb 25, 2020

Disable markdown link check and enable presubmit test fail fast #685

Merged

daisy-ycguo force-pushed the configsink branch from f44b920 to 3b6915f Compare February 27, 2020 10:35

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2020

daisy-ycguo force-pushed the configsink branch 2 times, most recently from f01501b to 60186f2 Compare February 27, 2020 15:08

Allow configuration sink prefixes

4b9cf7c

daisy-ycguo force-pushed the configsink branch from 60186f2 to 4b9cf7c Compare February 28, 2020 06:08

navidshaikh reviewed Feb 28, 2020

View reviewed changes

knative-prow-robot assigned navidshaikh Feb 28, 2020

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

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

knative-prow-robot merged commit d14c01f into knative:master Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration sink prefixes #676

Allow configuration sink prefixes #676

daisy-ycguo commented Feb 18, 2020 •

edited

Loading

rhuss left a comment

rhuss Feb 18, 2020

daisy-ycguo Feb 19, 2020

daisy-ycguo commented Feb 19, 2020

daisy-ycguo commented Feb 19, 2020

daisy-ycguo commented Feb 19, 2020

daisy-ycguo commented Feb 20, 2020

daisy-ycguo commented Feb 21, 2020

rhuss left a comment

rhuss Feb 21, 2020

daisy-ycguo Feb 25, 2020 •

edited

Loading

daisy-ycguo commented Feb 25, 2020

navidshaikh left a comment

navidshaikh Feb 25, 2020

daisy-ycguo Feb 25, 2020

navidshaikh Feb 25, 2020

daisy-ycguo Feb 25, 2020

navidshaikh Feb 25, 2020

daisy-ycguo Feb 25, 2020

navidshaikh Feb 25, 2020

daisy-ycguo Feb 25, 2020

navidshaikh Feb 25, 2020

daisy-ycguo Feb 25, 2020

navidshaikh Feb 25, 2020

daisy-ycguo Feb 25, 2020

navidshaikh commented Feb 25, 2020 •

edited

Loading

daisy-ycguo commented Feb 26, 2020

navidshaikh commented Feb 26, 2020

knative-metrics-robot commented Feb 28, 2020

daisy-ycguo commented Feb 28, 2020

navidshaikh left a comment

navidshaikh Feb 28, 2020

knative-prow-robot commented Feb 28, 2020

	3. `sink` defines your prefix to refer to Kubernetes resources which are described by APIGroup, Version and KIND. Then a `sink` object could be described as `<prefix>:<object name>` in `kn` command lines.
	3. `sink` defines your prefix to refer to Kubernetes addressable resources. To configure a sink prefix, define following in the config file:

	1. `prefix` is the prefix you want to describe your sink objects. `svc`, `service`, and `broker` are predefined prefixes in `kn`.
	1. `prefix`: Prefix you want to describe your sink as. `service` or `svc` (`serving.knative.dev/v1`) and `broker` (`eventing.knative.dev/v1alpha1`) are predefined prefixes in `kn`. These predefined prefixes can be overridden by values in configuration file.

	2. `group` is the APIGroup of Kubernetes resources.
	2. `group`: The APIGroup of Kubernetes resource.

	3. `version` is the version of Kubernetes resources.
	3. `version`: The version of Kubernetes resources.

	4. `resource` is the plural name of Kubernetes resources.
	4. `resource`: The plural name of Kubernetes resources (for example: services).

	For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`. It also defines a sink prefix `myprefix` which refer to `brokers` in `eventing.knative.dev/v1alpha1`. With this configuration, you could use `myprefix:default` to describe Broker `default` in `kn` command lines.
	For example, the following `kn` config will look for `kn` plugins in the user's `PATH` and also execute plugin in `~/.kn/plugins`.
	It also defines a sink prefix `myprefix` which refers to `brokers` in `eventing.knative.dev/v1alpha1`. With this configuration, you can use `myprefix:default` to describe a Broker `default` in `kn` command line.

Allow configuration sink prefixes #676

Allow configuration sink prefixes #676

Conversation

daisy-ycguo commented Feb 18, 2020 • edited Loading

Proposed Changes

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

daisy-ycguo commented Feb 19, 2020

daisy-ycguo commented Feb 19, 2020

daisy-ycguo commented Feb 19, 2020

daisy-ycguo commented Feb 20, 2020

daisy-ycguo commented Feb 21, 2020

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daisy-ycguo Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

daisy-ycguo commented Feb 25, 2020

navidshaikh 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

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

navidshaikh commented Feb 25, 2020 • edited Loading

daisy-ycguo commented Feb 26, 2020

navidshaikh commented Feb 26, 2020

knative-metrics-robot commented Feb 28, 2020

daisy-ycguo commented Feb 28, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 28, 2020

daisy-ycguo commented Feb 18, 2020 •

edited

Loading

daisy-ycguo Feb 25, 2020 •

edited

Loading

navidshaikh commented Feb 25, 2020 •

edited

Loading