-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
⛵️ Move k8s.io/test-infra over to dep #5301
Conversation
/retest |
I wouldn't necessarily have expected the BUILD files to change or for us to pull in 200k more LoC -- is that a misinterpretation of the current vendor state by |
19a8351
to
f6065d8
Compare
Doing this without the prune is even worse:
|
Right now we don't vendor aws stuff for the aws janitor. Is it possible to cleanly express that in go dep? |
https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#ignored |
BUILD
Outdated
@@ -49,7 +49,53 @@ filegroup( | |||
"//testgrid/jenkins_verify:all-srcs", | |||
"//triage:all-srcs", | |||
"//velodrome:all-srcs", | |||
"//vendor:all-srcs", | |||
"//vendor/bitbucket.org/ww/goautoneg:all-srcs", |
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.
what changed here? why isn't it just "//vendor:all-srcs"
?
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 updated the PR body with the commands I used. This was changed by verify/update-bazel.sh
.
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.
Found what I needed to fix: kubernetes/kubernetes#51229
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.
this is why we should make verify/ match k/k hack/ :-P
Probably we should add some basic instructions to the readme. |
1184e9a
to
7a05c25
Compare
Okay. So this works as-is. I was tinkering with also trying to break the dependency on k/k, as the imports we use are all ones that have been split off into the apimachinery/apiserver repos. However there was some bazel funkiness with that, so it'll have to wait for another PR. This one is big enough! @kubernetes/test-infra-maintainers PTAL :) |
It seems that most of the additions are test code, BUILD, or other documentation files. There is also some interesting cleanup. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, cblecker, kargakis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
100k LOC diff is still weird -- looks like we were not vendoring tests in the past, but now we are? Is that ok? Do we want them? |
@@ -0,0 +1,26 @@ | |||
# kubernetes/test-infra dependency management |
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.
Can we link to this from the main README so contributors can find it without digging too much?
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.
Done
Of the additions, the majority are tests: $ git diff --diff-filter=A --name-only a4c8fbf~1..a4c8fbf | wc -l
432
$ git diff --diff-filter=A --name-only a4c8fbf~1..a4c8fbf | grep -E '_test.go$' | wc -l
281 Then we also pull in a lot of new non-code files:
So updated vendored code is small:
So that actually makes me feel like this is a lot smaller of a move than it seems. Seems like we are updating those dependencies? |
@stevekuznetsov dep and godep function differently. When godep saves things to |
/hold cancel |
Right -- the majority of this is due to test and non-code changes, but that is still a couple transitive version bumps in the dependencies from my quick search. Should be fine but did want to point it out. |
also just noting over here that our prune logic is going to become a) automated b) more powerful and c) more granular pretty soon: golang/dep#944 |
@sdboyer that sounds awesome! |
I have no idea how this will go, but let's try it!This actually seems like it could work!
What I did:
@BenTheElder @stevekuznetsov @fejta @Q-Lee