Detect and prevent new vendor cycles #45176
Conversation
|
This change is |
lavalamp
added
the
release-note-none
k8s-github-robot
added
approved
hack/verify-no-vendor-cycles.sh
Outdated
| echo "Skipping known violator $i" | ||
| continue | ||
| fi | ||
| deps=$(go list -f '{{range .Deps}}{{.}}{{"\n"}}{{end}}' ./$i 2> /dev/null | grep -v "k8s.io/kubernetes/vendor/" | grep "k8s.io/kubernetes" || echo "") |
There was a problem hiding this comment.
Apart from k8s.io/kubernetes, they shouldn't depend on other staging repos either.
There was a problem hiding this comment.
That's only problematic if the staging repo itself depends on kubernetes, which this should catch. It should be safe to depend on something in staging, do you have a counter example?
There was a problem hiding this comment.
vendor/k8s.io/metrics depends on client-go/pkg/api. It's impossible to remove client-go/pkg/api.
There was a problem hiding this comment.
That's just a regular dependency management problem (main repo and metrics want different versions of client-go), it's not a cycle problem.
There was a problem hiding this comment.
I disagree. It is a cycle problem because the source of the staging code is still in kubernetes.
hack/verify-no-vendor-cycles.sh
Outdated
|
|
||
| failed=false | ||
| for i in $(find vendor/ -type d); do | ||
| if [ -n "$(echo ${exceptions} | grep "^${i}\$")" ]; then |
There was a problem hiding this comment.
style nit: prefer [[ ]] over [ ] in bash scripts.
also if we expect exceptions to be a list of more than one item, the conditional should probably be
if echo "${exceptions[@]}" | grep -q "\(^\| \)$i\( \|$\)"; thenThere was a problem hiding this comment.
I will take your word that the grep line actually works ;)
There was a problem hiding this comment.
yeah, basically, ${exceptions[@]} will be printed as a list of space-separated items.
we want to match if it's the first item, somewhere in the middle, or the last item.
backslashes necessary because bash.
There was a problem hiding this comment.
Yeah that grep line didn't seem to work for me so I did something more obvious. PTAL
|
looks fine besides that one comment |
k8s-ci-robot
added
the
cncf-cla: yes
hack/verify-no-vendor-cycles.sh
Outdated
| failed=false | ||
| for i in $(find vendor/ -type d); do | ||
| exceptionFound=false | ||
| for e in ${exceptions}; do |
There was a problem hiding this comment.
I think this should still be ${exceptions[@]} - without the [@] this is just the first element of the array.
There was a problem hiding this comment.
ah, right you are. fixed.
|
I'll send a PR to remove the exception. |
I see that we have added a dependency with a cyclic reference to kubernetes. This makes life much harder, we should not do it. This script should prevent any more offenders while we fix the existing one. Change-Id: Iabc22ee7a2c7c54697991d2f8c24e80acb0281e4
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ixdy, lavalamp
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
#45758 will remove the exception. |
|
@k8s-bot gce etcd3 e2e test this |
| continue | ||
| fi | ||
|
|
||
| deps=$(go list -f '{{range .Deps}}{{.}}{{"\n"}}{{end}}' ./$i 2> /dev/null | grep -v "k8s.io/kubernetes/vendor/" | grep "k8s.io/kubernetes" || echo "") |
There was a problem hiding this comment.
seems like this should also include packages whose authoritative source is still in this repo (like apimachinery, apiserver)? c.f. discussion in #45665 (comment)
There was a problem hiding this comment.
I agree, see #45176 (comment).
There was a problem hiding this comment.
The whole point of this is to break the repo into chunks that people are allowed to import?
There was a problem hiding this comment.
Yes, that's the point. But now since we have staging, we have the cycle.
|
Automatic merge from submit-queue (batch tested with PRs 41331, 45591, 45600, 45176, 45658) |
…alpha1 Automatic merge from submit-queue (batch tested with PRs 44337, 45775, 45832, 45574, 45758) Stop vendoring heapster v1alpha1 Changes to use the one in staging/metrics. TODO: remove the exception in #45176 Implementing #45498 (comment).
I see that we have added a dependency with a cyclic reference to
kubernetes. This makes life much harder, we should not do it. This
script should prevent any more offenders while we fix the existing one.