-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Use dep v0.4.1 in kubernetes/test-infra #6443
Conversation
IMO we should simplify update-deps.sh (remove the prune command, and the drop-dep calls) and still use it. |
For fun, I ran $ hack/prune-libraries.sh --fix
Unused libraries:
DEAD: //vendor/google.golang.org/appengine/urlfetch:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/urlfetch:go_default_library
DEAD: //vendor/google.golang.org/appengine:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/modules:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/app_identity:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/remote_api:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/log:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/datastore:go_default_library
DEAD: //vendor/google.golang.org/appengine/internal/base:go_default_library
DEAD: //vendor/golang.org/x/text/internal/ucd:go_default_library
DEAD: //vendor/golang.org/x/text/internal/triegen:go_default_library
DEAD: //vendor/golang.org/x/text/internal/gen:go_default_library
DEAD: //vendor/golang.org/x/text/unicode/cldr:go_default_library
DEAD: //vendor/github.com/spf13/cobra/cobra/cmd:go_default_library
DEAD: //vendor/github.com/petar/GoLLRB/llrb:go_default_library
Cleaning up unused dependencies...
INFO: Empty results
All remaining go libraries are alive |
dd7db8b
to
25eaf88
Compare
And I pushed one more commit which simplifies real 0m50.324s
user 0m15.784s
sys 0m5.880s We probably need some way to ensure that the right version of dep is being used. |
Simple solution would be to put the script in a presubmit and validate no changes occur? |
/meow box |
/joke |
@krzyzacy: My wife is on a tropical fruit diet, the house is full of stuff. It is enough to make a mango crazy. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/meow boxes |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
/lgtm
/hold
We should consider integrating prune-libraries.sh into update-bazel.sh
hack/update-deps.sh
Outdated
hack/update-bazel.sh | ||
drop-dep vendor/golang.org/x/text/language vendor/golang.org/x/text/internal |
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.
So amazing! Drop the drop-dep
method?
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
I was able to vendor dep and get it to run under |
585cd95
to
64d6866
Compare
Rebased and added |
64d6866
to
fd73b49
Compare
fd73b49
to
ecb0383
Compare
fixes #5488 |
I guess we don't actually have any presubmits checking this, yet. We should figure out how to do that... I'm not sure how to query bazel from inside a test running under bazel, though. |
I have a somewhat clever way of making |
So are we generally expecting PR reviewers to notice a huge diff in |
/meow space |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Any updates on this? imho we should get this finished and in and follow-up with a presubmit later. |
waiting on final approval for kubernetes/repo-infra#60. |
Seems I was only looking for confirmation on nits. Maybe we should merge than and nits can be addressed later? |
OK, this PR is officially ginormous. I've added several additional commits:
PTAL |
We need a new size label? 😂 |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@BenTheElder: Do you want a brief explanation of what an acorn is? In a nutshell, it's an oak tree. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
scripts, bazel, dep config, presubmit LGTM. vendor/
== 🤷♂️
/lgtm
/hold
/shrug
/meow space
/woof
/joke
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, fejta, ixdy 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 |
better yet since this is all about vendor and sandboxing: |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ok, I'm reasonably sure this works. /unhold |
I'll note that the verify job I added here is only running |
@ixdy: I updated Prow config for you! In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The verify job appears to be basically doing the right thing - it at least verifies that you've run |
It does seem like the new release of dep does a better job of removing unused libraries.
In this PR, I enabled the new prune options (I turned off pruning non-go files, because Bazel).
I then ran
dep ensure
followed byhack/update-bazel.sh
- I didn't run any of @fejta's other dep scripts inhack/
. It seems like it removed some dependencies that the scripts missed, but potentially added some it had removed?Amazingly,
bazel build ... && bazel test ...
still works.fixes #5488