[kn-source-github] initial PR for the kn-source-github dependencies #28
Conversation
will have the code. This two step PR should make the review process cleaner and easier. Please merge this ASAP, thx.
knative-prow-robot
added
the
approved
|
/ok-to-test |
knative-prow-robot
added
the
ok-to-test
maximilien
changed the title
|
[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 |
There was a problem hiding this comment.
Looks good as the initial commit to brin in dependencies.
I have a request for the go.mod deps (see below), but bringing in the eventing-contrib repository raises another intersting question: As eventing-contrib does not follow the client-contribs approach to clearly separate all plugin along with their dependency, every client plugin implementing any source from eventing-contrib has to depend on all source in eventing-contrib. This does for sure not scale (having N copies of eventing-contrib with M sources including their (server-side !) dependencies) and that's not only the faul of client-contrib to vendor on a per-plugin case.
I wonder how we should proceed here:
- Pushing eventing-contrib to use a similar model as client-cotrib, i.e. one self-contained directory per source with all source code and deps.
- Copying the client API manually, which would also skip all the server side dependencies that we don't need anyway. Ideally each source has a dedicated client package.
Maybe a combined approach would be best: To ask (or also help) to refactor the source that we are using in plugins to expose a dedicated client package that we can pick up in isolation ?
| github.com/maximilien/kn-source-pkg v0.4.0 | ||
| github.com/spf13/cobra v1.0.0 // indirect | ||
| knative.dev/eventing-contrib v0.14.0 | ||
| knative.dev/test-infra v0.0.0-20200413202711-9cf64fb1b912 |
There was a problem hiding this comment.
Why do you need test-infra ? We are including the (shared among all plugin) test-infra scripts in the top-level "test-infra" directory. Shared, because all plugins share the same test infra structure.
Plugins must not include test-infra anymore. See also the discussions at knative/test-infra#2068
There was a problem hiding this comment.
Yeah, I can remove test-infa. I think I left it there as I copied the ./hack/build.sh from client initially.
|
/lgtm |
Subsequent PRs will have the code. This two step PR should make the review process cleaner and easier. Please merge this ASAP, thx.