Eventing v1 api by vaikas · Pull Request #3372 · knative/eventing

vaikas · 2020-06-22T22:16:18Z

Addresses #3337

Proposed Changes

Introduce eventing.v1[Broker,Trigger] resources.
Will create additional PRs for wiring in webhooks, etc.

Release Note

- 🎁 Add new feature
Add eventing.v1.{Broker,Trigger} resources.

Docs
knative/docs#2609

knative-prow-robot · 2020-06-22T22:16:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas

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

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

knative-test-reporter-robot · 2020-06-22T23:26:09Z

The following jobs failed:

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

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

test/upgrade.Failure

matzew · 2020-06-23T04:12:29Z

Please see: #3368 @aliok started on this already, as stated on the #3337 ticket 😄

Perhaps working together on this?

aliok · 2020-06-23T06:55:35Z

I had a PR for types only (#3368), but I compared the branches and your branch does more things 😄 I am closing mine and I will contribute to yours.

aliok · 2020-06-23T07:04:03Z

 ${KNATIVE_CODEGEN_PKG}/hack/generate-knative.sh "injection" \
  knative.dev/eventing/pkg/client knative.dev/eventing/pkg/apis \
-  "eventing:v1beta1 messaging:v1beta1 flows:v1beta1 sources:v1alpha1 sources:v1alpha2 duck:v1alpha1 duck:v1beta1 duck:v1 configs:v1alpha1" \
+  "eventing:v1beta1 eventing:v1 messaging:v1beta1 flows:v1beta1 sources:v1alpha1 sources:v1alpha2 duck:v1alpha1 duck:v1beta1 configs:v1alpha1" \


Did you delete duck:v1 on purpose here?

nope, mistake :) Thanks!

aliok · 2020-06-23T07:14:52Z

/assign

aliok · 2020-06-23T07:18:31Z

+	corev1 "k8s.io/api/core/v1"
+
+	eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
+	messagingv1beta1 "knative.dev/eventing/pkg/apis/messaging/v1beta1"


/hold
FYI, messaging v1 is on its way by @nlopezgi.
Feel free to unhold if you think we can merge with messaging v1beta1

Yeah, this is just for tests, and really the type doesn't change, so I think we're ok. Let's see :)

I agree with @vaikas on this, @aliok

if you do too, please unhold

aliok · 2020-06-23T07:22:24Z

+// +genclient
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object


Suggested change

// +genclient

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// +genclient

// +genreconciler:krshapedlogic=true

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

v1beta1 api has // +genreconciler:krshapedlogic=true. Why we don't need it here?

Ah, it's only cause I didn't yet use it. Changed 👍

aliok · 2020-06-23T07:47:49Z


 func TestTriggerConversionBadType(t *testing.T) {
-	good, bad := &Trigger{}, &Trigger{}
+	good, bad := &Trigger{}, &Broker{}


There's not much value in this conversion test and other conversion tests for other types unless we add a test for good conversion case as well.

You didn't have to change the &Trigger to &Broker above to make the conversion fail and tests pass. That's what I mean why there's not much value.

But, let's ignore this for now and I am interested in having proper conversion tests in a later PR.

Yes, agreed :)

Created #3395

vaikas · 2020-06-23T14:20:11Z

Thanks, I think I addressed all the issues, PTAL :)

nachocano · 2020-06-23T14:32:12Z

@vaikas, I created an issue sometime back to improve the EventType object #2750. I haven't had the time to go ahead and focus on this, but I will in this upcoming quarter. Hope to start sometime in the next two weeks.

I'm saying this because I think we still need to do some improvements to the discoverability bits enabled by EventType. I had some discussions before my leave with @n3wscott and we were thinking of maybe moving spec.broker and spec.source to status fields, and probably make them lists, otherwise we are creating a different EventType for each different CE source, and the cardinality will blow up. I was interested in also advertising the extensions an EventType can provide, etc...

If you can hold off the move of EventType to v1, it'd be great. I'm planning to actively work on it, although it surely won't make the cut to this upcoming release. This has always been a contentious topic...
Do you mind if we make it for 0.17?

WDYT

vaikas · 2020-06-23T14:49:21Z

@nachocano sure thing, I'll yank that. Stay tuned :)

nachocano · 2020-06-23T14:52:29Z

@nachocano sure thing, I'll yank that. Stay tuned :)

Thanks a bunch!

matzew · 2020-06-23T15:56:55Z

/lgtm

vaikas · 2020-06-23T16:10:28Z

@aliok are you fine with removing the hold now?
@nachocano removed eventtype, PTAL

knative-metrics-robot · 2020-06-23T16:15:29Z

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/eventing/v1/broker_conversion.go	Do not exist	100.0%
pkg/apis/eventing/v1/broker_defaults.go	Do not exist	100.0%
pkg/apis/eventing/v1/broker_lifecycle.go	Do not exist	95.5%
pkg/apis/eventing/v1/broker_types.go	Do not exist	66.7%
pkg/apis/eventing/v1/broker_validation.go	Do not exist	94.4%
pkg/apis/eventing/v1/register.go	Do not exist	0.0%
pkg/apis/eventing/v1/test_helper.go	Do not exist	80.0%
pkg/apis/eventing/v1/trigger_conversion.go	Do not exist	100.0%
pkg/apis/eventing/v1/trigger_defaults.go	Do not exist	100.0%
pkg/apis/eventing/v1/trigger_lifecycle.go	Do not exist	51.1%
pkg/apis/eventing/v1/trigger_types.go	Do not exist	100.0%
pkg/apis/eventing/v1/trigger_validation.go	Do not exist	96.2%
pkg/apis/eventing/v1beta1/broker_conversion.go	100.0%	25.0%	-75.0
pkg/apis/eventing/v1beta1/trigger_conversion.go	100.0%	18.2%	-81.8

knative-prow-robot · 2020-06-23T16:15:36Z

@vaikas: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-go-coverage	`8592340`	link	`/test pull-knative-eventing-go-coverage`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

nachocano · 2020-06-23T16:18:54Z

/lgtm
Thanks @vaikas !!!

vaikas · 2020-06-23T17:27:33Z

/unhold
Once the v1 of messaging lands, we can clean up the test_helper. Unholding to unblock any work that is a follow on to this.

* new types, codegen * ducks to v1 * add types, boilerplate conversion * moar v1beta1->v1 manual conversion * fix pr comments * remove eventtypes

vaikas added 3 commits June 22, 2020 10:39

new types, codegen

0c2a84d

ducks to v1

9db04dd

add types, boilerplate conversion

e148274

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 Jun 22, 2020

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

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Jun 22, 2020

knative-prow-robot requested review from n3wscott and steuhs June 22, 2020 22:16

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

vaikas mentioned this pull request Jun 22, 2020

Add documentation and examples for eventing.v1 components. knative/docs#2609

Closed

moar v1beta1->v1 manual conversion

e00a8e7

aliok mentioned this pull request Jun 23, 2020

[WIP] v1 types for eventing.* #3368

Closed

aliok reviewed Jun 23, 2020

View reviewed changes

knative-prow-robot assigned aliok Jun 23, 2020

aliok reviewed Jun 23, 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 Jun 23, 2020

aliok reviewed Jun 23, 2020

View reviewed changes

fix pr comments

af98ca3

vaikas changed the title ~~[WIP]: Eventing v1 api~~ Eventing v1 api Jun 23, 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 Jun 23, 2020

knative-prow-robot assigned matzew Jun 23, 2020

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

remove eventtypes

8592340

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

knative-prow-robot assigned nachocano Jun 23, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 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 Jun 23, 2020

knative-prow-robot merged commit 13e5137 into knative:master Jun 23, 2020

vaikas deleted the eventing-v1-api branch June 23, 2020 17:42

aliok mentioned this pull request Jun 24, 2020

Improve eventing v1beta1 conversion tests #3395

Merged

lberk pushed a commit to lberk/eventing that referenced this pull request Jun 26, 2020

Eventing v1 api (knative#3372)

f5587ac

* new types, codegen * ducks to v1 * add types, boilerplate conversion * moar v1beta1->v1 manual conversion * fix pr comments * remove eventtypes

vaikas mentioned this pull request Jun 29, 2020

use messaging.v1 in test_helper. Follow up to 3372 comment. #3437

Merged

aliok mentioned this pull request Sep 25, 2020

Add aliok to approvers #4118

Merged

		// +genclient
		// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

Conversation

vaikas commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Jun 22, 2020

Uh oh!

knative-test-reporter-robot commented Jun 22, 2020

Uh oh!

matzew commented Jun 23, 2020

Uh oh!

aliok commented Jun 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aliok commented Jun 23, 2020

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!

vaikas commented Jun 23, 2020

Uh oh!

nachocano commented Jun 23, 2020

Uh oh!

vaikas commented Jun 23, 2020

Uh oh!

nachocano commented Jun 23, 2020

Uh oh!

matzew commented Jun 23, 2020

Uh oh!

vaikas commented Jun 23, 2020

Uh oh!

knative-metrics-robot commented Jun 23, 2020

Uh oh!

knative-prow-robot commented Jun 23, 2020

Uh oh!

nachocano commented Jun 23, 2020

Uh oh!

vaikas commented Jun 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vaikas commented Jun 22, 2020 •

edited

Loading