godep-save.sh: add sanity checks #48615

sttts · 2017-07-07T16:46:08Z

Might prevent #48593.

cblecker · 2017-07-07T16:54:21Z

hack/godep-save.sh

 pushd "${KUBE_ROOT}" > /dev/null
+  # sanity check that staging directories do not exist in GOPATH


Do they need to actually be removed? Before running a godep save? Did this not work before?

~~We had reports a couple of times from people who failed with non-trivial GOPATHs. I would better check that.~~

The magic with the staging dirs needs that.

cblecker · 2017-07-07T16:56:06Z

hack/godep-save.sh

 pushd "${KUBE_ROOT}" > /dev/null
+  # sanity check that staging directories do not exist in GOPATH
+  for R in "${STAGING_REPOS[@]}"; do
+    if [ -e "${GOPATH}/src/${R}" ]; then


This will error one directory at a time if all 8 staging repos exist, meaning unless the user knows to delete all 8 the first time they hit this, they'll have to run the script 8 times to get all the dirs they need to remove. Perhaps we could collect all the dirs together and output the message once?

cblecker · 2017-07-07T16:56:47Z

hack/godep-save.sh

@@ -33,35 +35,35 @@ REQUIRED_BINS=(
  "./..."
 )

+STAGING_REPOS=(


Can this be dynamic like https://github.com/kubernetes/kubernetes/blob/master/hack/update-staging-godeps.sh#L81 instead of a static list?

Very good idea!

cblecker · 2017-07-07T16:57:23Z

hack/lib/util.sh

@@ -513,6 +513,14 @@ kube::util::ensure_godep_version() {
  godep version
 }

+# Checks that the GOPATH is simple, i.e. consists only of one directory, not multiple.
+kube::util::ensure_single_dir_gopath() {


I like this 👍 ! Would be useful to add on to the staging godeps dockerized script I've got too.

sttts · 2017-07-07T17:09:51Z

@cblecker ptal

cblecker · 2017-07-07T17:15:23Z

/lgtm
Can you add #48593 as a ref to the body of the PR for the approval bot?

cblecker · 2017-07-07T17:15:55Z

/assign @fejta
for approval

smarterclayton · 2017-07-07T17:35:15Z

/approve

sttts · 2017-07-07T17:42:32Z

@cblecker add another small test: vendor/ and Godeps/ must be deleted. Thanks @nikhita for the idea :)

cblecker · 2017-07-07T18:09:44Z

@sttts I think you must have picked up an extra commit in a rebase somehow? I don't think 9125cae is supposed to be there.

sttts · 2017-07-07T18:29:57Z

@cblecker oops. fixing.

sttts · 2017-07-07T18:30:28Z

@cblecker done

cblecker · 2017-07-07T18:36:44Z

hack/godep-save.sh

+  fi
+
+  echo "Deleting vendor/ and Godeps/"
+  rm -rf "${KUBE_ROOT}/vendor" "${KUBE_ROOT}/Godeps"


This makes me uncomfortable. I don't see any other hack/ scripts that directly rm -rf anything outside of a temp directory... Perhaps we could warn/error like above, rather than actually rm the directories ourselves?

Am also fine with a warning. I had that in fact as a warning before and then switch to an explicit removal.

Now kube::util::ensure_clean_working_dir complains that vendor/ and Godeps/ are missing 🙄

So we can choose:

kube::util::ensure_clean_working_dir and automatic rm -rf

or no clean working dir check, but an error if vendor/ or Godeps/ exists.

Which one do you like?

My suggestion would be no clean working dir check, and a error for now. I'd be okay with swapping it to the first option if/when these scripts move into a docker container (less risk to a bad delete, and the changes would then be rsync'd out)

good point @ dockerized

fejta · 2017-07-07T18:55:35Z

/unassign
/assign @smarterclayton

cblecker

/lgtm

k8s-github-robot · 2017-07-07T19:20:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, smarterclayton, sttts

Associated issue: 48593

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

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-07T21:48:34Z

Automatic merge from submit-queue (batch tested with PRs 48374, 48524, 48519, 42548, 48615)

@sttts

Automatic merge from submit-queue (batch tested with PRs 47040, 48597, 48608, 48653) Fix godep verify to use godep restore script **What this PR does / why we need it**: A bug was introduced in #48615. `hack/verify-godeps.sh` only downloads and compares if godeps have changed, so it wasn't caught on the original PR. However, when it does run (e.g. https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/48630/pull-kubernetes-verify/38350/) it fails because the godep-save script now doesn't permit a compex GOPATH. verify-godeps.sh actually sets one because it restores godeps not using the `hack/godep-restore.sh` script. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note NONE ``` /assign @sttts /priority failing-test

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2017

k8s-github-robot assigned fejta and spxtr Jul 7, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 7, 2017

sttts force-pushed the sttts-sanity-check-godep-save branch from 02addaf to 8bce391 Compare July 7, 2017 16:50

sttts assigned cblecker and unassigned fejta and spxtr Jul 7, 2017

cblecker reviewed Jul 7, 2017

View reviewed changes

sttts force-pushed the sttts-sanity-check-godep-save branch from 8bce391 to 5b9c339 Compare July 7, 2017 17:06

sttts added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 7, 2017

sttts mentioned this pull request Jul 7, 2017

hack/godep-save.sh pulls down docker/docker/project dir #48593

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017

k8s-ci-robot assigned fejta Jul 7, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2017

sttts force-pushed the sttts-sanity-check-godep-save branch from 5b9c339 to 5328d4e Compare July 7, 2017 17:41

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017

sttts force-pushed the sttts-sanity-check-godep-save branch 2 times, most recently from 60cb000 to 8637a43 Compare July 7, 2017 18:04

sttts force-pushed the sttts-sanity-check-godep-save branch from 8637a43 to 4522505 Compare July 7, 2017 18:30

cblecker reviewed Jul 7, 2017

View reviewed changes

k8s-ci-robot assigned smarterclayton and unassigned fejta Jul 7, 2017

godep-save.sh: add sanity checks

9187210

sttts force-pushed the sttts-sanity-check-godep-save branch from 4522505 to 9187210 Compare July 7, 2017 19:17

cblecker approved these changes Jul 7, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017

k8s-github-robot merged commit 95a4a5d into kubernetes:master Jul 7, 2017

cblecker mentioned this pull request Jul 8, 2017

Fix godep verify to use godep restore script #48653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

godep-save.sh: add sanity checks #48615

godep-save.sh: add sanity checks #48615

sttts commented Jul 7, 2017 •

edited

cblecker Jul 7, 2017

sttts Jul 7, 2017 •

edited

sttts Jul 7, 2017

sttts Jul 7, 2017

cblecker Jul 7, 2017

sttts Jul 7, 2017

cblecker Jul 7, 2017

sttts Jul 7, 2017

sttts Jul 7, 2017

cblecker Jul 7, 2017

sttts commented Jul 7, 2017

cblecker commented Jul 7, 2017

cblecker commented Jul 7, 2017

smarterclayton commented Jul 7, 2017

sttts commented Jul 7, 2017

cblecker commented Jul 7, 2017

sttts commented Jul 7, 2017

sttts commented Jul 7, 2017

cblecker Jul 7, 2017

sttts Jul 7, 2017

sttts Jul 7, 2017

sttts Jul 7, 2017

cblecker Jul 7, 2017

sttts Jul 7, 2017

sttts Jul 7, 2017

fejta commented Jul 7, 2017

cblecker left a comment

k8s-github-robot commented Jul 7, 2017

k8s-github-robot commented Jul 7, 2017

		pushd "${KUBE_ROOT}" > /dev/null
		# sanity check that staging directories do not exist in GOPATH

godep-save.sh: add sanity checks #48615

godep-save.sh: add sanity checks #48615

Conversation

sttts commented Jul 7, 2017 • edited

Choose a reason for hiding this comment

sttts Jul 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 7, 2017

cblecker commented Jul 7, 2017

cblecker commented Jul 7, 2017

smarterclayton commented Jul 7, 2017

sttts commented Jul 7, 2017

cblecker commented Jul 7, 2017

sttts commented Jul 7, 2017

sttts commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta commented Jul 7, 2017

cblecker left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Jul 7, 2017

k8s-github-robot commented Jul 7, 2017

sttts commented Jul 7, 2017 •

edited

sttts Jul 7, 2017 •

edited