knative / client-contrib

Community contributed `kn` plugins.
Apache License 2.0
10 stars 23 forks source link

Remove vendoring from all plugins #49

Closed rhuss closed 4 years ago

rhuss commented 4 years ago

Compilation works without issues (with or without vendoring), but here are some numbers:

du -ks */vendor
30640   admin/vendor
768 hello/vendor
40104   migration/vendor
36868   source-github/vendor
36904   source-kafka/vendor

These numbers are a bit surprising especially that the local cache is that big. It looks like that when using vendoring only the touched dependency files are included whereas for the GOPATH cache the whole dependency repository is cloned:

du -ks plugins/migration/vendor/github.com/aws
1252    plugins/migration/vendor/github.com/aws

du -ks $GOPATH/pkg/mod/github.com/aws
122924  /Users/roland/Development/go/workspace/pkg/mod/github.com/aws

However, over the long run and when dependencies are updated, the vendor directory's impact might increase as Git will carry also its history when doing a git clone.

The question is how to proceed.

BTW, the stats also provide a data point in favour of the self-contained approach: All plugins are using different versions of cobrar, which leads to:

ls -d $GOPATH/pkg/mod/github.com/spf13/cobra@v*
/Users/roland/Development/go/workspace/pkg/mod/github.com/spf13/cobra@v0.0.5
/Users/roland/Development/go/workspace/pkg/mod/github.com/spf13/cobra@v0.0.6
/Users/roland/Development/go/workspace/pkg/mod/github.com/spf13/cobra@v0.0.7
/Users/roland/Development/go/workspace/pkg/mod/github.com/spf13/cobra@v1.0.0

If all plugins would share the dependencies any change in a plugin on such a library needs to be propagated to all other plugins, whether they are maintained by the same group of people or not. This requires quote some syncing efforts, which is avoided by the self-contained approach (for the price of using potentially more disk space but as seen above, different versions also eat up disk space).

Also note that Kubernetes doesn't use vendoring so "using vendoring" is not necessarily an invariant.

daisy-ycguo commented 4 years ago

/test pull-knative-client-contrib-integration-tests

daisy-ycguo commented 4 years ago

The failure of integration test happens when installing kn client. It looks like it is caused by use of incompatible dependency k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible.

maximilien commented 4 years ago

/retest

rhuss commented 4 years ago

Also interesting: Even when kn itself is using vendoring, the kn build still downloads a ton of stuff to $GOPATH/pkg/mod. I have to find out why, but maybe that's just needed, so that then the size difference as said in the comment above is not that much anymore.

rhuss commented 4 years ago

Let's start a poll for deciding whether we want vendoring or not (we already discussed the arguments pro & contra vendoring)

👍 for not vendoring dependencies and rely on the local GOPATH cache (so, for applying this PR) 👎 for keep vendoring for each individual plugin

Please vote until the next WG call so that we can decide on which direction to go (except when we get an agreement earlier)

rhuss commented 4 years ago

Sorry for the delay, I was quite busy last week. This PR should be merged within this week.

rhuss commented 4 years ago

/unhold

rhuss commented 4 years ago

If tests are passing, I think we are ready to go to merge those, so that we can start migration to knative-sandbox // @daisy-ycguo

knative-prow-robot commented 4 years ago

@rhuss: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-client-contrib-integration-tests a76bdf50acdab8e19773122960e90aab836366eb link /test pull-knative-client-contrib-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
daisy-ycguo commented 4 years ago

@rhuss I think you are using a wrong version of source-kafka, because the build result of source-kafka in your branch is "kn-source_kafka". It's already changed to "kn-source-kafka" in the master branch.

Now the integration test failed because:

installing 'kn' plugin 'kn-source-kafka' from path '/home/prow/go/src/knative.dev/client-contrib/plugins/source-kafka' to config path: /root/.config/kn
error copying plugin file to config directory: open /home/prow/go/src/knative.dev/client-contrib/plugins/source-kafka/kn-source-kafka: no such file or directory
--- FAIL: TestSourceKafka (7.27s)
    source_kafka_test.go:72: assertion failed: error is not nil: open /home/prow/go/src/knative.dev/client-contrib/plugins/source-kafka/kn-source-kafka: no such file or directory
knative-prow-robot commented 4 years ago

@rhuss: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
knative-prow-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, zhanggbj

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](https://github.com/knative/client-contrib/blob/master/OWNERS)~~ [rhuss] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
zhanggbj commented 4 years ago

Have to rebase again as admin has new PR merged...

rhuss commented 4 years ago

Let's close this PR and do the decission of vendoring or not on a case-by-case basis when we are moving plugins over to the sandbox orga