ApiServerSource serve v1alpha2 #2872

n3wscott · 2020-03-30T22:34:08Z

There is a very big API change between APIServerSource v1alpha1 and v1alpha2. Because of this we are removing the implementation of v1alpha1 and only use v1alpha2. Conversion between v1alpha1 and v1alpha2 is lossy so we are going to still store at v1alpha1, but the deprecated fields in v1alpha1 are ignored.

Proposed Changes

Serve APIServerSource v1alpha2.
Only reconcile APIServerSource as v1alpha2 resources.
Apply Adapter v2 to APIServerSource Receive Adapter.

Release Note

- Adding APIServerSource v1alpha2, APIServerSource is reconciled and realized only in the v1alpha2 API, some fields are now ignored, please use v1alpha2.

Docs

Document ApiServerSource v1alpha2 and delete all v1alpha1 examples. docs#2352

knative-prow-robot · 2020-03-30T22:34:24Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [n3wscott]

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

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/adapter/apiserver/adapter_test.go

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/apis/sources/v1alpha2/apiserver_types.go

lionelvillard · 2020-04-01T14:23:00Z

pkg/apis/sources/v1alpha1/apiserver_types.go

@@ -132,8 +126,6 @@ type ApiServerResource struct {

 	// LabelSelector restricts this source to objects with the selected labels
 	// More info: http://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors
-	// Deprecated: Per-resource label selectors will no longer be supported in
-	// v1alpha2, please use Spec.LabelSelector.
 	LabelSelector metav1.LabelSelector `json:"labelSelector"`

 	// ControllerSelector restricts this source to objects with a controlling owner reference of the specified kind.


I agree with removing ControllerSelector (redundant). Not so sure about deprecating Controller as it has a real use case. k8s sample controller does it sightly differently (see https://github.com/kubernetes/sample-controller/blob/041853156b367ba768df5cda747e653dd98a7127/controller.go#L370) by getting the controlling owner and filtering by GVK.

I am filtering by gvk, but the owner gvk comes from spec. If you need both resources that are owned and unowned, it would be two resources.

I think next pass is to mt the source in a namespace for the same service account.

pkg/adapter/apiserver/adapter.go

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

n3wscott · 2020-04-01T14:59:31Z

/hold

no selector on the Owner... fixing.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

n3wscott · 2020-04-01T15:31:17Z

selector is gone from owner GVK and panics are removed.

/unhold

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor · 2020-04-01T15:49:17Z

pkg/reconciler/apiserversource/resources/receive_adapter_test.go

@@ -18,74 +18,52 @@ package resources

 import (
 	"fmt"


Format Go code:

Suggested change

"fmt"

"fmt"

"testing"

mattmoor · 2020-04-01T15:49:17Z

pkg/reconciler/apiserversource/resources/receive_adapter_test.go

@@ -18,74 +18,52 @@ package resources

 import (
 	"fmt"
+	"knative.dev/pkg/kmeta"
 	"testing"


Format Go code:

Suggested change

"testing"

knative-metrics-robot · 2020-04-01T18:32:16Z

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

File	Old Coverage	New Coverage	Delta
pkg/adapter/apiserver/adapter.go	77.0%	76.9%	-0.1
pkg/adapter/apiserver/adapter_injection.go	Do not exist	76.9%
pkg/adapter/apiserver/delegate.go	Do not exist	88.9%
pkg/adapter/apiserver/events/events.go	95.2%	95.7%	0.5
pkg/adapter/apiserver/filter.go	Do not exist	66.7%
pkg/apis/sources/v1alpha1/apiserver_conversion.go	92.2%	96.2%	4.0
pkg/reconciler/apiserversource/apiserversource.go	84.2%	75.2%	-9.0
pkg/reconciler/apiserversource/resources/receive_adapter.go	100.0%	83.3%	-16.7

vaikas · 2020-04-01T21:21:59Z

config/core/resources/apiserversource.yaml

@@ -79,5 +79,5 @@ spec:
    served: true
    storage: true
  - name: v1alpha2
-    served: false
+    served: true
    storage: false


I'm confused between this and the comments. Shouldn't this be flipped to storage = true?

it is stored as v1alpha1 still.

it is lossy so I think we have to do that manual upgrade touch thing. So I am leaving it as v1alpha1 stored until we fix this issue as a whole

lionelvillard · 2020-04-01T21:31:04Z

/lgtm

The missing controller filter can always be added later if needed.

Scott Nichols added 4 commits March 30, 2020 14:44

rework adapter.

6b4a8a9

generate api for v1alpha2

8c2b0a0

fix up the args.

09876b9

Merge branch 'master' into apiserversource-serve-v1alpha2

1a7b928

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 Mar 30, 2020

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

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

mattmoor reviewed Mar 30, 2020

View reviewed changes

knative-prow-robot requested review from grantr and lionelvillard March 30, 2020 22:34

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2020

mattmoor reviewed Mar 30, 2020

View reviewed changes

marshal the config

fede527

mattmoor reviewed Mar 30, 2020

View reviewed changes

Scott Nichols added 2 commits March 30, 2020 17:16

working apiserversource v1alpha2

d55eaaa

lint

473c053

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Mar 31, 2020

mattmoor reviewed Mar 31, 2020

View reviewed changes

n3wscott mentioned this pull request Mar 31, 2020

Document ApiServerSource v1alpha2 and delete all v1alpha1 examples. knative/docs#2352

Closed

skip tests for now. move to context based client.

222a0b4

mattmoor reviewed Mar 31, 2020

View reviewed changes

n3wscott commented Mar 31, 2020

View reviewed changes

pkg/adapter/apiserver/adapter_test.go Outdated Show resolved Hide resolved

move label selector back to resource and out of parent.

ae1070c

mattmoor reviewed Mar 31, 2020

View reviewed changes

lint

140980f

mattmoor reviewed Mar 31, 2020

View reviewed changes

headers

28fde03

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 Mar 31, 2020

missed the namespace.

58dc465

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2020

lionelvillard reviewed Apr 1, 2020

View reviewed changes

pkg/apis/sources/v1alpha2/apiserver_types.go Outdated Show resolved Hide resolved

lionelvillard reviewed Apr 1, 2020

View reviewed changes

pkg/adapter/apiserver/adapter.go Show resolved Hide resolved

n3wscott mentioned this pull request Apr 1, 2020

ApiServerSource does not seem to work with cluster scoped resources. #2892

Closed

Merge branch 'master' into apiserversource-serve-v1alpha2

1ac195d

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2020

mattmoor reviewed Apr 1, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

no selector on owner.

de423df

mattmoor reviewed Apr 1, 2020

View reviewed changes

remove panic.

8ae1d77

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

mattmoor reviewed Apr 1, 2020

View reviewed changes

fix e2e test schema

7d6101f

mattmoor reviewed Apr 1, 2020

View reviewed changes

Scott Nichols added 4 commits April 1, 2020 08:52

lint

7767744

Merge branch 'master' into apiserversource-serve-v1alpha2

80a2162

add a nil

1283b84

no metrics.

4e1cd4b

vaikas reviewed Apr 1, 2020

View reviewed changes

knative-prow-robot assigned lionelvillard Apr 1, 2020

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

knative-prow-robot merged commit 62e741c into knative:master Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ApiServerSource serve v1alpha2 #2872

ApiServerSource serve v1alpha2 #2872

n3wscott commented Mar 30, 2020 •

edited

Loading

knative-prow-robot commented Mar 30, 2020

mattmoor left a comment

mattmoor left a comment

mattmoor left a comment

lionelvillard Apr 1, 2020

n3wscott Apr 1, 2020

mattmoor left a comment

n3wscott commented Apr 1, 2020

mattmoor left a comment

n3wscott commented Apr 1, 2020

mattmoor left a comment

mattmoor left a comment

mattmoor Apr 1, 2020

mattmoor Apr 1, 2020

knative-metrics-robot commented Apr 1, 2020

vaikas Apr 1, 2020

n3wscott Apr 1, 2020

n3wscott Apr 1, 2020

lionelvillard commented Apr 1, 2020

ApiServerSource serve v1alpha2 #2872

ApiServerSource serve v1alpha2 #2872

Conversation

n3wscott commented Mar 30, 2020 • edited Loading

Proposed Changes

knative-prow-robot commented Mar 30, 2020

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

lionelvillard Apr 1, 2020

Choose a reason for hiding this comment

n3wscott Apr 1, 2020

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

n3wscott commented Apr 1, 2020

mattmoor left a comment

Choose a reason for hiding this comment

n3wscott commented Apr 1, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor Apr 1, 2020

Choose a reason for hiding this comment

mattmoor Apr 1, 2020

Choose a reason for hiding this comment

knative-metrics-robot commented Apr 1, 2020

vaikas Apr 1, 2020

Choose a reason for hiding this comment

n3wscott Apr 1, 2020

Choose a reason for hiding this comment

n3wscott Apr 1, 2020

Choose a reason for hiding this comment

lionelvillard commented Apr 1, 2020

n3wscott commented Mar 30, 2020 •

edited

Loading