-
Notifications
You must be signed in to change notification settings - Fork 261
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
Changes to SinkFlags in order to support plugin event sources #748
Conversation
… in client-contrib
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.
Sorry, I thought I already sent the review (but probably forgot to push the button).
I don't think that anything from client-contrib must depend on client, as this would also couple the release cycle of otherwise independent plugins to the client release cycle.
Especially when the reuse is only marginally, its better to duplicate for the plugins to make plugins self-contained and independent.
As this, I don't think we should merge this PR.
/hold
@@ -132,3 +134,17 @@ func SinkToString(sink duckv1.Destination) string { | |||
} | |||
return "" | |||
} | |||
|
|||
// SinkToDuckV1Beta1 converts a Destination from duckv1 to duckv1beta1 | |||
func SinkToDuckV1Beta1(destination *duckv1.Destination) *duckv1beta1.Destination { |
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.
Its not really a Sink
but a Destination
what you are converting (could be used for other destinations than sinks).
I would it call it ToDuckV1Beta1
as Destination
is already clear from the signature, (alternative: DestinationAsDuckV1Beta1
if we want to be more verbose)
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.
/ok-to-test
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: daisy-ycguo, maximilien 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 |
The following is the coverage report on the affected files.
|
While I would agree with the point of having common code in a separate repo, perhaps So while I like the idea of extracting some code for common packages, I also think we need to figure out a way to allow exported types and code to be usable for external components. These “users” would simply vendor the version of the |
@maximilien I get you point, so let's look for an smaller, not-optimal solution as long as its still hard to introduce new repositories. If separation of client-internal and exported libraries can not be enforced by using different dependencies (or GitHub repos), we should at least base that separation on Golang package level and maybe even reflect it in the directory structure (e.g. introducing a top-level directory for code that is meant to be reused). Only relying on the golang idiom of uppercase types for exported functionality is not good enough for us (i.e. there are cross-package dependencies whithin the client which are not meant to used outside as the can arbitrarily be changed). Having this clear seperation even within the The candidates for exported functionalities that I see are:
Anything else ? So I would suggest to introduce eg. @daisy-ycguo would this work for you ? If so, I would pick up your PR and include it in a separate PR for setting up this structure. |
Good.
The e2e test infrastructure as well as some parts of the UT code. Let's start with that and I am sure lots more will come up.
Sounds good. |
I will close this PR since dr.max has tracked it with the issue #763. Thank you for the comments and discussions. |
Description
Make two changes to SinkFlags in order to support other event sources at client-contrib.
Changes
flag.FlagSet
, similar as AddNamespaceFlags().