Fork godep to fix inconsistent abbreviation size #70718
Conversation
Currently, this setting depends on whatever the user of `godep` has in their `git config --get core.abbrev`, if it is set. Even when it is not set, the number of characters used in the shortened git commit will depend on whether there are collisions for that prefix on the local repository (which in large part depends on how much activity exists there.) As a result, when multiple users are maintaining Godeps/ for a single project, many spurious changes to Godeps/Godeps.json are generated due to the lack of stability in the length of the abbreviated commit in output of `git describe`. Let's fix this by enforcing a reasonable abbreviation length in godep's use of `git describe`. 14 characters is very unlikely to result in collisions for prefixes.
k8s-ci-robot
added
release-note-none
cblecker
changed the title
k8s-ci-robot
added
area/apiserver
area/code-generation
kind/api-change
cblecker
added
priority/important-soon
cblecker
force-pushed
the
godep-round-a-million
branch
from
882370b
to
c0967ed
Compare
k8s-ci-robot
added
area/apiserver
area/code-generation
kind/api-change
cblecker
removed
area/apiserver
area/code-generation
kind/api-change
|
/test pull-kubernetes-e2e-kops-aws |
|
Should we rename it so there's no ambiguity about which binary is being run? Maybe not important. Looking now. |
|
@thockin I changed the "version" to something we can verify and ensure we're running the right version, but we could easily rename the binary too. I'm indifferent. |
k8s-ci-robot
added
the
lgtm
| @@ -57,7 +57,6 @@ fi | |||
| REQUIRED_BINS=( | |||
| "github.com/onsi/ginkgo/ginkgo" | |||
| "github.com/jteeuwen/go-bindata/go-bindata" | |||
| "github.com/tools/godep" | |||
There was a problem hiding this comment.
do we not want to ensure that the binary gets built?
There was a problem hiding this comment.
This list is the "additional go repos to vendor that aren't explicit code dependencies" list. It's not exceptionally well named.
| kube::log::error " PATH: ${PATH}" | ||
| # Otherwise, install forked godep | ||
| kube::log::status "Installing godep version ${godep_target_version}" | ||
| # Run in hermetic GOPATH |
There was a problem hiding this comment.
Does this have implications for callers of this function who might find themselves in a new directory?
There was a problem hiding this comment.
Not that I've found in my testing. It doesn't impact the CWD or environment once the calling bash script itself exits out (either godep-restore or godep-save).
k8s-ci-robot
added
the
approved
kodep? :) |
|
ANAT (all names are taken) |
| # Otherwise, install forked godep | ||
| kube::log::status "Installing godep version ${godep_target_version}" | ||
| # Run in hermetic GOPATH | ||
| kube::golang::setup_env |
There was a problem hiding this comment.
I think this line is now causing GOPATH to be set to ${KUBE_GOPATH} (i.e. ${KUBE_ROOT}/_output/local/go) for the rest of the script invocation.
This means that hack/godep-restore.sh and hack/godep-save.sh are saving into _output/local/go now, which is maybe fine (the general recommendation is to run these under hack/run-in-gopath.sh), but it's also unexpected.
cc @apelisse
There was a problem hiding this comment.
Yup, you're right. Fix in #71538
What this PR does / why we need it:
This forks godep instead of vendoring it, and applies a fix to the abbreviation size.
Lots of tiny changes here. Best reviewed on a commit by commit basis.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #70686
Special notes for your reviewer:
This is just might be crazy enough to work.
Does this PR introduce a user-facing change?: