Apiserversource defaults conversion by capri-xiyue · Pull Request #3616 · knative/eventing

capri-xiyue · 2020-07-16T22:55:07Z

Proposed Changes

🎁 Adding ApiServerSource v1beta1 defaults and conversion.

None

capri-xiyue · 2020-07-16T22:55:53Z

/hold wait until #3614 gets merged and then rebase

capri-xiyue · 2020-07-16T22:56:44Z

/assign @nachocano @lionelvillard

capri-xiyue · 2020-07-16T23:04:27Z

The coverage is low is because pkg/apis/sources/v1alpha2/apiserver_conversion.go.
I don't add additional UT to it because all needed UT has been covered in pkg/apis/sources/v1alpha1/apiserver_conversion_test.go

capri-xiyue · 2020-07-17T00:00:42Z

Got kind of confused because the coding style when doing conversion is not quite consistent in the codebase.
For example in the codebase
Both CloudEventOverrides and ResourceOwner are pointer. Sometimes we use deep copy, sometimes we don't.
I thought as long as the object is a pointer or the struct contains pointer or array, we should use deep copy when doing conversion. But I checked the codebase, it's quite inconsistent. Not sure whether deep copy really matters when doing conversion.
Or deep copy is just a good habit when doing conversion.
Please correct me if I'm wrong.

eventing/pkg/apis/sources/v1alpha1/apiserver_conversion.go

Line 119 in a0a5daf

sink.Spec.CloudEventOverrides = source.Spec.CloudEventOverrides

eventing/pkg/apis/sources/v1alpha1/apiserver_conversion.go

Line 88 in a0a5daf

if source.Spec.CloudEventOverrides != nil {

eventing/pkg/apis/sources/v1alpha1/apiserver_conversion.go

Line 68 in a0a5daf

if source.Spec.ResourceOwner != nil {

capri-xiyue · 2020-07-17T02:26:37Z

/unhold

knative-metrics-robot · 2020-07-17T02:53:28Z

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/apis/sources/v1alpha2/apiserver_conversion.go	100.0%	39.0%	-61.0

aliok · 2020-07-17T07:40:33Z

+		},
+		URI: path,
+	}
+	sinkUri, _ := apis.ParseURL("http://example.com/path")


You can use apis.HTTP() here

apis.ParseURL is used in a lot of places in the codebase. I will just keep it for now. We can change all of them in another PR.

another clean up issue created #3623

aliok · 2020-07-17T07:44:43Z

+
+	tests := []struct {
+		name string
+		in   apis.Convertible


Changing this with *v1beta1.ApiServerSource could make things easier later.
Examples I saw so far are like that.
But not a big deal.

I followed the

eventing/pkg/apis/sources/v1alpha1/apiserver_conversion_test.go

Line 222 in 6caed6a

in apis.Convertible

here. I think at first we use apis.Convertible so that we will be able provide different versions of sources as in. But now we use proxy when doing the conversion and the api v1beta1 and v1alpha2 apiserversource are the same, therefore if the in in the UT is the highest verison, when it does conversion, it will also test the conversion between middle versions and lowest version, so both *v1beta1.ApiServerSource and apis.Convertible work

For now, I will keep apis.Convertible since it's more flexible

aliok

/lgtm

Added some minor comments, feel free to skip it

lionelvillard · 2020-07-17T13:21:38Z

DeepCopy is not needed in the conversion code.

nachocano · 2020-07-17T16:47:05Z

/lgtm
/approve

@capri-xiyue can you address @aliok comments in the reconciler/e2e follow up?
If DeepCopy is not needed as @lionelvillard mentioned, I think it might be worth creating a separate issue to track it, and change everywhere. I think we are DeepCopying in multiple places, not just sources.

knative-prow-robot · 2020-07-17T16:47:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, capri-xiyue, nachocano

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~~ [nachocano]

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

capri-xiyue · 2020-07-17T17:11:13Z

/lgtm
/approve

@capri-xiyue can you address @aliok comments in the reconciler/e2e follow up?
If DeepCopy is not needed as @lionelvillard mentioned, I think it might be worth creating a separate issue to track it, and change everywhere. I think we are DeepCopying in multiple places, not just sources.

#3622 and #3623 created

nachocano · 2020-07-17T17:30:36Z

/retest

…

On Fri, Jul 17, 2020 at 10:29 AM Knative test reporter robot < ***@***.***> wrote: The following jobs failed: Test name Triggers Retries pull-knative-eventing-integration-tests 0/3 Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests: test/e2e.TestChannelChain test/e2e.TestChannelChain/Channel-messaging.knative.dev/v1 — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#3616 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DEN5T4MXVBHKCQGDN3R4CDA7ANCNFSM4O5CUXFQ> .

knative-test-reporter-robot · 2020-07-17T17:36:05Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-integration-tests		0/3
pull-knative-eventing-upgrade-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-upgrade-tests:

test/upgrade.TestPreUpgrade
test/upgrade.TestPreUpgrade/InMemoryChannel-messaging.knative.dev/v1beta1

capri-xiyue · 2020-07-17T17:42:08Z

/test pull-knative-eventing-upgrade-tests

capri-xiyue added 4 commits July 16, 2020 13:46

added v1beta1 apiserversource type

b812a48

fixed UT

3cef481

fixed v1alpha1 apiserver_lifecycle

6da4967

added apiversource source conversion for v1beta1

3716f86

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

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

knative-prow-robot requested review from Harwayne and aliok July 16, 2020 22:55

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

capri-xiyue changed the title ~~Apiserversource defaults conversion~~ [WIP]Apiserversource defaults conversion Jul 16, 2020

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 Jul 16, 2020

knative-prow-robot assigned lionelvillard and nachocano Jul 16, 2020

merged with master

34f92cd

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 17, 2020

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

capri-xiyue changed the title ~~[WIP]Apiserversource defaults conversion~~ Apiserversource defaults conversion Jul 17, 2020

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 Jul 17, 2020

fixed format

6c35aae

added ut for v1alpha2 conversion

de3dce6

aliok reviewed Jul 17, 2020

View reviewed changes

aliok approved these changes Jul 17, 2020

View reviewed changes

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

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

This was referenced Jul 17, 2020

DeepCopy is not needed in the conversion code #3622

Closed

Use apis.HTTP() instead of apis.ParseURL() in the codebase #3623

Closed

knative-prow-robot merged commit 5f23f34 into knative:master Jul 17, 2020

aliok mentioned this pull request Sep 25, 2020

Add aliok to approvers #4118

Merged

Conversation

capri-xiyue commented Jul 16, 2020

Proposed Changes

Uh oh!

capri-xiyue commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

capri-xiyue commented Jul 16, 2020

Uh oh!

capri-xiyue commented Jul 16, 2020

Uh oh!

capri-xiyue commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

capri-xiyue commented Jul 17, 2020

Uh oh!

knative-metrics-robot commented Jul 17, 2020

Uh oh!

aliok Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

aliok Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionelvillard commented Jul 17, 2020

Uh oh!

nachocano commented Jul 17, 2020

Uh oh!

knative-prow-robot commented Jul 17, 2020

Uh oh!

capri-xiyue commented Jul 17, 2020

Uh oh!

nachocano commented Jul 17, 2020 via email

Uh oh!

knative-test-reporter-robot commented Jul 17, 2020

Uh oh!

capri-xiyue commented Jul 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

capri-xiyue commented Jul 16, 2020 •

edited

Loading

capri-xiyue commented Jul 17, 2020 •

edited

Loading

capri-xiyue Jul 17, 2020 •

edited

Loading

capri-xiyue Jul 17, 2020 •

edited

Loading

aliok left a comment •

edited

Loading