-
Notifications
You must be signed in to change notification settings - Fork 601
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
More EventType v1beta2 work #6903
Conversation
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #6903 +/- ##
==========================================
- Coverage 79.84% 79.70% -0.15%
==========================================
Files 243 243
Lines 12530 12576 +46
==========================================
+ Hits 10005 10024 +19
- Misses 2028 2055 +27
Partials 497 497
☔ View full report in Codecov by Sentry. |
@vishal-chdhry interested in doing a review here too? |
name: v1beta2 | ||
served: true | ||
storage: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one doubt, why is storage false
here and server is true
, unlike in #5272 where it was slightly different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was bad idea, you go step by step on these. So we take it like this and turn on storage when the v1b1 is really done/obsolate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vishal-chdhry see here:
- https://github.com/knative/eventing/pull/5324/files#diff-cc7409beb5d3f079b67c9fba7e5dde4f5839d237761b927794577b9b1232d77dR199
- b2387db#diff-95ba4ac834854b8d9f83dc9eddf0fb917fa5d70165a04eb22693b60d91ec9bf1R66
Both PRs did a more reasonal approach, NOT bumping both fields at the same time.
Hence I did only enalbe servred
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a nit / question. Otherwise LGTM
func (source *EventType) ConvertTo(ctx context.Context, to apis.Convertible) error { | ||
return fmt.Errorf("v1beta1 is the highest known version, got: %T", to) | ||
func (source *EventType) ConvertTo(ctx context.Context, obj apis.Convertible) error { | ||
switch sink := obj.(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Isn't the name sink
a bit misleading in our case? (instead of using to
/from
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@creydr I followed the pattern from previous iterations, see: https://github.com/knative/eventing/blob/release-0.20/pkg/apis/eventing/v1beta1/broker_conversion.go#L31
Co-authored-by: Vishal Choudhary <contactvishaltech@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: creydr, matzew 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 |
Fixes #6891
Fixes #6892
Proposed Changes
Pre-review Checklist
Release Note
Docs