Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catalog dirs. #14787

aveshagarwal · 2017-06-20T20:34:16Z

@derekwaynecarr @eparis @stevekuznetsov @jpeeler

stevekuznetsov · 2017-06-20T20:38:46Z

hack/godep-save.sh

@@ -56,7 +56,13 @@ REQUIRED_BINS=(
 )

 TMPGOPATH=`mktemp -d`
-trap "rm -rf $TMPGOPATH" EXIT
+TMPCMDREPOS=`mktemp -d`


Don't use deprecated back-tick expansions, instead:

TMPGOPATH="$( mktemp -d )"

We put temporary dirs under ${BASETMPDIR}:

TMPGOPATH="$( mktemp --directory --tmpdir="${BASETMPDIR}" )"

stevekuznetsov · 2017-06-20T20:43:13Z

hack/godep-save.sh

-trap "rm -rf $TMPGOPATH" EXIT
+TMPCMDREPOS=`mktemp -d`
+function finish {
+    mv $TMPCMDREPOS/* cmd/


Always use braces and quote expansions:

mv "${TMPCMDREPOS}"/* cmd/

and likewise for other additions

eparis · 2017-06-20T20:43:57Z

uggh. Could we do it by making REQUIRED_BINS not have ./... and instead have:

[things and stuffs]
pkg/...
api/...
assets/...
contrib/...
 docs/...
examples/...
hack/...
images/...
pkg/...
test/...
tools/...
cmd/dockerregistry/...
cmd/gitserver/...
cmd/kubefed/...
cmd/oc/...
cmd/openshift/...

I'm sure there is some way to generate that. But just using that list will tell us if it works...

aveshagarwal · 2017-06-20T20:53:18Z

Could we do it by making REQUIRED_BINS not have ./... and instead have:

Though I understand its not everyday that we are adding top level dirs that should be handled by godep-save, but whenever it is done, that means remembering updating hack/godep-save.sh.

aveshagarwal · 2017-06-20T20:55:04Z

Could we do it by making REQUIRED_BINS not have ./... and instead have:

Also do we need to include files too? Like there is doc.go?

aveshagarwal · 2017-06-21T03:02:43Z

@eparis @stevekuznetsov updated PTAL.

stevekuznetsov · 2017-06-21T13:41:05Z

hack/godep-save.sh

@@ -164,8 +163,13 @@ fork-with-fake-packages github.com/docker/docker \
 # kubernetes tests, we have to extract the missing test dependencies and run godep-save again
 # until we converge. Because we rsync kubernetes itself above, two iterations should be enough.
 MISSING_TEST_DEPS=""
+ALL_DIRS="$(os::util::list_go_src_files | cut -d '/' -f 1-2 | sort -u | grep -v "^./cmd"|grep -v doc.go)"
+ALL_DIRS+=" $(os::util::list_go_src_files | grep "^./cmd/"| cut -d '/' -f 1-3 | LC_ALL=C sort -u|grep -v doc.go)"


We're doing this three times now -- let's make a function for it, slap it into hack/lib/util/misc.sh and not dupe it

This one is a bit different than the one in verify-govet.go as it also ignores doc.go (grep -v doc.go).

OK, so cut out the part that is shared maybe? There's too many pipes in general and too much shared for my liking -- suuuuper easy to have a visually indistinguishable change that breaks something between the three this way

eparis · 2017-06-21T14:01:00Z

hack/godep-save.sh

@@ -164,8 +163,13 @@ fork-with-fake-packages github.com/docker/docker \
 # kubernetes tests, we have to extract the missing test dependencies and run godep-save again
 # until we converge. Because we rsync kubernetes itself above, two iterations should be enough.
 MISSING_TEST_DEPS=""
+ALL_DIRS="$(os::util::list_go_src_files | cut -d '/' -f 1-2 | sort -u | grep -v "^./cmd"|grep -v doc.go)"


For predictability please use LC_ALL=C (agree it shouldn't matter)

The doc.go scares me a little. Would this work? What if other .go files show up in the root dir?

ALL_DIRS="$(os::util::list_go_src_files | cut -d '/' -f 1-2 | grep -v ".go$" | grep -v "^./cmd" | LC_ALL=C sort -u)

If you are concerned about other .go files show up in the root dir in future, why do want to have grep -v ".go$"? Thats why I was only excluding doc.go specifically not *.go.

Another thing to consider what if buildable *.go show up?

Then you'd end up with buildable.go/... in REQUIRED_ALL_DIRS, which would be bad.

eparis · 2017-06-21T14:05:58Z

hack/godep-save.sh

@@ -164,8 +163,13 @@ fork-with-fake-packages github.com/docker/docker \
 # kubernetes tests, we have to extract the missing test dependencies and run godep-save again
 # until we converge. Because we rsync kubernetes itself above, two iterations should be enough.
 MISSING_TEST_DEPS=""
+ALL_DIRS="$(os::util::list_go_src_files | cut -d '/' -f 1-2 | sort -u | grep -v "^./cmd"|grep -v doc.go)"
+ALL_DIRS+=" $(os::util::list_go_src_files | grep "^./cmd/"| cut -d '/' -f 1-3 | LC_ALL=C sort -u|grep -v doc.go)"


doc.go worries me similarly. But, that also doesn't seem to do what we need. Since it isn't excluding cc and sc.

@eparis what do you mean it is not excluding cc and sc? os::util::list_go_src_files already excludes cc and sc.

ohhhhhh, I'm an idiot.

aveshagarwal · 2017-06-21T18:34:54Z

@eparis @stevekuznetsov updated PTAL.

@stevekuznetsov I am not sure how to really do what you are suggesting as only 2 places I see these changes: verify-govet.sh and godep-save.sh, as not sure to find a common stuff that really helps. Do you have any suggestion?

stevekuznetsov · 2017-06-21T18:48:32Z

Not sure vetting and linting needs to care about the top-level doc.go either so it seems like both places could use the same manipulations to os::util::list_go_src_files

aveshagarwal · 2017-06-21T18:53:35Z

Not sure vetting and linting needs to care about the top-level doc.go either so it seems like both places could use the same manipulations to os::util::list_go_src_files

Its just not doc.go now (though those are the ones that exists today) but any *.go in top level origin/ and cmd/ dirs. Are we ok now to remove them from vetting and linting?

stevekuznetsov · 2017-06-21T20:07:18Z

I am very confident that there will never be any file other than the single doc.go at the top level or directly under cmd/.

aveshagarwal · 2017-06-21T23:02:56Z

@stevekuznetsov @eparis updated PTAL.

stevekuznetsov · 2017-06-22T01:07:50Z

hack/lib/util/misc.sh

+function os::util::list_go_src_dirs() {
+        local ALL_DIRS="$(os::util::list_go_src_files | cut -d '/' -f 1-2 | grep -v ".go$" | grep -v "^./cmd" | LC_ALL=C sort -u)"
+        ALL_DIRS+=" $(os::util::list_go_src_files | grep "^./cmd/"| cut -d '/' -f 1-3 | grep -v ".go$" | LC_ALL=C sort -u)"
+        echo "${ALL_DIRS}"


No need for echo or for collecting in a variable:

function os::util::list_go_src_dirs() { os::util::list_go_src_files | cut -d '/' -f 1-2 | grep -v ".go$" | grep -v "^./cmd" | LC_ALL=C sort -u os::util::list_go_src_files | grep "^./cmd/"| cut -d '/' -f 1-3 | grep -v ".go$" | LC_ALL=C sort -u }

…og dirs.

aveshagarwal · 2017-06-22T01:17:36Z

@stevekuznetsov updated PTAL.

eparis · 2017-06-22T15:48:00Z

seems good to me...
@aveshagarwal will you remind us all to merge on monday? (or is this blocking you on something you must get in today/tomorrow)

aveshagarwal · 2017-06-23T02:46:12Z

@eparis sure.

aveshagarwal · 2017-06-27T19:08:39Z

@eparis @stevekuznetsov Reminder for merge tag.

stevekuznetsov · 2017-06-27T19:33:18Z

[merge]

openshift-bot · 2017-06-27T19:39:41Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 1

openshift-bot · 2017-06-27T19:39:42Z

Evaluated for origin merge up to e1b2918

aveshagarwal changed the title ~~Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-cataog dirs.~~ Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catalog dirs. Jun 20, 2017

stevekuznetsov suggested changes Jun 20, 2017

View reviewed changes

aveshagarwal force-pushed the master-origin-issue-14751 branch from 8d91c3f to 352e165 Compare June 21, 2017 03:01

stevekuznetsov suggested changes Jun 21, 2017

View reviewed changes

eparis reviewed Jun 21, 2017

View reviewed changes

aveshagarwal force-pushed the master-origin-issue-14751 branch from 352e165 to 3ca7432 Compare June 21, 2017 18:32

aveshagarwal force-pushed the master-origin-issue-14751 branch from 3ca7432 to da2edb1 Compare June 21, 2017 23:02

stevekuznetsov reviewed Jun 22, 2017

View reviewed changes

Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catal…

e1b2918

…og dirs.

aveshagarwal force-pushed the master-origin-issue-14751 branch from da2edb1 to e1b2918 Compare June 22, 2017 01:16

stevekuznetsov approved these changes Jun 22, 2017

View reviewed changes

smarterclayton merged commit 2197c0e into openshift:master Jun 27, 2017

aveshagarwal deleted the master-origin-issue-14751 branch July 24, 2017 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catalog dirs. #14787

Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catalog dirs. #14787

aveshagarwal commented Jun 20, 2017

stevekuznetsov Jun 20, 2017

stevekuznetsov Jun 20, 2017

stevekuznetsov Jun 20, 2017

stevekuznetsov Jun 20, 2017

eparis commented Jun 20, 2017

aveshagarwal commented Jun 20, 2017

aveshagarwal commented Jun 20, 2017 •

edited

Loading

aveshagarwal commented Jun 21, 2017

stevekuznetsov Jun 21, 2017

aveshagarwal Jun 21, 2017

stevekuznetsov Jun 21, 2017

eparis Jun 21, 2017

eparis Jun 21, 2017

aveshagarwal Jun 21, 2017 •

edited

Loading

aveshagarwal Jun 21, 2017

eparis Jun 21, 2017

eparis Jun 21, 2017

aveshagarwal Jun 21, 2017

eparis Jun 21, 2017

aveshagarwal commented Jun 21, 2017

stevekuznetsov commented Jun 21, 2017

aveshagarwal commented Jun 21, 2017

stevekuznetsov commented Jun 21, 2017

aveshagarwal commented Jun 21, 2017

stevekuznetsov Jun 22, 2017

aveshagarwal commented Jun 22, 2017

eparis commented Jun 22, 2017

aveshagarwal commented Jun 23, 2017

aveshagarwal commented Jun 27, 2017

stevekuznetsov commented Jun 27, 2017

openshift-bot commented Jun 27, 2017 •

edited

Loading

openshift-bot commented Jun 27, 2017

Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catalog dirs. #14787

Fix godep-save by ignoring cmd/cluster-capacity and cmd/service-catalog dirs. #14787

Conversation

aveshagarwal commented Jun 20, 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

eparis commented Jun 20, 2017

aveshagarwal commented Jun 20, 2017

aveshagarwal commented Jun 20, 2017 • edited Loading

aveshagarwal commented Jun 21, 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

aveshagarwal Jun 21, 2017 • edited Loading

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

aveshagarwal commented Jun 21, 2017

stevekuznetsov commented Jun 21, 2017

aveshagarwal commented Jun 21, 2017

stevekuznetsov commented Jun 21, 2017

aveshagarwal commented Jun 21, 2017

Choose a reason for hiding this comment

aveshagarwal commented Jun 22, 2017

eparis commented Jun 22, 2017

aveshagarwal commented Jun 23, 2017

aveshagarwal commented Jun 27, 2017

stevekuznetsov commented Jun 27, 2017

openshift-bot commented Jun 27, 2017 • edited Loading

openshift-bot commented Jun 27, 2017

aveshagarwal commented Jun 20, 2017 •

edited

Loading

aveshagarwal Jun 21, 2017 •

edited

Loading

openshift-bot commented Jun 27, 2017 •

edited

Loading