Update ApiServerSource by n3wscott · Pull Request #1175 · knative/eventing

n3wscott · 2019-05-07T22:11:05Z

Proposed Changes

do not use a controller to watch each type.
adds mode to ApiServerSource:
- Ref (how it was)
- Resource (full object contents.)

Release Note

Adds spec.mode to ApiServerSource.

nachocano · 2019-05-07T23:12:04Z

/cc @nachocano

n3wscott · 2019-05-08T17:57:48Z

Follow-up to add event registry types.

n3wscott · 2019-05-08T17:58:12Z

/assign @lionelvillard

knative-prow-robot · 2019-05-08T17:58:13Z

@n3wscott: GitHub didn't allow me to assign the following users: lionelvillard.

Note that only knative members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @lionelvillard

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.

nachocano · 2019-05-08T18:00:34Z

Follow-up to add event registry types.

I have a working branch, need to properly set up the source and add some UTs.

n3wscott · 2019-05-08T18:33:50Z

/assign @vaikas-google

n3wscott · 2019-05-08T18:54:20Z

knative/eventing-contrib#404 removes the old k8s source.

vaikas · 2019-05-08T22:26:42Z

-
 		// This is really bad.
-		gvr, _ := meta.UnsafeGuessKindToResource(gvk)
+		gvr, _ := meta.UnsafeGuessKindToResource(schema.GroupVersionKind{Kind: kind, Group: gv.Group, Version: gv.Version})


re, the comment above, is there a TODO here?

vaikas · 2019-05-08T23:10:20Z

+	go func() {
+		err = a.Start(stopCh)
+	}()
+	time.Sleep(1 * time.Millisecond)


is this just to make sure we yield the processor? Perhaps use runtime.GoSched() instead?

it turns out the test does not work, I am not sure how to get the cache to do it's thing so I am going to skip the test with a TODO for now.

vaikas · 2019-05-08T23:18:42Z

+	if len(a.controlledGVRs) > 0 {
+		object := obj.(*unstructured.Unstructured)
+		gvk := object.GroupVersionKind()
+		// This is really bad.


it is the tragedy of kubernetes.

vaikas · 2019-05-08T23:23:02Z

+	// Mode is the mode the receive adapter controller runs under: Ref or Resource.
+	// `Ref` sends only the reference to the resource.
+	// `Resource` send the full resource.
+	Mode string `json:"mode,omitempty"`


Shouldn't this be validated somewhere?

it defaults to ref if set to a random thing, see the Trash test.

Yes, but it would be better if we could validate it when the resource is created.

Ah got it. How about we do a followup to wire in the event sources into the webhook and implement the validatable stuff?

…icely.

lionelvillard · 2019-05-09T13:35:26Z

+			ListFunc:  asUnstructuredLister(a.k8s.Resource(gvrc.GVR).Namespace(a.namespace).List),
+			WatchFunc: asUnstructuredWatcher(a.k8s.Resource(gvrc.GVR).Namespace(a.namespace).Watch),
+		}
+		informer = cache.NewSharedIndexInformer(lw, &unstructured.Unstructured{}, resyncPeriod, cache.Indexers{


@n3wscott have you considered getting rid of the informer altogether and use only Reflector?

This is a great idea. Thanks! Worked it in and updated.

My understanding is that the informer will remove some possible duplicate information and give some serialization for us, but mayhaps for what we're doing here it's fine.

knative-metrics-robot · 2019-05-09T15:33:07Z

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
pkg/adapter/apiserver/adapter.go	Do not exist	76.7%
pkg/adapter/apiserver/events/events.go	Do not exist	95.1%
pkg/adapter/apiserver/ref.go	Do not exist	83.3%
pkg/adapter/apiserver/resource.go	Do not exist	80.6%
pkg/apis/sources/v1alpha1/apiserver_types.go	Do not exist	0.0%
pkg/reconciler/apiserversource/resources/receive_adapter.go	Do not exist	100.0%

vaikas · 2019-05-09T16:26:26Z

/approve

knative-prow-robot · 2019-05-09T16:26:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, vaikas-google

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~~ [n3wscott,vaikas-google]

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

vaikas · 2019-05-09T16:26:59Z

Leaving lgtm to @lionelvillard since he too had some comments about it so don't want to merge underneath...

lionelvillard · 2019-05-09T18:26:08Z

it's working for me.

/lgtm

knative-prow-robot · 2019-05-09T18:26:20Z

@lionelvillard: changing LGTM is restricted to assignees, and only knative/eventing repo collaborators may be assigned issues.

Details

In response to this:

it's working for me.

/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.

nachocano · 2019-05-09T18:29:16Z

it's working for me.

/lgtm

let me try, apparently your lgtm didn't make it...

/lgtm

* working through using the informer directly. * getting closer.: * working. * cleanup. * Working with controller flag. * add todo. * newline. * fix the test. * fix format. * adding tests for event generation. * Adding tests for ref and resource adapter event emitters. * add adapter tests. * trying to fix the tests but the cache is not playing with the fakes nicely. * update lock. * Use cache reflector. * fix race.

Scott Nichols added 7 commits May 6, 2019 16:14

working through using the informer directly.

b0e4bb1

getting closer.:

47b3776

Merge branch 'master' into k8s_source

a54db53

working.

bd88076

cleanup.

a9d7162

Working with controller flag.

867c01b

add todo.

6ce8e88

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 May 7, 2019

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

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 7, 2019

knative-prow-robot requested review from matzew and vaikas May 7, 2019 22:11

newline.

e858e4b

knative-prow-robot requested a review from nachocano May 7, 2019 23:12

Scott Nichols added 3 commits May 7, 2019 18:05

fix the test.

37a05ab

fix format.

f8d463c

adding tests for event generation.

5f2a78b

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

Scott Nichols added 2 commits May 8, 2019 10:19

Adding tests for ref and resource adapter event emitters.

7d46a6e

add adapter tests.

efc01aa

n3wscott changed the title ~~[WIP] Update ApiServerSource~~ Update ApiServerSource May 8, 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 May 8, 2019

knative-prow-robot assigned vaikas May 8, 2019

n3wscott mentioned this pull request May 8, 2019

Delete KubernetesEventSource, replaced by ApiServerSource knative/eventing-contrib#404

Merged

vaikas reviewed May 8, 2019

View reviewed changes

trying to fix the tests but the cache is not playing with the fakes n…

cf73f39

…icely.

lionelvillard reviewed May 9, 2019

View reviewed changes

Scott Nichols added 3 commits May 9, 2019 07:28

update lock.

21210d5

Use cache reflector.

19c0ca9

fix race.

91da462

n3wscott mentioned this pull request May 9, 2019

Include sources.eventing.knative.dev in validation webhook. #1196

Closed

knative-prow-robot assigned nachocano May 9, 2019

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

knative-prow-robot merged commit fd45b55 into knative:master May 9, 2019

n3wscott mentioned this pull request May 10, 2019

KubernetesSource has been deleted, update docs to use ApiServerSource knative/docs#1301

Closed

evankanderson mentioned this pull request Oct 9, 2019

Cleanup receive adapter main code. #2016

Merged

lionelvillard mentioned this pull request Dec 12, 2019

adding lionelvillard to approvers #2314

Merged

Conversation

n3wscott commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

nachocano commented May 7, 2019

Uh oh!

n3wscott commented May 8, 2019

Uh oh!

n3wscott commented May 8, 2019

Uh oh!

knative-prow-robot commented May 8, 2019

Uh oh!

nachocano commented May 8, 2019

Uh oh!

n3wscott commented May 8, 2019

Uh oh!

n3wscott commented May 8, 2019

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!

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!

knative-metrics-robot commented May 9, 2019

Uh oh!

vaikas commented May 9, 2019

Uh oh!

knative-prow-robot commented May 9, 2019

Uh oh!

vaikas commented May 9, 2019

Uh oh!

lionelvillard commented May 9, 2019

Uh oh!

knative-prow-robot commented May 9, 2019

Uh oh!

nachocano commented May 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

n3wscott commented May 7, 2019 •

edited

Loading