[kn-source-kafka] Add kafka source command group #33

daisy-ycguo · 2020-05-21T00:53:45Z

Including:

source kafka create
source kafka delete
source kafka describe

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

knative-prow-robot · 2020-05-26T12:57:05Z

@daisy-ycguo: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

navidshaikh

I tried building locally and noticed the auto generated README is out of sync with the PR, also added a few suggestions in README

plugins/source-kafka/README.md

plugins/source-kafka/pkg/client/client_fake.go

maximilien · 2020-06-23T20:32:41Z

Please address @navidshaikh comments. Once done LGTM

maximilien · 2020-06-23T20:33:27Z

/approve

navidshaikh

Can we try and bump the client dep to master and see how that goes? This should help use broker at v1beta1 in sink prefixes for addressable of kafka source.

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

plugins/source-kafka/README.md

plugins/source-kafka/vendor/github.com/maximilien/kn-source-pkg/pkg/util/docs_generator.go

plugins/source-kafka/vendor/google.golang.org/appengine/CONTRIBUTING.md

plugins/source-kafka/vendor/google.golang.org/appengine/README.md

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

plugins/source-kafka/README.md

plugins/source-kafka/vendor/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go

plugins/source-kafka/vendor/k8s.io/client-go/plugin/pkg/client/auth/oidc/oidc.go

plugins/source-kafka/vendor/knative.dev/client/lib/test/flags.go

daisy-ycguo · 2020-06-24T16:07:54Z

@navidshaikh thank you for the review. I have addressed your comments.
To answer your question about README.md out of sync, it is because that README.md is auto generated when building but the auto generated README.md cannot meet with the format review by mattmoore. So I updated README.md manually after building. Based on our discussion in slack, I will stick with the auto generated version of README.md and ignore the format review comments.
I update e2e test script a little to apply kafka topic yaml file when setting up kafka cluster also.

maximilien · 2020-06-24T19:56:10Z

@navidshaikh LGTM so feel free to check @daisy-ycguo’s changes and merge when you feel ready. Best.

navidshaikh

Suggested multiple changes, IMO there are a few important changes we should address, for example error handling, client dependency, command help messages, etc. We may also look at doing these fixes in next iteration(s) as well.

navidshaikh · 2020-06-26T06:41:26Z

plugins/source-kafka/pkg/factories/command_factory.go

+
+func (f *kafkaSourceCommandFactory) CreateCommand() *cobra.Command {
+	createCmd := f.defaultCommandFactory.CreateCommand()
+	createCmd.Short = "create NAME"


this should be createCmd.Use with signature for mandatory flags.

See https://github.com/knative/client/blob/master/pkg/kn/commands/source/ping/create.go#L35-L36 for reference.

navidshaikh · 2020-06-26T06:42:08Z

plugins/source-kafka/pkg/factories/command_factory.go

+	sourceCmd := f.defaultCommandFactory.SourceCommand()
+	sourceCmd.Use = "kafka"
+	sourceCmd.Short = "Knative eventing Kafka source plugin"
+	sourceCmd.Long = "Manage your Knative Kafka eventing sources"


Suggested change

sourceCmd.Long = "Manage your Knative Kafka eventing sources"

sourceCmd.Long = "Manage Knative Kafka eventing sources"

or remove sourceCmd.Long if its same as .Short text.

Yes, I removed Long but use Short instead. I also changed short to use. Thank you.

navidshaikh · 2020-06-26T06:51:20Z

plugins/source-kafka/go.mod

+	gotest.tools v2.2.0+incompatible
+	k8s.io/apimachinery v0.17.4
+	k8s.io/client-go v0.17.4
+	knative.dev/client v0.14.0


IMO, we should try to keep to client's dep on the latest release (or even master) to make sure the latest fixes, eventing/serving API support is up-to-date. For example: client v0.14.0 refers brokers at v1alpha1, client master refers brokers at v1beta1 in sink prefixes; this could bring uncertain behavior. Also, IMO plugins should declare the deps and support on its root README for API support.

navidshaikh · 2020-06-26T07:25:22Z

plugins/source-kafka/pkg/client/client.go

+// NewKafkaSourceClient is to create a KafkaSourceClient
+func NewKafkaSourceClient(kafkaParams *types.KafkaSourceParams, c clientv1alpha1.SourcesV1alpha1Interface, ns string) types.KafkaSourceClient {
+	if c == nil {
+		c, _ = kafkaParams.NewSourcesClient()


not handling error here ?

throws stacktrace in absence of kubeconfig

kn source kafka create mykafkasrc --servers my-cluster-kafka-bootstrap.kafka.svc:9092 --topics test-topic --consumergroup test-consumer-group --sink svc:event-display panic: Could not create Kn source client, error: no kubeconfig has been provided, please use a valid configuration to connect to the cluster goroutine 1 [running]: github.com/maximilien/kn-source-pkg/pkg/client.NewKnSourceClient(0xc0000f7c80, 0x160ef5d, 0x7, 0xc00078f200, 0x180ab60) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/vendor/github.com/maximilien/kn-source-pkg/pkg/client/client.go:35 +0x193 knative.dev/client-contrib/plugins/source-kafka/pkg/client.NewKafkaSourceClient(0xc000057000, 0x1818a40, 0x0, 0x160ef5d, 0x7, 0xc00078e1a0, 0x0) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/pkg/client/client.go:46 +0x63 knative.dev/client-contrib/plugins/source-kafka/pkg/factories.(*kafkaClientFactory).initKafkaSourceClient(0xc0003e1740, 0x160ef5d, 0x7) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/pkg/factories/source_factory.go:99 +0x65 knative.dev/client-contrib/plugins/source-kafka/pkg/factories.(*kafkaClientFactory).CreateKafkaSourceClient(0xc0003e1740, 0x160ef5d, 0x7, 0x7, 0x0) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/pkg/factories/source_factory.go:51 +0x62 knative.dev/client-contrib/plugins/source-kafka/pkg/factories.(*kafkaSourceRunEFactory).KafkaSourceClient(...) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/pkg/factories/rune_factory.go:62 knative.dev/client-contrib/plugins/source-kafka/pkg/factories.(*kafkaSourceRunEFactory).CreateRunE.func1(0xc0000cedc0, 0xc00018e2d0, 0x1, 0x9, 0x0, 0x0) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/pkg/factories/rune_factory.go:81 +0xaf github.com/spf13/cobra.(*Command).execute(0xc0000cedc0, 0xc00018e1b0, 0x9, 0x9, 0xc0000cedc0, 0xc00018e1b0) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/vendor/github.com/spf13/cobra/command.go:832 +0x453 github.com/spf13/cobra.(*Command).ExecuteC(0xc0000ceb00, 0xc0003e1740, 0x7f9ef81c2180, 0xc0005110a0) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/vendor/github.com/spf13/cobra/command.go:920 +0x2fb github.com/spf13/cobra.(*Command).Execute(...) /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/vendor/github.com/spf13/cobra/command.go:870 main.main() /home/nshaikh/go/src/github.com/knative/client-contrib/plugins/source-kafka/cmd/main.go:34 +0x40c Error: exit status 2 Run 'kn --help' for usage

navidshaikh · 2020-06-26T08:26:37Z

plugins/source-kafka/pkg/factories/rune_factory.go

@@ -0,0 +1,222 @@
+// Copyright © 2018 The Knative Authors


run_factory.go instead of rune_factory.go ?

navidshaikh · 2020-06-26T08:53:20Z

plugins/source-kafka/pkg/client/client.go

+	return nil
+}
+
+//GetKafkaSource is used to create an instance of KafkaSource


Suggested change

//GetKafkaSource is used to create an instance of KafkaSource

//GetKafkaSource to get an instance of Kafka source by given name

navidshaikh · 2020-06-26T08:55:17Z

plugins/source-kafka/pkg/factories/command_factory.go

+func (f *kafkaSourceCommandFactory) DescribeCommand() *cobra.Command {
+	describeCmd := f.defaultCommandFactory.DescribeCommand()
+	describeCmd.Short = "describe NAME"
+	describeCmd.Long = "update a Kafka source"


Suggested change

describeCmd.Long = "update a Kafka source"

describeCmd.Long = "describe a Kafka source"

navidshaikh · 2020-06-26T08:59:31Z

plugins/source-kafka/pkg/factories/rune_factory.go

+	ref := sink.Ref
+	if ref != nil {


Suggested change

ref := sink.Ref

if ref != nil {

if sink.Ref != nil {

navidshaikh · 2020-06-26T10:26:57Z

plugins/source-kafka/cmd/main.go

+		if err.Error() != "subcommand is required" {
+			fmt.Fprintln(os.Stderr, err)
+		}


Suggested change

if err.Error() != "subcommand is required" {

fmt.Fprintln(os.Stderr, err)

}

fmt.Fprintln(os.Stderr, err)

navidshaikh · 2020-06-26T10:50:26Z

/lgtm
/approve

lets get this in and work on

in next iterations.

knative-prow-robot · 2020-06-26T10:50:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, maximilien, 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~~ [maximilien,navidshaikh]

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-29T13:33:42Z

[source-kafka] Describe fails with runtime error if sink is not set #61 Describe fails with runtime error if sink is not set

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

knative-prow-robot requested review from mattmoor and sixolet May 21, 2020 00:53

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 21, 2020

mattmoor reviewed May 21, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from 12422c9 to eb51201 Compare May 21, 2020 16:09

mattmoor reviewed May 21, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from eb51201 to a898a25 Compare May 21, 2020 16:10

mattmoor reviewed May 21, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from a898a25 to 35c6bee Compare May 21, 2020 16:15

mattmoor reviewed May 21, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from 35c6bee to e99905f Compare May 26, 2020 12:50

mattmoor reviewed May 26, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from e99905f to 8ca6e92 Compare May 26, 2020 13:00

mattmoor reviewed May 26, 2020

View reviewed changes

mattmoor reviewed Jun 22, 2020

View reviewed changes

navidshaikh reviewed 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 24, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from e2c3ef3 to 00a0e13 Compare June 24, 2020 15:11

mattmoor reviewed Jun 24, 2020

View reviewed changes

daisy-ycguo force-pushed the kafka1 branch from 00a0e13 to 1167bac Compare June 24, 2020 15:56

mattmoor reviewed Jun 24, 2020

View reviewed changes

add kafka source command group

599b45c

daisy-ycguo force-pushed the kafka1 branch from 1167bac to 599b45c Compare June 24, 2020 16:01

mattmoor reviewed Jun 24, 2020

View reviewed changes

navidshaikh reviewed Jun 26, 2020

View reviewed changes

This was referenced Jun 26, 2020

[source-kafka] Fix error handling for kubeconfig #58

Closed

[source-kafka] Fix subcommand's long, short, Use values and update docs #59

Closed

knative-prow-robot assigned navidshaikh Jun 26, 2020

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

knative-prow-robot merged commit 49864fb into knative:master Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kn-source-kafka] Add kafka source command group #33

[kn-source-kafka] Add kafka source command group #33

daisy-ycguo commented May 21, 2020 •

edited

Loading

mattmoor left a comment

mattmoor left a comment

mattmoor left a comment

mattmoor left a comment

knative-prow-robot commented May 26, 2020

navidshaikh left a comment

maximilien commented Jun 23, 2020

maximilien commented Jun 23, 2020

navidshaikh left a comment

mattmoor left a comment

mattmoor left a comment

daisy-ycguo commented Jun 24, 2020 •

edited

Loading

maximilien commented Jun 24, 2020

navidshaikh left a comment

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

daisy-ycguo Jul 8, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh Jun 26, 2020

navidshaikh commented Jun 26, 2020

knative-prow-robot commented Jun 26, 2020

navidshaikh commented Jun 29, 2020

	sourceCmd.Long = "Manage your Knative Kafka eventing sources"
	sourceCmd.Long = "Manage Knative Kafka eventing sources"

	//GetKafkaSource is used to create an instance of KafkaSource
	//GetKafkaSource to get an instance of Kafka source by given name

	describeCmd.Long = "update a Kafka source"
	describeCmd.Long = "describe a Kafka source"

[kn-source-kafka] Add kafka source command group #33

[kn-source-kafka] Add kafka source command group #33

Conversation

daisy-ycguo commented May 21, 2020 • edited Loading

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 26, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

maximilien commented Jun 23, 2020

maximilien commented Jun 23, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

daisy-ycguo commented Jun 24, 2020 • edited Loading

maximilien commented Jun 24, 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 Jun 26, 2020

knative-prow-robot commented Jun 26, 2020

navidshaikh commented Jun 29, 2020

daisy-ycguo commented May 21, 2020 •

edited

Loading

daisy-ycguo commented Jun 24, 2020 •

edited

Loading