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

Fork godep to fix inconsistent abbreviation size #70718

Merged
merged 9 commits into from Nov 8, 2018

Conversation

@cblecker
Member

cblecker commented Nov 7, 2018

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?:

NONE

cblecker and others added some commits Oct 25, 2018

Use a fixed abbreviation size in `git describe` output
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.
@cblecker

This comment has been minimized.

Member

cblecker commented Nov 7, 2018

cblecker added some commits Nov 7, 2018

@cblecker

This comment has been minimized.

Member

cblecker commented Nov 7, 2018

/test pull-kubernetes-e2e-kops-aws

@thockin

This comment has been minimized.

Member

thockin commented Nov 7, 2018

Should we rename it so there's no ambiguity about which binary is being run? Maybe not important. Looking now.

@cblecker

This comment has been minimized.

Member

cblecker commented Nov 7, 2018

@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 k8s-ci-robot added the lgtm label Nov 8, 2018

@thockin

/lgtm
/approve

@@ -57,7 +57,6 @@ fi
REQUIRED_BINS=(
"github.com/onsi/ginkgo/ginkgo"
"github.com/jteeuwen/go-bindata/go-bindata"
"github.com/tools/godep"

This comment has been minimized.

@thockin

thockin Nov 8, 2018

Member

do we not want to ensure that the binary gets built?

This comment has been minimized.

@cblecker

cblecker Nov 8, 2018

Member

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

This comment has been minimized.

@thockin

thockin Nov 8, 2018

Member

Does this have implications for callers of this function who might find themselves in a new directory?

This comment has been minimized.

@cblecker

cblecker Nov 8, 2018

Member

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

This comment has been minimized.

Contributor

k8s-ci-robot commented Nov 8, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ixdy

This comment has been minimized.

Member

ixdy commented Nov 8, 2018

Should we rename it so there's no ambiguity about which binary is being run?

kodep? :)

@neolit123

This comment has been minimized.

Member

neolit123 commented Nov 8, 2018

ANAT (all names are taken)

@k8s-ci-robot k8s-ci-robot merged commit e998d6c into kubernetes:master Nov 8, 2018

18 checks passed

cla/linuxfoundation cblecker authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@cblecker cblecker deleted the cblecker:godep-round-a-million branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment