feature(source cronjobs): Implementation of CronJobSource management #542

rhuss · 2019-12-09T18:33:05Z

Contains create/delete/update/describe but not tests yet.

Still todo:

Think how to provide a namespace for the sink
Support for more sinks
Synchronous mode for create & update
Add list (or implement "source list")

googlebot · 2019-12-09T18:33:09Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

rhuss · 2019-12-09T18:33:51Z

Still todo: Adding tests.

This PR is based on #499 which should be merged first.

navidshaikh

Neat!

pkg/kn/commands/source/cronjob/client.go

pkg/kn/commands/source/cronjob/cronjob.go

pkg/kn/commands/source/cronjob/create.go

pkg/kn/commands/source/cronjob/delete.go

pkg/kn/commands/source/cronjob/describe.go

googlebot · 2019-12-10T13:00:40Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

daisy-ycguo

I see you add eventing server side invocation methods here pkg/kn/commands/source/cronjob/client.go. Just as knative serving API invocations go to pkg/serving, I think all eventing server side invocation methods shall go to pkg/eventing. Because some day, we may want to expose these server side invocations as a client-go library. Forgive me if I'm wrong. I think it's one of team decisions.

daisy-ycguo · 2019-12-10T14:27:03Z

docs/cmd/kn_source_cronjob.md

+### SEE ALSO
+
+* [kn source](kn_source.md)	 - Event Source command group
+* [kn source cronjob create](kn_source_cronjob_create.md)	 - Create a Crontab scheduler source.


Some use Crontab scheduler source and some use CronJob source. I think it's better to use a consistent word.

Just as knative serving API invocations go to pkg/serving, I think all eventing server side invocation methods shall go to pkg/eventing. Because some day, we may want to expose these server side invocations as a client-go library. Forgive me if I'm wrong. I think it's one of team decisions.

I might have missed that discussion, do you have some pointers for me ?

The real design decision is whether we want to make the sources support self-contained and all under one package (which make it easier to refactor e.g. when they will be moved away from eventing proper), or whether we want expose them as API and maintaining it (in that case we should build up the same structure as for serving).

I personally tend to the 'self-contained' approach for sources, but would also be happy to go over eventing pkg.

But yeah, probably better to be consistent. Let me change this later. thanks

pkg/kn/commands/source/source.go

daisy-ycguo · 2019-12-11T02:56:11Z

@rhuss Here is a short discussion about sdk: #375. I would strongly suggest that we put all server (serving+eventing) side functions together in a separated folder of command lines.

For end users, it will be very hard for them to use Knative API directly because of the complex K8s CRD types. Knative needs a sdk-go for end users to manage the server side objects, using simple type parameters, not complicated K8s types. In the future, when community decide to have a knative sdk-go, Knative Client could easily separate the server side methods and the command lines, and easily create the basic version of knative sdk-go.

rhuss · 2019-12-11T10:49:50Z

For end users, it will be very hard for them to use Knative API directly because of the complex K8s CRD types. Knative needs a sdk-go for end-users to manage the server-side objects, using simple type parameters, not complicated K8s types. In the future, when community decide to have a knative sdk-go, Knative Client could easily separate the server side methods and the command lines, and easily create the basic version of knative sdk-go.

Even our API is using "complex CRD types", actually the same as the Knative client SDKs. The benefit of a client-side SDK is not so much hiding the Knative types (there are already a simplification over the standard K8s CRs), but to provide extra, client-related features like:

CRD version alignment to also work with older clusters (still to implement, currently we send v1alpha1 objects to server but we could adapt this depending on the cluster's version).
Some syntactic sugar, like creating the client for a specific namespace or some easy way to provide list options.

So not really much (except maybe the version negotiation to come which is probably quite helpful for supporting multiple cluster versions), but I'm not sure if this is what you have in mind. How would a kn based Go library much simpler to use than the generated sdk libraries from knative-serving and knative-eventing that kn is using internally? Do you have an example in mind? If you mean to translate all CRDs field to method arguments, I would be strongly against this. Nobody wants methods with 20+ arguments. Maybe we could help with a kind of builder patterns, but at the end, complexity just does not go simply away.

googlebot · 2019-12-11T17:03:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-12-11T17:50:45Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Contains create/delete/update/describe but not tests yet. Still todo: * Think how to provide a namespace for the sink * Support for more sinks * Synchronous mode for create & update * Add list (or implement "source list")

rhuss · 2019-12-13T06:35:52Z

/lint

knative-prow-robot

@rhuss: 34 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/source/cronjob/create.go

knative-prow-robot · 2019-12-13T06:36:03Z

pkg/eventing/sources/v1alpha1/apiserver_client.go

+	kn_errors "knative.dev/client/pkg/errors"
+)
+
+// Interface for working with ApiServer sources


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

knative-prow-robot · 2019-12-13T06:36:03Z

pkg/eventing/sources/v1alpha1/apiserver_client.go

+)
+
+// Interface for working with ApiServer sources
+type KnApiServerSourcesClient interface {


Golint naming: type KnApiServerSourcesClient should be KnAPIServerSourcesClient. More info.

knative-prow-robot · 2019-12-13T06:36:03Z

pkg/eventing/sources/v1alpha1/apiserver_client.go

+}
+
+// NewKnSourcesClient is to invoke Eventing Sources Client API to create object
+func newKnApiServerSourcesClient(client client_v1alpha1.ApiServerSourceInterface, namespace string) KnApiServerSourcesClient {


Golint naming: func newKnApiServerSourcesClient should be newKnAPIServerSourcesClient. More info.

knative-prow-robot · 2019-12-13T06:36:03Z

pkg/eventing/sources/v1alpha1/apiserver_client.go

+}
+
+//CreateApiServerSource is used to create an instance of ApiServerSource
+func (c *apiServerSourcesClient) CreateApiServerSource(apisvrsrc *v1alpha1.ApiServerSource) (*v1alpha1.ApiServerSource, error) {


Golint naming: method CreateApiServerSource should be CreateAPIServerSource. More info.

knative-prow-robot · 2019-12-13T06:36:05Z

pkg/eventing/sources/v1alpha1/cronjob_client_mock.go

+	return mock.ErrorOrNil(call.Result[0])
+}
+
+// Delete CronJob


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

pkg/eventing/sources/v1alpha1/cronjob_client_mock.go

knative-prow-robot · 2019-12-13T06:36:05Z

pkg/eventing/sources/v1alpha1/cronjob_client_mock.go

+	return c.recorder.r.Namespace()
+}
+
+// Create CronJob


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

knative-prow-robot · 2019-12-13T06:36:05Z

pkg/eventing/sources/v1alpha1/cronjob_client_mock.go

+	return mock.ErrorOrNil(call.Result[0])
+}
+
+// Get CronJob


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

knative-prow-robot · 2019-12-13T06:36:05Z

pkg/kn/commands/source/cronjob/update.go

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


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

knative-metrics-robot · 2019-12-13T08:02:21Z

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/sources/v1alpha1/apiserver_client.go	Do not exist	62.5%
pkg/eventing/sources/v1alpha1/client.go	62.5%	100.0%	37.5
pkg/eventing/sources/v1alpha1/cronjob_client.go	Do not exist	100.0%
pkg/eventing/sources/v1alpha1/cronjob_client_mock.go	Do not exist	89.5%
pkg/kn/commands/source/cronjob/create.go	Do not exist	92.0%
pkg/kn/commands/source/cronjob/cronjob.go	Do not exist	57.1%
pkg/kn/commands/source/cronjob/delete.go	Do not exist	85.7%
pkg/kn/commands/source/cronjob/describe.go	Do not exist	83.7%
pkg/kn/commands/source/cronjob/flags.go	Do not exist	100.0%
pkg/kn/commands/source/cronjob/update.go	Do not exist	75.8%

daisy-ycguo

lgtm

navidshaikh

/lgtm

navidshaikh · 2019-12-13T09:23:19Z

pkg/eventing/sources/v1alpha1/apiserver_client.go

+type KnApiServerSourcesClient interface {
+
+	// Get an ApiServerSource by object
+	CreateApiServerSource(apisvrsrc *v1alpha1.ApiServerSource) (*v1alpha1.ApiServerSource, error)


FYI - In the apiserver update PR I am preparing, I've updated this function definition to only return the error as the created object is not used (only used in the AddReactor styles tests and object is used to check the name and namespace).

navidshaikh · 2019-12-13T09:27:06Z

pkg/kn/commands/types.go

-// prepareRestConfig returns REST config, which can be to use to create specific clientset
-func (params *KnParams) prepareRestConfig() (*rest.Config, error) {
+// RestConfig returns REST config, which can be to use to create specific clientset
+func (params *KnParams) RestConfig() (*rest.Config, error) {
 	var err error


knative-prow-robot · 2019-12-13T09:27:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

…ests reporting (knative#542) * add prow job for flaky test reporting * updates for PR comment * rename to flakes-reporter * name github and slack token more specific rather than general names

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

knative-prow-robot requested review from mattmoor and navidshaikh December 9, 2019 18:33

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 9, 2019

rhuss added kind/feature New feature or request topic/eventing-mvp labels Dec 9, 2019

rhuss force-pushed the pr/cronjob-source branch 2 times, most recently from a6f2bb1 to 6b517f8 Compare December 10, 2019 08:36

navidshaikh reviewed Dec 10, 2019

View reviewed changes

rhuss mentioned this pull request Dec 10, 2019

kn source cronjob create - builtin support for cronjob source (typed) #357

Closed

rhuss force-pushed the pr/cronjob-source branch from 6b517f8 to b851305 Compare December 10, 2019 13:00

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Dec 10, 2019

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

rhuss mentioned this pull request Dec 10, 2019

kn source cronjob - manage CronJobSources #515

Closed

4 tasks

daisy-ycguo reviewed Dec 10, 2019

View reviewed changes

rhuss force-pushed the pr/cronjob-source branch from a71d4f5 to a0397bf Compare December 11, 2019 17:03

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Dec 11, 2019

knative-prow-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 11, 2019

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

rhuss force-pushed the pr/cronjob-source branch from a0397bf to 9284399 Compare December 11, 2019 17:50

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Dec 11, 2019

rhuss force-pushed the pr/cronjob-source branch 3 times, most recently from fea1da2 to 7e416d0 Compare December 11, 2019 19:47

rhuss added 5 commits December 12, 2019 14:29

chore: Add a mock test for cronjob source

c8c72ba

addd missing license headers

c4426c0

cosmetic changes

0250022

chore(cronjobsource): Add tests

69f443d

rhuss force-pushed the pr/cronjob-source branch from 435d9fd to 69f443d Compare December 12, 2019 13:29

rhuss added 2 commits December 12, 2019 16:05

fixup

3d449df

chore(cronjob source): Improved test coverage

5b6cdf4

knative-prow-robot reviewed Dec 13, 2019

View reviewed changes

chore(cronjob source): some review polishing

9436c35

daisy-ycguo reviewed Dec 13, 2019

View reviewed changes

navidshaikh approved these changes Dec 13, 2019

View reviewed changes

knative-prow-robot assigned navidshaikh Dec 13, 2019

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

knative-prow-robot merged commit 0ca6f14 into knative:master Dec 13, 2019

dsimansk added a commit to dsimansk/client that referenced this pull request Dec 8, 2020

Update kn-plugin-source-kafka to v0.18.0 (knative#542)

290f96b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(source cronjobs): Implementation of CronJobSource management #542

feature(source cronjobs): Implementation of CronJobSource management #542

rhuss commented Dec 9, 2019 •

edited

Loading

googlebot commented Dec 9, 2019

rhuss commented Dec 9, 2019

navidshaikh left a comment

googlebot commented Dec 10, 2019

daisy-ycguo left a comment

daisy-ycguo Dec 10, 2019

rhuss Dec 10, 2019

rhuss Dec 10, 2019

daisy-ycguo commented Dec 11, 2019 •

edited

Loading

rhuss commented Dec 11, 2019 •

edited

Loading

googlebot commented Dec 11, 2019

googlebot commented Dec 11, 2019

rhuss commented Dec 13, 2019

knative-prow-robot left a comment

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-prow-robot Dec 13, 2019

knative-metrics-robot commented Dec 13, 2019

daisy-ycguo left a comment

navidshaikh left a comment

navidshaikh Dec 13, 2019

navidshaikh Dec 13, 2019

knative-prow-robot commented Dec 13, 2019

feature(source cronjobs): Implementation of CronJobSource management #542

feature(source cronjobs): Implementation of CronJobSource management #542

Conversation

rhuss commented Dec 9, 2019 • edited Loading

googlebot commented Dec 9, 2019

rhuss commented Dec 9, 2019

navidshaikh left a comment

Choose a reason for hiding this comment

googlebot commented Dec 10, 2019

daisy-ycguo 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

daisy-ycguo commented Dec 11, 2019 • edited Loading

rhuss commented Dec 11, 2019 • edited Loading

googlebot commented Dec 11, 2019

googlebot commented Dec 11, 2019

rhuss commented Dec 13, 2019

knative-prow-robot 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

knative-metrics-robot commented Dec 13, 2019

daisy-ycguo left a comment

Choose a reason for hiding this comment

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

knative-prow-robot commented Dec 13, 2019

rhuss commented Dec 9, 2019 •

edited

Loading

daisy-ycguo commented Dec 11, 2019 •

edited

Loading

rhuss commented Dec 11, 2019 •

edited

Loading