-
Notifications
You must be signed in to change notification settings - Fork 587
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
Cleanup channel template internal types #4749
Cleanup channel template internal types #4749
Conversation
/assign |
I'm loving the red lines. Could you add more comprehensive UT so we get more coverage? |
Codecov Report
@@ Coverage Diff @@
## master #4749 +/- ##
==========================================
+ Coverage 81.11% 81.25% +0.14%
==========================================
Files 291 291
Lines 8239 8281 +42
==========================================
+ Hits 6683 6729 +46
+ Misses 1156 1143 -13
- Partials 400 409 +9
Continue to review full report at Codecov.
|
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.
Just a comment about maybe trying to document the channel duck package more. +1 on more UT coverage. Other than that, lgtm.
|
||
duckv1 "knative.dev/eventing/pkg/apis/duck/v1" | ||
) | ||
|
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.
Well, seeing this is very essential package in eventing pkg to be used in gazillion of other components, I think it would be great to add a more detailed generic comment explaining the context here. Example points to cover:
- What is the physical channel?
- How do the different structs relate to the
Channel
type. - Maybe with an example?
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 see your point, let me add some additional docs to NewPhysicalChannel
so it explains why this thing is important
/retest |
}, | ||
}, | ||
}, | ||
"KafkaChannel": { |
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.
It seems okay to remove some of these tests, especially the KafkaChannel test, as it should be the responsibility of eventing-kafka. But there seem to be other things being thrown out by removing this test entirely, e.g., the BrokerChannelName check. Any reason for this?
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.
The reason is that the tested function here doesn't exist anymore. There is still a test for BrokerChannelName
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 see. And the OwnerReferences test done by assertSoleOwner
?
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.
assertSoleOwner
was an assert executed by the deleted test, and note that now the OwnerReferences
is set by the caller of NewPhysicalChannel
/retest |
…venting/pkg/duck module Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
226d206
to
ce87ac2
Compare
The following is the coverage report on the affected files.
|
/lgtm |
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.
great work!
/lgtm
/unhold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devguyio, slinkydeveloper, vaikas 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 |
/retest |
While working on #4652, we found out that we have these internal types, necessary to implement some duck typing magic, that are implementation details that shouldn't be exposed and shouldn't be versioned. This PR cleanup that code and adds a single entrypoint to create from the various ducks a physical channel, removing the various duplicated code in each controller.
Signed-off-by: Francesco Guardiani [email protected]
Proposed Changes
Release Note