Skip to content

API + Reconciler for CronJobSource.#1092

Merged
knative-prow-robot merged 21 commits into
knative:masterfrom
n3wscott:cronjobsource_rec
Apr 25, 2019
Merged

API + Reconciler for CronJobSource.#1092
knative-prow-robot merged 21 commits into
knative:masterfrom
n3wscott:cronjobsource_rec

Conversation

@n3wscott
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott commented Apr 23, 2019 •
edited
Loading

Related to #734 and knative/eventing-contrib#341

Proposed Changes

  • Move CronJobSource api from knative/eventing-sources to knative/eventing
  • Move and re-implement cron job source reconciler.

Release Note

CronJobSource CRD will now be installed from knative/eventing.
CronJobSource will be reconciled by eventing.

@knative-prow-robot 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 Apr 23, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 23, 2019
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 23, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Apr 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott

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:

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 knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2019
@n3wscott n3wscott mentioned this pull request Apr 24, 2019
Closed
@n3wscott n3wscott changed the title [WIP] Reconciler for CronJobSource. API + Reconciler for CronJobSource. Apr 24, 2019
@knative-prow-robot 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 Apr 24, 2019
@n3wscott n3wscott mentioned this pull request Apr 24, 2019
Closed
@n3wscott n3wscott mentioned this pull request Apr 24, 2019
Merged
@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 24, 2019

/test pull-knative-eventing-integration-tests

whynowy
whynowy reviewed Apr 24, 2019
View reviewed changes
Comment thread pkg/apis/sources/v1alpha1/cron_job_lifecycle_test.go Outdated
Comment thread pkg/apis/sources/v1alpha1/register_test.go Outdated
@n3wscott n3wscott mentioned this pull request Apr 24, 2019
Closed
vaikas
vaikas reviewed Apr 24, 2019
View reviewed changes
Comment thread cmd/controller/main.go Outdated
@n3wscott n3wscott force-pushed the cronjobsource_rec branch from 83a37eb to db65b15 Compare April 24, 2019 20:28
Scott Nichols added 2 commits April 24, 2019 13:31
@n3wscott n3wscott force-pushed the cronjobsource_rec branch from 7393530 to 409faf7 Compare April 24, 2019 20:39
@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Apr 24, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/adapter/cronjobevents/adapter.go Do not exist 89.3%
pkg/apis/sources/v1alpha1/cron_job_lifecycle.go Do not exist 100.0%
pkg/apis/sources/v1alpha1/cron_job_types.go Do not exist 0.0%
pkg/apis/sources/v1alpha1/register.go Do not exist 100.0%

matzew
matzew reviewed Apr 25, 2019
View reviewed changes
Comment thread config/200-controller-clusterrole.yaml
resources:
- "cronjobsources"
- "cronjobsources/status"
- "cronjobsources/finalizers"
Copy link
Copy Markdown
Member

@matzew matzew Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

matzew
matzew reviewed Apr 25, 2019
View reviewed changes
Comment thread pkg/apis/duck/v1alpha1/subscribable_types.go
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copyright 2019 The Knative Authors
Copy link
Copy Markdown
Member

@matzew matzew Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a lawyer: but 2018, 2019 (or 2018-2019) instead ?

matzew
matzew reviewed Apr 25, 2019
View reviewed changes
Comment thread config/200-controller-clusterrole.yaml
- "events"
- "serviceaccounts"
verbs:
verbs: &everything
Copy link
Copy Markdown
Member

@matzew matzew Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this

@matzew
Copy link
Copy Markdown
Member

matzew commented Apr 25, 2019

will the https://github.com/n3wscott/eventing-sources/tree/delete_moved_cron/samples/cronjob-source be moved to the docs repo for this ?

@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 25, 2019

will the https://github.com/n3wscott/eventing-sources/tree/delete_moved_cron/samples/cronjob-source be moved to the docs repo for this ?

that is the plan. which is why the followup delete pr did not remove them yet. I will make that clear in the delete pr.

@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 25, 2019

/assign @vaikas-google

@matzew
Copy link
Copy Markdown
Member

matzew commented Apr 25, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2019
@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 25, 2019

/assign @matzew

@knative-prow-robot knative-prow-robot merged commit 47e3de9 into knative:master Apr 25, 2019
@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 25, 2019

Related to knative/docs#1235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matzew matzew matzew left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

+2 more reviewers

@whynowy whynowy whynowy left review comments

@vaikas vaikas vaikas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@matzew matzew

@vaikas vaikas

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@n3wscott @knative-prow-robot @knative-metrics-robot @matzew @whynowy @vaikas @googlebot