Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Eventing Sources API to v1 #1299

Merged
merged 7 commits into from
May 3, 2021

Conversation

dsimansk
Copy link
Contributor

Description

Follow-up on recent v1alpha1 removal from Eventing main.
knative/eventing#5318

Changes

  • Update to APIServerSource, ContainerSource, PingSource & SinkBinding API to sources/v1

/cc @knative/client-reviewers

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 30, 2021
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@dsimansk: 0 warnings.

In response to this:

Description

Follow-up on recent v1alpha1 removal from Eventing main.
knative/eventing#5318

Changes

  • Update to APIServerSource, ContainerSource, PingSource & SinkBinding API to sources/v1

/cc @knative/client-reviewers

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.

@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 30, 2021
@dsimansk
Copy link
Contributor Author

dsimansk commented Apr 30, 2021

There's one open question though. PingSource was promoted to v1 API just now, that's why integration tests are failing currently. For that reason we can't use PingSource v1 on 0.22 as well, breaking the backward compatibility, therefore we need to keep PingSource at v1beta2 for now. (applying the common client API policy keeping the lowest version until available)

@rhuss @maximilien any objections to the above? I'm mainly asking due to implications for 1.0 release etc.

@rhuss
Copy link
Contributor

rhuss commented Apr 30, 2021

Yeah, let's stay with PingSource v1beta2.

If Eventing goes to Knative 1.0 with v1beta2, too, I don't see any problems to use this for a 1.0 client.

@rhuss
Copy link
Contributor

rhuss commented Apr 30, 2021

@lionelvillard Are there any plans for Evention 1.0 to remove PingSource v1beta2, too ?

@lionelvillard
Copy link
Member

@lionelvillard Are there any plans for Evention 1.0 to remove PingSource v1beta2, too ?

We have to keep v1beta2 for at least of couple of releases.

@dsimansk
Copy link
Contributor Author

/retest


"knative.dev/client/pkg/util"
)

func TestSimpleCreatePingSource(t *testing.T) {
mysvc := &servingv1.Service{
TypeMeta: v1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1"},
ObjectMeta: v1.ObjectMeta{Name: "mysvc", Namespace: "default"},
TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1beta2"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Serving v1beta2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Looks good, however while cross checking it looks like that there has been a gotcha when reverting from v1 back to v1beta2 for the ping source (see comment inline). Not sure if other parts are affected, too.

@rhuss
Copy link
Contributor

rhuss commented Apr 30, 2021

/retest

/lgtm
/approve

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 30, 2021
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

/ok-to-test
/lgtm

@knative-prow-robot knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 30, 2021
@rhuss
Copy link
Contributor

rhuss commented May 2, 2021

Looks like that the test indeed captured a valid issue:

source_list_test.go:85: assertion failed: 
        Actual output: NAME              TYPE         RESOURCE                          SINK   READY
        testpingsource0   PingSource   pingsources.sources.knative.dev          True
        
        Missing strings: ksvc:testsvc0

i.e. there is no output of the connected sink.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 2, 2021
@dsimansk
Copy link
Contributor Author

dsimansk commented May 2, 2021

Looks like that the test indeed captured a valid issue:

source_list_test.go:85: assertion failed: 
        Actual output: NAME              TYPE         RESOURCE                          SINK   READY
        testpingsource0   PingSource   pingsources.sources.knative.dev          True
        
        Missing strings: ksvc:testsvc0

i.e. there is no output of the connected sink.

Well, this one was pretty hard to find actually. 😓
The error is caused by failed unstructured conversion due to knative/eventing@7c0b477.

Update: adding more details. The above change/fix was backported to Eventing patch release 0.22.1. Therefore the next client release will fully work with v1beta2 PingSource and Eventing 0.22.1 combination.
The only issue is with 0.22.0 and kn source list - general source, list that uses Unstructured to list all present sources, doesn't display sink reference (or further details with -o yaml, etc.), but kn source ping describe and everything else related to PingSource CRUD operations is OK.

@knative-metrics-robot
Copy link

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/kn/commands/types.go 55.1% 57.8% 2.7
pkg/sources/common.go Do not exist 100.0%
pkg/sources/v1/apiserver_client.go Do not exist 90.0%
pkg/sources/v1/apiserver_client_mock.go Do not exist 77.3%
pkg/sources/v1/binding_client.go Do not exist 84.7%
pkg/sources/v1/binding_client_mock.go Do not exist 90.9%
pkg/sources/v1/client.go Do not exist 100.0%
pkg/sources/v1/container_client.go Do not exist 89.5%
pkg/sources/v1/container_client_mock.go Do not exist 90.9%
pkg/sources/v1beta2/client.go Do not exist 100.0%
pkg/sources/v1beta2/ping_client.go Do not exist 90.0%
pkg/sources/v1beta2/ping_client_mock.go Do not exist 77.3%

@rhuss
Copy link
Contributor

rhuss commented May 3, 2021

The only issue with 0.22.0 ...

That's fine I would say, as we can always say that's an issue with the backend and the fix is, to update the eventing backend to 0.22.1

@rhuss
Copy link
Contributor

rhuss commented May 3, 2021

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 merged commit e5b19e1 into knative:main May 3, 2021
@dsimansk dsimansk deleted the pr/sources-v1 branch May 3, 2021 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants