Add command to create/delete a plain trigger by daisy-ycguo · Pull Request #541 · knative/client

daisy-ycguo · 2019-12-09T09:14:26Z

Fixes #479

Proposed Changes

Add command for creation of a plain trigger
Add command for deletion of a trigger
Add command for description of a trigger

daisy-ycguo · 2019-12-12T01:58:23Z

/test pull-knative-client-integration-tests

sixolet · 2019-12-12T22:17:53Z

/approve
/lgtm

sixolet · 2019-12-12T22:45:46Z

/retest

maximilien

OK I stopped at file 19. There are too many issues. Please address and I’ll take another look.

Also, use the /lint option to see many of these issues and address them before submitting PR

maximilien · 2019-12-12T22:44:30Z

+### SEE ALSO
+
+* [kn](kn.md)	 - Knative client
+* [kn trigger create](kn_trigger_create.md)	 - Create a Trigger


Why is “trigger” spelled with capital T here? Be consistent with Delete and the rest.

Change to lower case t in the whole PR (unless starting a sentence)

maximilien · 2019-12-12T22:45:27Z

@@ -0,0 +1,42 @@
+## kn trigger create
+
+Create a Trigger


Consistent spelling of Trigger or trigger (unless starting a sentence)... across all text.

Change to lower case t in the whole PR (unless starting a sentence)

Change to lower case t in the whole PR (unless starting a sentence)

maximilien · 2019-12-12T22:45:41Z

@@ -0,0 +1,39 @@
+## kn trigger delete
+
+Delete a trigger.


Spelling...

Change to lower case t in the whole PR (unless starting a sentence)

maximilien · 2019-12-12T22:47:22Z

+// Temporarily help to add sources dependencies
+// May be changed when adding real sources features
+type knEventingClient struct {
+	client    client_v1alpha1.EventingV1alpha1Interface


Be consistent with creation in the order of the fields? It’s reverse here to the NewKnEventingClient constructor...

I changed the order of fields in NewKnEventingClient constructor to be consistent.

maximilien · 2019-12-12T22:48:27Z

+
+//CreateTrigger is used to create an instance of Trigger
+func (c *knEventingClient) CreateTrigger(trigger *v1alpha1.Trigger) (*v1alpha1.Trigger, error) {
+	ins, err := c.client.Triggers(c.namespace).Create(trigger)


Minor: What does ins mean? Why not call it trigger or something descriptive.

ins means instance. :). I updated to use trigger

maximilien · 2019-12-12T23:07:56Z

+
+// BrokerInterface has methods to work with Broker resources.
+type BrokerInterface interface {
+	Create(*v1alpha1.Broker) (*v1alpha1.Broker, error)


Inconsistent use of comments. In previous interface you added comments for each method. Not here. IMO less comments is better since if name is well selected it’s clear. However, I also would agree to have comments for public interfaces and methods.

Either way be consistent and follow the rest of code. This is not being done here.

It's Eventing client dependency file, added by go mod while building and testing.

maximilien · 2019-12-12T23:08:38Z

+	restClient rest.Interface
+}
+
+func (c *EventingV1alpha1Client) Brokers(namespace string) BrokerInterface {


Public methods need comments or won’t pass the linter

It's Eventing client dependency file, added by go mod while building and testing.

maximilien · 2019-12-12T23:10:08Z

+// NewForConfig creates a new EventingV1alpha1Client for the given config.
+func NewForConfig(c *rest.Config) (*EventingV1alpha1Client, error) {
+	config := *c
+	if err := setConfigDefaults(&config); err != nil {


Don’t use this form. Extract the setCopnfigDefaults(&config) outside of the if since that’s consistent to the other line and most of this repo’s code

It's Eventing client dependency file, added by go mod while building and testing.

maximilien · 2019-12-12T23:11:10Z

+// RESTClient returns a RESTClient that is used to communicate
+// with API server by this client implementation.
+func (c *EventingV1alpha1Client) RESTClient() rest.Interface {
+	if c == nil {


How would c == nil be ever true?

If c is nill then an exception will be thrown before RESTClient() be executed. So these three lines are dead code...

It's Eventing client dependency file, added by go mod while building and testing.

@maximilien Of course can c be nil. Try

EventingV1alphaClient(nil).RESTClient()

and as mentioned, its a dependeny added. It's also autogenerated.

The reason for this check is to allow chained called where an intermediate call returns nil like in

myServicHub.EventingClient().RESTClient()

and EventingClient() return nil (but is of type *EventingV1alphaClient)

Ok but yuck... :)

maximilien · 2019-12-12T23:13:17Z

+
+// Get takes name of the eventType, and returns the corresponding eventType object, and an error if there is any.
+func (c *eventTypes) Get(name string, options v1.GetOptions) (result *v1alpha1.EventType, err error) {
+	result = &v1alpha1.EventType{}


Again do not use named return args since it’s not consistent With rest of your code and this repo.

Future versions of Golang might remove this “feature” which is also another reason.

It's Eventing client dependency file, added by go mod while building and testing.

daisy-ycguo · 2019-12-13T03:01:58Z

@maximilien Thank you for review. This pr has dependencies to

knative.dev/eventing/pkg/client/clientset/versioned/typed/eventing/v1alpha1
knative.dev/eventing/pkg/client/clientset/versioned/typed/eventing/v1alpha1/fake

All files under "vendor" folder are automatically added by the build and test scripts.
The files under "pkg" are added by me.

daisy-ycguo · 2019-12-13T13:20:51Z

/lint

knative-prow-robot

@daisy-ycguo: 6 warnings.

Details

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.

knative-prow-robot · 2019-12-13T13:21:02Z

+)
+
+// TriggerUpdateFlags are flags for create and update a trigger
+type TriggerUpdateFlags struct {


Suggested change

type TriggerUpdateFlags struct {

type UpdateFlags struct {

Golint naming: type name will be used as trigger.TriggerUpdateFlags by other packages, and that stutters; consider calling this UpdateFlags. More info.

knative-prow-robot · 2019-12-13T13:21:03Z

+	duckv1 "knative.dev/pkg/apis/duck/v1"
+)
+
+func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command {


Golint comments: exported function NewTriggerCreateCommand should have comment or be unexported. More info.

knative-prow-robot · 2019-12-13T13:21:03Z

+	"knative.dev/client/pkg/kn/commands"
+)
+
+func NewTriggerCommand(p *commands.KnParams) *cobra.Command {


Golint comments: exported function NewTriggerCommand should have comment or be unexported. More info.

knative-prow-robot · 2019-12-13T13:21:03Z

+	"knative.dev/eventing/pkg/apis/eventing/v1alpha1"
+)
+
+type MockKnEventingClient struct {


Golint comments: exported type MockKnEventingClient should have comment or be unexported. More info.

knative-prow-robot · 2019-12-13T13:21:03Z

+// Ensure that the interface is implemented
+var _ KnEventingClient = &MockKnEventingClient{}
+
+// recorder for service


Golint comments: comment on exported type EventingRecorder should be of the form "EventingRecorder ..." (with optional leading article). More info.

knative-prow-robot · 2019-12-13T13:21:03Z

+	return mock.ErrorOrNil(call.Result[0])
+}
+
+// Validates validates whether every recorded action has been called


Golint comments: comment on exported method EventingRecorder.Validate should be of the form "Validate ...". More info.

rhuss

Thanks a lot, some comments see below.

In general we should use only Mock tests for the command tests. Also it woudl be awesome if you could reduce duplicated code in the tests to minimize the references to v1alpha1 structs, so that a later upgrade becomes easier.

rhuss · 2019-12-13T13:43:40Z

+### Options
+
+```
+      --broker string      Name of the Broker which the trigger associates with. Defaults to 'default' if not specified. (default "default")


You could skip the explanation for 'default' as it is automatically added by cobra

rhuss · 2019-12-13T13:44:52Z

+
+```
+      --broker string      Name of the Broker which the trigger associates with. Defaults to 'default' if not specified. (default "default")
+      --filter strings     Comma seperate key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo


Again, I would prefer to allow multiple flter arguments instead of puttng everything tog gether into a single filter. E.g. what's about escaping for value that contain comma ?

I changed to use multiple --filter

rhuss · 2019-12-13T13:45:31Z

+      --filter strings     Comma seperate key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo
+  -h, --help               help for create
+  -n, --namespace string   Specify the namespace to operate in.
+  -s, --sink string        Addressable sink for events


Should contain examples, like for sources.

Examples are contained in line #13.

I mean, describing the possible format (e.g. prefix : name) and a reference to the list of possible prefixes. Lets keep it like now, going to make an overall check of all sources later.

rhuss · 2019-12-13T13:47:42Z

+)
+
+// MockKnEventingClient is a combine of test object and recorder
+type MockKnEventingClient struct {


rhuss · 2019-12-13T13:47:59Z

+
+//GetTrigger is used to get an instance of trigger
+func (c *knEventingClient) GetTrigger(name string) (*v1alpha1.Trigger, error) {
+	return c.client.Triggers(c.namespace).Get(name, apis_v1.GetOptions{})


Why no error conversion here ?

rhuss · 2019-12-13T13:59:57Z

+	}
+
+	//construct a wanted instance
+	wanted := &v1alpha1.Trigger{


You should put the duplicated code imo a method, parameterized with the stuff that changes. The more often you use v1alpha objects directly the harder it will be to upgrade to a v1 version. Even in tests, we should avoid duplicated code as much as possible.

rhuss · 2019-12-13T14:00:46Z

+	cmd, fakeEventing, buf := commands.CreateEventingTestKnCommand(NewTriggerCommand(knParams), knParams)
+	fakeEventing.AddReactor("delete", "triggers",
+		func(a client_testing.Action) (bool, runtime.Object, error) {
+			deleteAction, _ := a.(client_testing.DeleteAction)


Because the test will panic anyway if its wrong.

rhuss · 2019-12-13T14:03:34Z

+//Add is to set parameters
+func (f *TriggerUpdateFlags) Add(cmd *cobra.Command) {
+	cmd.Flags().StringVar(&f.Broker, "broker", "default", "Name of the Broker which the trigger associates with. Defaults to 'default' if not specified.")
+	cmd.Flags().StringSliceVar(&f.Filter, "filter", nil, "Comma seperate key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo")


we allow multiple --filter options, with every option being a single filter. So singular is correct. However its inocrrect that you can use comma separated filters in one option, if I understand the code above correctly.

rhuss · 2019-12-13T14:06:52Z


+	if params.NewEventingClient == nil {
+		params.NewEventingClient = params.newEventingClient
+	}


This technique allows test code to hook in by setting the NewEventingClient in advance before reaching this method with a fake implementation.

I'm also not happy with this construct (for other reasons, mostly because of coupling layers unnecessarily), but I'm happy to take this over as is (as its follows the same scheme like for the other clients).

In a subsequent PR we should refactor the client handling to be more delocalized maybe.

rhuss · 2019-12-13T14:08:59Z

+// RESTClient returns a RESTClient that is used to communicate
+// with API server by this client implementation.
+func (c *EventingV1alpha1Client) RESTClient() rest.Interface {
+	if c == nil {


The reason for this check is to allow chained called where an intermediate call returns nil like in

myServicHub.EventingClient().RESTClient()

and EventingClient() return nil (but is of type *EventingV1alphaClient)

knative-metrics-robot · 2019-12-16T09:52:50Z

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/v1alpha1/client.go	Do not exist	92.9%
pkg/eventing/v1alpha1/client_mock.go	Do not exist	87.5%
pkg/kn/commands/testing_helper.go	64.4%	54.3%	-10.1
pkg/kn/commands/trigger/create.go	Do not exist	86.1%
pkg/kn/commands/trigger/delete.go	Do not exist	82.4%
pkg/kn/commands/trigger/describe.go	Do not exist	83.3%
pkg/kn/commands/trigger/flag.go	Do not exist	93.8%
pkg/kn/commands/trigger/trigger.go	Do not exist	100.0%
pkg/kn/commands/types.go	62.2%	53.8%	-8.4
pkg/kn/core/root.go	60.8%	61.2%	0.4

daisy-ycguo · 2019-12-16T14:20:40Z

@maximilien @rhuss @sixolet This PR is ready for a second review.
Besides the minor changes, I

used multiple --filter to manage filter options
rewrote unit tests to use mock client

rhuss · 2019-12-16T15:32:31Z

+}
+
+//CreateTrigger is used to create an instance of trigger
+func (c *knEventingClient) CreateTrigger(trigger *v1alpha1.Trigger) (*v1alpha1.Trigger, error) {


To be in accordance with our other APIs, we should not return the created trigger here.

rhuss

/lgtm

Thanks !

knative-prow-robot · 2019-12-16T15:40:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss,sixolet]

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 requested review from evankanderson and rhuss December 9, 2019 09:14

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 9, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 9, 2019

daisy-ycguo force-pushed the trigger branch from f2202d9 to 0a0d991 Compare December 9, 2019 09:17

daisy-ycguo changed the title ~~Add command for creation of a plain trigger~~ [WIP] Add command for creation of a plain trigger Dec 9, 2019

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 Dec 9, 2019

daisy-ycguo force-pushed the trigger branch from 0a0d991 to 88367ed Compare December 11, 2019 14:42

daisy-ycguo changed the title ~~[WIP] Add command for creation of a plain trigger~~ [Add command for creation of a plain trigger Dec 11, 2019

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 Dec 11, 2019

daisy-ycguo changed the title ~~[Add command for creation of a plain trigger~~ Add command for creation of a plain trigger Dec 11, 2019

daisy-ycguo force-pushed the trigger branch 4 times, most recently from 887f4da to 2ea32d6 Compare December 11, 2019 15:38

daisy-ycguo force-pushed the trigger branch 2 times, most recently from 18880c5 to 5bd6789 Compare December 12, 2019 09:28

daisy-ycguo changed the title ~~Add command for creation of a plain trigger~~ Add command to create/delete a plain trigger Dec 12, 2019

knative-prow-robot assigned sixolet Dec 12, 2019

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 12, 2019

maximilien suggested changes Dec 12, 2019

View reviewed changes

daisy-ycguo force-pushed the trigger branch from 5bd6789 to 36a6e0e Compare December 13, 2019 05:06

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2019

daisy-ycguo force-pushed the trigger branch 2 times, most recently from 1c113a1 to 2173fa1 Compare December 13, 2019 10:57

daisy-ycguo force-pushed the trigger branch from 2173fa1 to 703def5 Compare December 13, 2019 11:17

knative-prow-robot reviewed Dec 13, 2019

View reviewed changes

daisy-ycguo force-pushed the trigger branch from 703def5 to a53d09c Compare December 13, 2019 13:27

rhuss suggested changes Dec 13, 2019

View reviewed changes

daisy-ycguo force-pushed the trigger branch from a53d09c to fee3021 Compare December 16, 2019 07:31

Add command for creation of a plain trigger

ec0a8d8

daisy-ycguo force-pushed the trigger branch from fee3021 to ec0a8d8 Compare December 16, 2019 09:50

rhuss reviewed Dec 16, 2019

View reviewed changes

rhuss approved these changes Dec 16, 2019

View reviewed changes

knative-prow-robot assigned rhuss Dec 16, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019

knative-prow-robot merged commit 7def9f4 into knative:master Dec 16, 2019

dsimansk added a commit to dsimansk/client that referenced this pull request Nov 29, 2024

[release-v1.16] Fix image generator and builds (knative#541)

17b0730

Conversation

daisy-ycguo commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

daisy-ycguo commented Dec 12, 2019

Uh oh!

sixolet commented Dec 12, 2019

Uh oh!

sixolet commented Dec 12, 2019

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!

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!

daisy-ycguo Dec 13, 2019 • 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!

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!

rhuss Dec 13, 2019 • 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!

daisy-ycguo commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daisy-ycguo commented Dec 13, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo commented Dec 9, 2019 •

edited

Loading

daisy-ycguo Dec 13, 2019 •

edited

Loading

rhuss Dec 13, 2019 •

edited

Loading

daisy-ycguo commented Dec 13, 2019 •

edited

Loading