Skip to content

ApiServerSource serve v1alpha2#2872

Merged
knative-prow-robot merged 23 commits into
knative:masterfrom
n3wscott:apiserversource-serve-v1alpha2
Apr 1, 2020
Merged

ApiServerSource serve v1alpha2#2872
knative-prow-robot merged 23 commits into
knative:masterfrom
n3wscott:apiserversource-serve-v1alpha2

Conversation

@n3wscott
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott commented Mar 30, 2020 •
edited
Loading

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

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

knative-prow-robot commented Mar 30, 2020

[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 Mar 30, 2020
mattmoor
mattmoor reviewed Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

mattmoor
mattmoor reviewed Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Mar 31, 2020
@n3wscott n3wscott mentioned this pull request Mar 31, 2020
Closed
n3wscott
n3wscott commented Mar 31, 2020
View reviewed changes
Comment thread pkg/adapter/apiserver/adapter_test.go Outdated
mattmoor
mattmoor reviewed Mar 31, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

@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 Mar 31, 2020
@knative-prow-robot 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
lionelvillard reviewed Apr 1, 2020
View reviewed changes
Comment thread pkg/apis/sources/v1alpha2/apiserver_types.go Outdated
lionelvillard
lionelvillard reviewed Apr 1, 2020
View reviewed changes
Comment thread pkg/apis/sources/v1alpha1/apiserver_types.go
// 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.
Copy link
Copy Markdown
Contributor

@lionelvillard lionelvillard Apr 1, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Apr 1, 2020

Choose a reason for hiding this comment

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

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.

lionelvillard
lionelvillard reviewed Apr 1, 2020
View reviewed changes
Comment thread pkg/adapter/apiserver/adapter.go
@n3wscott n3wscott mentioned this pull request Apr 1, 2020
Closed
@knative-prow-robot 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
mattmoor reviewed Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 1, 2020

/hold

no selector on the Owner... fixing.

@knative-prow-robot 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
mattmoor
mattmoor reviewed Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Apr 1, 2020

selector is gone from owner GVK and panics are removed.

/unhold

@knative-prow-robot 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
mattmoor reviewed Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

mattmoor
mattmoor reviewed Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

Comment thread pkg/reconciler/apiserversource/resources/receive_adapter_test.go
@@ -18,74 +18,52 @@ package resources

import (
"fmt"
Copy link
Copy Markdown
Member

@mattmoor mattmoor Apr 1, 2020

Choose a reason for hiding this comment

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

Format Go code:

Suggested change
"fmt"
"fmt"
"testing"

Comment thread pkg/reconciler/apiserversource/resources/receive_adapter_test.go
import (
"fmt"
"knative.dev/pkg/kmeta"
"testing"
Copy link
Copy Markdown
Member

@mattmoor mattmoor Apr 1, 2020

Choose a reason for hiding this comment

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

Format Go code:

Suggested change
"testing"

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Apr 1, 2020

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
vaikas reviewed Apr 1, 2020
View reviewed changes
Comment thread config/core/resources/apiserversource.yaml
- name: v1alpha2
served: false
served: true
storage: false
Copy link
Copy Markdown
Contributor

@vaikas vaikas Apr 1, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Apr 1, 2020

Choose a reason for hiding this comment

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

it is stored as v1alpha1 still.

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Apr 1, 2020

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Contributor

lionelvillard commented Apr 1, 2020

/lgtm

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

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2020
@knative-prow-robot knative-prow-robot merged commit 62e741c into knative:master Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grantr grantr Awaiting requested review from grantr

3 more reviewers

@mattmoor mattmoor mattmoor left review comments

@lionelvillard lionelvillard lionelvillard left review comments

@vaikas vaikas vaikas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lionelvillard lionelvillard

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release 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 @lionelvillard @mattmoor @vaikas @googlebot