Skip to content
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

go_install_from_commit: copy vendored dependencies into GOPATH #64928

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented Jun 8, 2018

What this PR does / why we need it: go_install_from_commit is an abomination and I regret ever having written it.

gazelle recently vendored golang.org/x/tools/go/vcs (bazelbuild/bazel-gazelle#228).

As a result, go get -d github.com/bazelbuild/bazel-gazelle/cmd/gazelle no longer installs golang.org/x/tools/go/vcs into the GOPATH, and so when we check out the older commit of gazelle to install, this dependency is missing.

This PR adds another level of hackery by copying any vendor/ directory in the package path into the GOPATH before checking out the requested commit. This theoretically future-proofs us from similar breakages in the future, and using an arbitrary version of these dependencies from the vendor/ directory is no worse than go get -d downloading the dependencies from tip.

Cherry-picking #57600 is the better fix, but it's also more involved.

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 #64925

NONE

/assign @cblecker @BenTheElder

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 8, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 8, 2018
@k8s-ci-robot k8s-ci-robot requested review from eparis and jbeda June 8, 2018 18:02
# As a hacky workaround, copy the dependencies from tip into GOPATH.
(
cd "${KUBE_TEMP}/go/src/${pkg}"
while [[ "$(pwd)" != "${KUBE_TEMP}/go/src" && "$(pwd)" != "/" ]]; do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know temp directories on macOS are weird, so I'd appreciate someone with a Mac testing to make sure this works correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do. also @cblecker

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

held to test #64929 (comment) instead, running a few things to sanity check on mac

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2018
@k8s-github-robot k8s-github-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Jun 8, 2018
@k8s-github-robot
Copy link

This PR is not for the master branch but does not have the cherrypick-approved label. Adding the do-not-merge/cherry-pick-not-approved label.

@ixdy
Copy link
Member Author

ixdy commented Jun 8, 2018

/hold

The cherrypick PR #64929 is probably a better fix.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2018
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 8, 2018

@ixdy: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 222fb77 link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@ixdy
Copy link
Member Author

ixdy commented Jun 8, 2018

/close

in favor of #64929

k8s-github-robot pushed a commit that referenced this pull request Jun 11, 2018
…57600-#62499-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #61575: Remove all upstream BUILD, BUILD.bazel, and WORKSPACE #57600: Vendor gazelle #62499: Update kazel to include openapi tag detection fix

Cherry pick of #61575 #57600 #62499 on release-1.10.

#61575: Remove all upstream BUILD, BUILD.bazel, and WORKSPACE
#57600: Vendor gazelle
#62499: Update kazel to include openapi tag detection fix

This is an alternative to #64928, and would fix #64925.

We can't just cherry-pick #57600, since there was a prerequisite PR and a fixup PR afterwards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants