Reconcile ET properly with no reference #7423

Cali0707 · 2023-10-31T20:28:06Z

Feature track: https://docs.google.com/document/d/1US5Ve0CwhXwO8pUudN36vLkuSetEtSgwQ1btuG_9Xmk/edit

Proposed Changes

Update the reconciler to check if the reference is set before validating the reference
Add a new status helper function for when the reference is not set

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

EventTypes no longer need a reference to reconcile.

Docs

Signed-off-by: Calum Murray <[email protected]>

knative-prow · 2023-10-31T20:28:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707

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

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

codecov · 2023-10-31T20:37:50Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (79bb385) 76.63% compared to head (56346e9) 75.55%.
Report is 27 commits behind head on main.

Files	Patch %	Lines
pkg/apis/eventing/v1beta3/eventtype_lifecycle.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7423      +/-   ##
==========================================
- Coverage   76.63%   75.55%   -1.09%     
==========================================
  Files         259      261       +2     
  Lines       14270    14673     +403     
==========================================
+ Hits        10936    11086     +150     
- Misses       2778     3008     +230     
- Partials      556      579      +23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Calum Murray <[email protected]>

matzew · 2023-11-16T16:10:47Z

/hold

Let's do this in ET_v1b3, not in this v1b2 version!

See: #7304

matzew · 2023-11-16T16:12:14Z

pkg/reconciler/eventtype/eventtype.go

+//  1. Check if there is a reference
+//     a) if not, reconcile to true
+//     b) if yes, continue reconciling
+//  2. Verify the Reference exists.


I like having this comment here. thx

Signed-off-by: Calum Murray <[email protected]>

dsimansk · 2023-12-04T15:20:18Z

pkg/reconciler/testing/v1/eventtype.go

+func NewEventType(name, namespace string, o ...EventTypeOption) *v1beta3.EventType {
+	et := &v1beta3.EventType{


I think this should land in new package testing/v1beta3. Perhaps the package was supposed to be introduced by v1beta3 PR. :)

the v1 used to use the "latest".

I think that is OK... but we can (separately) do different?

Any comments, @Cali0707, @dsimansk ?

I'm happy to update this either in this PR or a separate PR if we want, but this file seemed to be using v1beta2 so I had assumed it would be fine to bump to v1beta3 :)

Cali0707 · 2023-12-07T16:29:38Z

/retest-required

matzew · 2023-12-08T07:04:52Z

/hold

spec:
  description: My ET no ref
  reference:
    apiVersion: eventing.knative.dev/v1
    kind: Broker
    name: default
    namespace: default
  source: /yo/man
  type: blah.blub
status: {}

I see this still uses the wrong defaulting /cc @dsimansk

Cali0707 · 2024-01-03T18:04:46Z

@matzew I think the problem is that the cluster still has the v1beta2 CRD, not the v1beta3 CRD. When I get the CRD from the cluster after building from main, I get v1beta2.

matzew · 2024-01-04T09:56:20Z

The defaulting is just wrong, so perhaps we need to change that, we do need the CRD version. yes

That's a new different PR. See the old issues for all of this, there are a plenty of PRs that are needed in order to fully get it. Here is a list of old issues (and they have links to PRs):
https://github.com/orgs/knative/projects/62

We can also wait, and do all of that AFTER the 1.13 reelease - in order to relax things, and buy time

/cc @Cali0707 @dsimansk

knative-prow · 2024-01-04T09:56:23Z

@matzew: GitHub didn't allow me to request PR reviews from the following users: Cali0707.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

The defaulting is just wrong, so perhaps we need to change that, we do need the CRD version. yes

That's a new different PR. See the old issues for all of this, there are a plenty of PRs that are needed in order to fully get it

We can also wait, and do all of that AFTER the 1.13 reelease - in order to relax things, and buy time

/cc @Cali0707 @dsimansk

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.

Signed-off-by: Calum Murray <[email protected]>

Cali0707 · 2024-01-04T18:24:14Z

The newest commit fixed the defaulting behaviour, but it uncovered the following issue:

0104 18:18:19.795077       1 reflector.go:533] k8s.io/[email protected]/tools/cache/reflector.go:231: failed to list *v1beta3.EventType: the server could not find the requested resource (get eventtypes.eventing.knative.dev)
E0104 18:18:19.795115       1 reflector.go:148] k8s.io/[email protected]/tools/cache/reflector.go:231: Failed to watch *v1beta3.EventType: failed to list *v1beta3.EventType: the server could not find the requested resource (get eventtypes.eventing.knative.dev)
W0104 18:18:20.658009       1 reflector.go:533] k8s.io/[email protected]/tools/cache/reflector.go:231: failed to list *v1beta3.EventType: the server could not find the requested resource (get eventtypes.eventing.knative.dev)

As such, I think this should be on hold until after the v1beta3 CRD is present

Signed-off-by: Calum Murray <[email protected]>

knative-prow · 2024-01-04T19:46:24Z

@Cali0707: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
upgrade-tests_eventing_main	`56346e9`	link	true	`/test upgrade-tests`
conformance-tests_eventing_main	`56346e9`	link	true	`/test conformance-tests`
reconciler-tests_eventing_main	`56346e9`	link	true	`/test reconciler-tests`

Your PR dashboard.

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.

knative-prow-robot · 2024-01-08T15:31:10Z

PR needs rebase.

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.

pierDipi · 2024-01-22T15:57:26Z

@matzew @Cali0707 what's the status of this PR, if it's not valid anymore, can we close it ?

Cali0707 · 2024-01-22T15:59:01Z

@pierDipi it would work if we served a v1beta3 CRD. I could update it to work on v1beta2 if we want to change the reconcile behaviour for v1beta2 wrt no reference being an allowed state for the CRD. Otherwise, we could close this. WDYT? I'm a little confused about the current plan for event types tbh

pierDipi · 2024-01-22T16:01:52Z

Thanks for raising that there is confusion, let's discuss this in the working group meeting after the release

matzew · 2024-01-24T08:30:34Z

/close
as state in #7264 (comment)

knative-prow · 2024-01-24T08:30:38Z

@matzew: Closed this PR.

In response to this:

/close
as state in #7264 (comment)

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.

reconcile et to true if no reference set

d6a98e3

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot requested review from creydr and odacremolbap October 31, 2023 20:28

knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 31, 2023

Add unit test

d40cd1b

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 1, 2023

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

matzew reviewed Nov 16, 2023

View reviewed changes

matzew mentioned this pull request Nov 16, 2023

WIP: Discovery: change behavior to allow ETs without any reference for better discovery #7302

Closed

5 tasks

Cali0707 added 2 commits December 4, 2023 08:42

Merge branch 'main' into et-no-reference-set

3f28384

Bump changes onto v1beta3 types

dd41e9b

Signed-off-by: Calum Murray <[email protected]>

dsimansk reviewed Dec 6, 2023

View reviewed changes

knative-prow bot requested a review from dsimansk January 4, 2024 09:56

fix default behaviour for event types

9cfa2fa

Signed-off-by: Calum Murray <[email protected]>

fix unit tests

56346e9

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 4, 2024

Cali0707 linked an issue Jan 5, 2024 that may be closed by this pull request

Eventtype defaults to a "default" broker #6754

Closed

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

dsimansk mentioned this pull request Jan 16, 2024

Enable serving of EventTypes v1beta3 #7583

Closed

5 tasks

knative-prow bot closed this Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile ET properly with no reference #7423

Reconcile ET properly with no reference #7423

Cali0707 commented Oct 31, 2023 •

edited by matzew

Loading

knative-prow bot commented Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

matzew commented Nov 16, 2023

matzew Nov 16, 2023

dsimansk Dec 4, 2023

matzew Dec 7, 2023

Cali0707 Dec 7, 2023

Cali0707 commented Dec 7, 2023

matzew commented Dec 8, 2023

Cali0707 commented Jan 3, 2024

matzew commented Jan 4, 2024 •

edited

Loading

knative-prow bot commented Jan 4, 2024

Cali0707 commented Jan 4, 2024

knative-prow bot commented Jan 4, 2024

knative-prow-robot commented Jan 8, 2024

pierDipi commented Jan 22, 2024

Cali0707 commented Jan 22, 2024

pierDipi commented Jan 22, 2024 •

edited

Loading

matzew commented Jan 24, 2024

knative-prow bot commented Jan 24, 2024

		func NewEventType(name, namespace string, o ...EventTypeOption) *v1beta3.EventType {
		et := &v1beta3.EventType{

Reconcile ET properly with no reference #7423

Reconcile ET properly with no reference #7423

Conversation

Cali0707 commented Oct 31, 2023 • edited by matzew Loading

Proposed Changes

Pre-review Checklist

knative-prow bot commented Oct 31, 2023

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

matzew commented Nov 16, 2023

matzew Nov 16, 2023

Choose a reason for hiding this comment

dsimansk Dec 4, 2023

Choose a reason for hiding this comment

matzew Dec 7, 2023

Choose a reason for hiding this comment

Cali0707 Dec 7, 2023

Choose a reason for hiding this comment

Cali0707 commented Dec 7, 2023

matzew commented Dec 8, 2023

Cali0707 commented Jan 3, 2024

matzew commented Jan 4, 2024 • edited Loading

knative-prow bot commented Jan 4, 2024

Cali0707 commented Jan 4, 2024

knative-prow bot commented Jan 4, 2024

knative-prow-robot commented Jan 8, 2024

pierDipi commented Jan 22, 2024

Cali0707 commented Jan 22, 2024

pierDipi commented Jan 22, 2024 • edited Loading

matzew commented Jan 24, 2024

knative-prow bot commented Jan 24, 2024

Cali0707 commented Oct 31, 2023 •

edited by matzew

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

matzew commented Jan 4, 2024 •

edited

Loading

pierDipi commented Jan 22, 2024 •

edited

Loading