Merge pull request #1034 from grafana/dev-qa

better qa scripts

.circleci/config.yml

@@ -33,12 +33,13 @@ jobs:
     steps:
       - checkout
       - run: scripts/qa/gofmt.sh
-      - run: scripts/qa/go-generate.sh
+      - run: scripts/qa/go-generate.sh --force
       - run: scripts/qa/ineffassign.sh
       - run: scripts/qa/misspell.sh
       - run: scripts/qa/gitignore.sh
       - run: scripts/qa/unused.sh
       - run: scripts/qa/vendor.sh
+      - run: scripts/qa/vet-high-confidence.sh
 
   qa-post-build:
     working_directory: /home/circleci/.go_workspace/src/github.com/grafana/metrictank

.gitignore

@@ -1,14 +1,13 @@
 .*.sw?
 .notouch
-build/
+/build
 /cmd/metrictank/metrictank
 /cmd/mt-aggs-explain/mt-aggs-explain
 /cmd/mt-explain/mt-explain
 /cmd/mt-index-cat/mt-index-cat
 /cmd/mt-index-migrate/mt-index-migrate
 /cmd/mt-kafka-mdm-sniff-out-of-order/mt-kafka-mdm-sniff-out-of-order
 /cmd/mt-kafka-mdm-sniff/mt-kafka-mdm-sniff
-/cmd/mt-replicator-via-tsdb/mt-replicator-via-tsdb
 /cmd/mt-schemas-explain/mt-schemas-explain
 /cmd/mt-split-metrics-by-ttl/mt-split-metrics-by-ttl
 /cmd/mt-store-cat/mt-store-cat

Makefile

@@ -11,6 +11,20 @@ bin-race:
 	./scripts/build.sh -race
 docker:
 	./scripts/build_docker.sh
+
+qa: bin
+	# regular qa steps (can run directly on code)
+	scripts/qa/gofmt.sh
+	scripts/qa/go-generate.sh
+	scripts/qa/ineffassign.sh
+	scripts/qa/misspell.sh
+	scripts/qa/gitignore.sh
+	scripts/qa/unused.sh
+	scripts/qa/vendor.sh
+	scripts/qa/vet-high-confidence.sh
+	# qa-post-build steps minus stack tests
+	scripts/qa/docs.sh
 all:
 	$(MAKE) bin
 	$(MAKE) docker
+	$(MAKE) qa

docs/CONTRIBUTING

@@ -6,6 +6,15 @@ Thanks for your interest in contributing to metrictank!
 * `make` to build binaries and docker image.
 * [metrics2docs](https://github.com/Dieterbe/metrics2docs) generates the metrics documentation for the [metrics page](https://github.com/grafana/metrictank/blob/master/docs/metrics.md) though it is a bit buggy and often manual intervention is needed.
 
+# Building
+
+* `make bin`: for building all binaries.
+* `make docker`: for building docker image (after building binaries)
+* `make test`: unit tests
+* `make qa`: run all QA (code quality) tests
+
+see the [Makefile](../Makefile) for more targets
+
 # When contributing PR's
 
 1. functions, methods, types should be clearly understandable, either through an obvious name, or documentation when needed.

scripts/qa/cyclo.sh → scripts/qa-subjective/cyclo.sh

@@ -1,4 +1,7 @@
 #!/bin/bash
+
+# finds highly complex (cyclomatic complexity) functions
+
 # find the dir we exist within...
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # and cd into root project dir

scripts/qa-subjective/errcheck.sh

@@ -0,0 +1,11 @@
+#!/bin/bash
+
+# finds unchecked errors
+
+# find the dir we exist within...
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+# and cd into root project dir
+echo $DIR
+cd ${DIR}/../..
+go get -u  github.com/kisielk/errcheck
+errcheck ./...

scripts/qa-subjective/golint.sh

@@ -0,0 +1,11 @@
+#!/bin/bash
+
+# finds all kinds of lint
+
+# find the dir we exist within...
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+# and cd into root project dir
+echo $DIR
+cd ${DIR}/../..
+go get -u golang.org/x/lint/golint
+golint $(go list ./... | grep -v vendor)

scripts/qa-subjective/vet.sh

@@ -0,0 +1,10 @@
+#!/bin/bash
+
+# runs all of the "go vet" checks
+
+# find the dir we exist within...
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+# and cd into root project dir
+cd ${DIR}/../..
+
+go vet ./...

scripts/qa/docs.sh

@@ -1,4 +1,6 @@
 #!/bin/bash
+# this script checks whether doc files that are auto-generated
+# have been updated
 # Find the directory we exist within
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 cd ${DIR}/../..

scripts/qa/gitignore.sh

@@ -1,23 +1,67 @@
 #!/bin/bash
+
+# this script checks whether we have gitignore rules for all binaries we compile
+
 # Find the directory we exist within
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 cd $DIR/../../cmd
 file=../.gitignore
 
-# NOTE: known limitation: does not clean up stale gitignore rules
 
 declare -a missing
+declare -a malformed
+declare -a extraneous
+
+ret=0
+
 for bin in *; do
 	rule="/cmd/$bin/$bin"
 	grep -q "^$rule$" $file || missing=("${missing[@]}" "$rule")
 done
 
+while read a b c; do
+	if [ -n "$c" ]; then
+		malformed=("${malformed[@]}" "/cmd/$a/$b/$c...")
+	elif [ "$a" != "$b" ]; then
+		malformed=("${malformed[@]}" "/cmd/$a/$b")
+	elif [ ! -d "$a" ]; then
+		extraneous=("${extraneous[@]}" "/cmd/$a/$b")
+	fi
+done < <(grep '^/cmd/' $file | sed -e 's#/cmd/##' -e 's#/# #g')
+
 if [ ${#missing[@]} -gt 0 ]; then
 	echo "missing the following rules:"
 	for rule in "${missing[@]}"; do
 		echo "$rule"
 	done
-	exit 2
+	ret=2
+fi
+
+if [ ${#malformed[@]} -gt 0 ]; then
+	echo "malformed rules:"
+	for rule in "${malformed[@]}"; do
+		echo "$rule"
+	done
+	ret=2
+fi
+
+if [ ${#extraneous[@]} -gt 0 ]; then
+	echo "extraneous rules:"
+	for rule in "${extraneous[@]}"; do
+		echo "$rule"
+	done
+	ret=2
+fi
+
+sorted=$(mktemp)
+LC_ALL=en_US sort $file > $sorted
+if ! cmp --silent $file $sorted; then
+	echo ".gitignore file needs sorting"
+	diff $file $sorted
+	rm $sorted
+	ret=2
 fi
 
-echo "gitignore rules for all binaries found"
+[ $ret -gt 0 ] && exit $ret
+
+echo "all gitignore rules for commands in sync with cmd directory"

scripts/qa/go-generate.sh

@@ -1,10 +1,21 @@
 #!/bin/bash
+
+# this script checks if running go-generate results in a git diff.
+# if so, you forgot to run go generate (using the latest tools)
+# note: if you already have dirty code changes before running this tool, it has no choice but to bail out
+# but in CI mode, run with '-f' (--force) flag, which will always apply the check, as the working copy should always be clean
+
 # find the dir we exist within...
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # and cd into root project dir
 cd ${DIR}/../..
 gopath=${GOPATH/:*/} # get the first dir
 
+if [ "$1" != "-f" -a "$1" != "--force" ]; then
+	out=$(git status --short)
+	[ -n "$out" ] && echo "WARNING: working copy dirty. cannot accurately operate. skipping test" && exit 0
+fi
+
 go get -u golang.org/x/tools/cmd/stringer github.com/tinylib/msgp
 
 go generate $(go list ./... | grep -v /vendor/)

scripts/qa/gofmt.sh

@@ -1,4 +1,8 @@
 #!/bin/bash
+
+# this script checks whether all files (except vendorred and generated files)
+# are properly formatted and simplified with gofmt.
+
 # find the dir we exist within...
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # and cd into root project dir

scripts/qa/ineffassign.sh

@@ -1,7 +1,12 @@
 #!/bin/bash
+
+# this tool detects "ineffectual assignments"
+# not very clear but you may find some more info on
+# https://github.com/gordonklaus/ineffassign
+
 # find the dir we exist within...
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # and cd into root project dir
 cd ${DIR}/../..
-go get -u -v github.com/gordonklaus/ineffassign
+go get -u github.com/gordonklaus/ineffassign
 ineffassign .

scripts/qa/misspell.sh

@@ -1,4 +1,7 @@
 #!/bin/bash
+
+# checks for misspelled words
+
 # find the dir we exist within...
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # and cd into root project dir

scripts/qa/unused.sh

@@ -1,13 +1,17 @@
 #!/bin/bash
+
+# runs the tools unused, varcheck and structcheck
+# see their websites for more info.
+
 # find the dir we exist within...
 DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
 # and cd into root project dir
 cd ${DIR}/../..
-which unused &>/dev/null || go get honnef.co/go/tools/cmd/unused
-which varcheck &>/dev/null || go get github.com/opennota/check/cmd/varcheck
-which structcheck &>/dev/null || go get github.com/opennota/check/cmd/structcheck
+go get -u honnef.co/go/tools/cmd/unused
+go get -u github.com/opennota/check/cmd/varcheck
+go get -u github.com/opennota/check/cmd/structcheck
 # for https://github.com/remyoudompheng/go-misc/pull/14
-which deadcode &>/dev/null || go get -u github.com/Dieterbe/go-misc/deadcode
+go get -u github.com/Dieterbe/go-misc/deadcode
 
 ret=0
 

scripts/qa/vendor.sh

@@ -1,19 +1,9 @@
 #!/bin/bash
 
-if ! which dep >/dev/null; then
-	go get -u github.com/golang/dep/cmd/dep || exit 1
-fi
+# checks whether vendor directory is healthy
 
-dep version
-
-# until we have https://docs.google.com/document/d/1j_Hka8eFKqWwGJWFSFedtBsNkFaRN3yvL4g8k30PLmg/edit#
-# this should do fine:
-# (note dep ensure -dry-run and dep ensure would add a whole bunch of packages to vendor, which dep prune deletes again, so we can't just check those)
-# we can expect this to change soon though: https://github.com/golang/dep/issues/944
-
-dep ensure -no-vendor -dry-run
-ret=$?
+go get -u github.com/golang/dep/cmd/dep
 
+dep version
 dep status
-
-exit $ret
+dep check

scripts/qa/vet-high-confidence.sh

@@ -0,0 +1,13 @@
+#!/bin/bash
+# go vet has high confidence checks and low confidence ones.
+# for the purpose of CI, we shall only execute the high confidence ones.
+# we can just follow the ones `go test` uses, it's not documented, but
+# see https://github.com/golang/go/blob/release-branch.go1.10/src/cmd/go/internal/test/test.go#L509-L533
+# NOTE: why not just rely on the auto go vet execution via go test -> because not all packages use go test
+
+# find the dir we exist within...
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+# and cd into root project dir
+cd ${DIR}/../..
+
+go vet -atomic -bool -buildtags -nilfunc -printf ./...