Add utility to prune unused go library targets #6149
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
I know you are trying to reduce the files in vendor folder, and I like that. A challenge is when you add a new library and are not using the library, but will use it in other PRs that are inbound. I would rather not vendor a dependency, and add code for that dependency in the same PR, as the PR review can be nutty. What is the workflow that is used in this repo? Are we going to run into the same problem I just ran into with the aws vendoring? I like reducing the number of files, but not sure how we cannot step on our own toes. |
|
@chrislovecnm Typically we include the dependencies in the same PR, but add them in a separate commit so that reviewers can filter changes to the interesting commits with the Github review UI. |
hack/prune-libraries.sh
Outdated
|
|
||
| # Output the target if it has only one dependency (itself) | ||
| is-unused() { | ||
| local n="$(bazel query "rdeps(//..., \"$1\", 1)" | wc -l)" |
There was a problem hiding this comment.
Don't mix scoping and evaluation or scoping will overwrite the return code of evaluation:
local n
n=$(...)|
|
||
| # Convert //foo/bar:stuff to //foo/bar | ||
| package-name() { | ||
| echo $1 | cut -d : -f 1 |
There was a problem hiding this comment.
There's probably a potential word-splitting bug here without quotes around $1
hack/prune-libraries.sh
Outdated
| # Output foo/bar/whatever.sh | ||
| local base=$(basename $i) | ||
| local dir=$(dirname $i) | ||
| echo ${dir:2}/$(echo $base | sed -e 's|:|/|1') |
There was a problem hiding this comment.
Not sure how you feel about this but could get rid of two exec calls by using the shell built-in string replace instead of echo | sed
echo "${dir:2}/${base/:/\/}"
k8s-ci-robot
added
do-not-merge/hold
hack/verify-vendor.sh
Outdated
|
|
||
| script="$(dirname ${BASH_SOURCE})/prune-libraries.sh" --check | ||
| if ! "$script" ; then | ||
| echo "Run $script -- fix" |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cjwagner, fejta 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 |
|
@chrislovecnm let's commit new vendored deps when they are used, as cjwagner describes (what we have been doing). /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
How do we get |
|
|
/assign @stevekuznetsov @ixdy @cjwagner