remove gometalinter (istio#12375)

* remove for good Signed-off-by: Kuat Yessenov <[email protected]> * typo Signed-off-by: Kuat Yessenov <[email protected]> * test Signed-off-by: Kuat Yessenov <[email protected]> * skip coverage Signed-off-by: Kuat Yessenov <[email protected]> * correct the comment Signed-off-by: Kuat Yessenov <[email protected]> * address todo Signed-off-by: Kuat Yessenov <[email protected]>
AmitKumarDas · Mar 14, 2019 · 7b50cb8 · 7b50cb8
1 parent 09c054d
commit 7b50cb8
Show file tree

Hide file tree

Showing 44 changed files with 83 additions and 34,421 deletions.
diff --git a/.gitignore b/.gitignore
@@ -27,9 +27,11 @@ lintconfig.gen.json
 .istiorc
 .istiorc.mk
 # codegen stuff
+bin/adapterlinter
 bin/protoc-gen-gogoslick*
 bin/protoc-min-version*
 bin/protoc-gen-docs*
+bin/testlinter
 # Install generated files
 install/consul/istio.yaml
 install/kubernetes/addons/grafana.yaml

diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/Gopkg.toml b/Gopkg.toml
@@ -25,9 +25,6 @@
 #   unused-packages = true
 
 required = [
-  ### BEGIN spelling check deps
-  "github.com/client9/misspell",
-  ### END spelling check deps
   ### BEGIN Mixer codegen deps
   "github.com/istio/tools/protoc-gen-docs",
   ### END Mixer codegen deps

diff --git a/bin/linters.sh b/bin/linters.sh
@@ -38,42 +38,6 @@ function check_licenses() {
     echo 'licenses OK'
 }
 
-function has_latest_gometalinter() {
-    local local_binary
-    local lastest_version
-    local current_version
-
-    local_binary="${1}"
-    lastest_version="${2}"
-    current_version="$(${local_binary} --version 2>/dev/null | cut -d ' ' -f 3)"
-
-    if [ "${lastest_version}" != "${current_version}" ]; then
-        return 1
-    fi
-
-    return 0
-}
-
-function install_gometalinter() {
-    gometalinter=$(command -v gometalinter 2> /dev/null || echo "${ISTIO_BIN}/gometalinter")
-    latest_version=$(curl -L -s https://api.github.com/repos/alecthomas/gometalinter/releases/latest \
-      | grep tag_name | sed "s/ *\"tag_name\": *\"\\(.*\\)\",*/\\1/" | sed "s/v//")
-
-    if has_latest_gometalinter "${gometalinter}" "${latest_version}"; then
-      echo "Skipping gometalinter installation, we already have the latest version"
-      return 0
-    fi
-
-    echo 'Installing gometalinter ....'
-    curl -s "https://raw.githubusercontent.com/alecthomas/gometalinter/v${latest_version}/scripts/install.sh" | bash -s -- -b "${ISTIO_BIN}"
-    if [ ! -x "${ISTIO_BIN}/gometalinter" ]; then
-      echo "Installation of gometalinter failed"
-      exit 1
-    fi
-
-    echo 'Gometalinter installed successfully'
-}
-
 function install_golangcilint() {
     GOLANGCI_VERSION="v1.15.0"
     curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b "$GOPATH"/bin "$GOLANGCI_VERSION"
@@ -82,21 +46,15 @@ function install_golangcilint() {
 
 function run_adapter_lint() {
     echo 'Running adapterlinter ....'
-    pushd mixer/tools/adapterlinter
-    go install .
-    popd
-
-    $gometalinter --config=./mixer/tools/adapterlinter/gometalinter.json ./mixer/adapter/...
-    echo 'gometalinter on adapters OK'
+    go build -o bin/adapterlinter mixer/tools/adapterlinter/main.go
+    bin/adapterlinter ./mixer/adapter/...
+    echo 'adapterlinter OK'
 }
 
 function run_test_lint() {
     echo 'Running testlinter ...'
-    pushd tests/util/checker/testlinter
-    go install .
-    popd
-
-    $gometalinter --config=./tests/util/checker/testlinter/testlinter.json ./...
+    go build -o bin/testlinter tests/util/checker/testlinter/*.go
+    bin/testlinter
     echo 'testlinter OK'
 }
 
@@ -127,7 +85,6 @@ ensure_pilot_types
 check_licenses
 install_golangcilint
 run_golangcilint
-install_gometalinter
 run_adapter_lint
 run_test_lint
 run_helm_lint

diff --git a/codecov.skip b/codecov.skip
@@ -6,6 +6,7 @@ istio.io/istio/mixer/pkg/perf
 istio.io/istio/mixer/pkg/runtime/testing
 istio.io/istio/mixer/template/sample
 istio.io/istio/mixer/tools/codegen
+istio.io/istio/mixer/tools/adapterlinter
 istio.io/istio/pilot/test
 istio.io/istio/pkg/mcp/testing
 istio.io/istio/pkg/test

diff --git a/mixer/tools/adapterlinter/gometalinter.json b/mixer/tools/adapterlinter/gometalinter.json
diff --git a/mixer/tools/adapterlinter/main.go b/mixer/tools/adapterlinter/main.go
@@ -20,7 +20,9 @@ import (
 	"go/ast"
 	"go/parser"
 	"go/token"
+	"io/ioutil"
 	"os"
+	"path"
 	"sort"
 	"strings"
 )
@@ -36,54 +38,79 @@ var invalidImportPaths = map[string]string{
 		"wants to see it.",
 }
 
+const (
+	dots = "/..."
+)
+
 func main() {
 	flag.Parse()
 	for _, r := range getReport(flag.Args()) {
-		reportErr(r)
+		// check if the line in the file has nolint annotation
+		if dat, err := ioutil.ReadFile(r.file); err == nil {
+			lines := strings.Split(string(dat), "\n")
+			line := strings.Replace(lines[r.line-1], " ", "", -1)
+			if strings.Contains(line, "nolint:adapterlinter") {
+				continue
+			}
+		}
+
+		reportErr(r.msg)
 	}
 	os.Exit(exitCode)
 }
 
-func getReport(args []string) []string {
-	var reports []string
+func getReport(args []string) reports {
+	var reports reports
 	if len(args) == 0 {
-		reports = doAllDirs([]string{"."})
+		reports = doDir(".")
 	} else {
 		reports = doAllDirs(args)
 	}
 	return reports
 }
 
-func doAllDirs(args []string) []string {
-	reports := make([]string, 0)
+func doAllDirs(args []string) reports {
+	reports := make(reports, 0)
 	for _, name := range args {
-		// Is it a directory?
-		if fi, err := os.Stat(name); err == nil && fi.IsDir() {
-			for _, r := range doDir(name) {
-				reports = append(reports, r.msg)
-			}
-		} else {
-			reportErr(fmt.Sprintf("not a directory: %s", name))
+		for _, r := range doDir(name) {
+			reports = append(reports, r)
 		}
 	}
 	return reports
 }
 
+func notests(info os.FileInfo) bool {
+	if !info.IsDir() && strings.HasSuffix(info.Name(), ".go") &&
+		!strings.HasSuffix(info.Name(), "_test.go") {
+		return true
+	}
+	return false
+}
+
 func doDir(name string) reports {
-	notests := func(info os.FileInfo) bool {
-		if !info.IsDir() && strings.HasSuffix(info.Name(), ".go") &&
-			!strings.HasSuffix(info.Name(), "_test.go") {
-			return true
+	rpts := make(reports, 0)
+	if strings.HasSuffix(name, dots) {
+		name = name[:len(name)-len(dots)]
+
+		// depth first recurse into subdirectories
+		files, err := ioutil.ReadDir(name)
+		if err != nil {
+			reportErr(err.Error())
+			return nil
+		}
+		for _, file := range files {
+			if file.IsDir() {
+				rpts = append(rpts, doDir(path.Join(name, file.Name())+dots)...)
+			}
 		}
-		return false
 	}
+
 	fs := token.NewFileSet()
 	pkgs, err := parser.ParseDir(fs, name, notests, parser.Mode(0))
 	if err != nil {
 		reportErr(fmt.Sprintf("%v", err))
 		return nil
 	}
-	rpts := make(reports, 0)
 	for _, pkg := range pkgs {
 		rpts = append(rpts, doPackage(fs, pkg)...)
 	}
@@ -140,6 +167,8 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor {
 func (v *visitor) invalidImportReport(pos token.Pos, msg string) report {
 	return report{
 		pos,
+		v.fs.Position(pos).Filename,
+		v.fs.Position(pos).Line,
 		fmt.Sprintf("%v:%v:%v:%s",
 			v.fs.Position(pos).Filename,
 			v.fs.Position(pos).Line,
@@ -151,6 +180,8 @@ func (v *visitor) invalidImportReport(pos token.Pos, msg string) report {
 func (v *visitor) goroutineReport(pos token.Pos) report {
 	return report{
 		pos,
+		v.fs.Position(pos).Filename,
+		v.fs.Position(pos).Line,
 		fmt.Sprintf("%v:%v:%v:Adapters must use env.ScheduleWork or env.ScheduleDaemon in order to "+
 			"dispatch goroutines. This ensures all adapter goroutines are prevented from crashing Mixer as a "+
 			"whole by catching any panics they produce.",
@@ -159,8 +190,10 @@ func (v *visitor) goroutineReport(pos token.Pos) report {
 }
 
 type report struct {
-	pos token.Pos
-	msg string
+	pos  token.Pos
+	file string
+	line int
+	msg  string
 }
 
 type reports []report

diff --git a/mixer/tools/adapterlinter/main_test.go b/mixer/tools/adapterlinter/main_test.go
@@ -21,7 +21,12 @@ import (
 )
 
 func TestDoAllDirs(t *testing.T) {
-	got := doAllDirs([]string{"testdata/bad"})
+	reports := doAllDirs([]string{"testdata/bad"})
+
+	got := make([]string, len(reports))
+	for i := range reports {
+		got[i] = reports[i].msg
+	}
 
 	sort.Strings(got)
 	want := []string{
@@ -46,23 +51,23 @@ func TestDoAllDirs(t *testing.T) {
 func TestDoAllDirsBadPath(t *testing.T) {
 	// check no panics and no reports
 	got := getReport([]string{"testdata/unknown"})
-	want := []string{}
+	want := reports{}
 	if !reflect.DeepEqual(want, got) {
 		t.Errorf("errors don't match\nwant:%v\ngot :%v", want, got)
 	}
 }
 
 func TestDoAllDirsGood(t *testing.T) {
 	got := getReport([]string{"testdata/bad2"})
-	want := []string{}
+	want := reports{}
 	if !reflect.DeepEqual(want, got) {
 		t.Errorf("errors don't match\nwant:%v\ngot :%v", want, got)
 	}
 }
 
 func TestDoAllDirsCurrentDir(t *testing.T) {
 	got := getReport([]string{})
-	want := []string{}
+	want := reports{}
 	if !reflect.DeepEqual(want, got) {
 		t.Errorf("errors don't match\nwant:%v\ngot :%v", want, got)
 	}

diff --git a/pilot/docker/Dockerfile.lint b/pilot/docker/Dockerfile.lint
diff --git a/tests/util/checker/checker.go b/tests/util/checker/checker.go
@@ -21,6 +21,12 @@ import (
 	"go/token"
 	"os"
 	"path/filepath"
+	"strings"
+)
+
+var (
+	// IgnoreTestLinterData skips over unit tests
+	IgnoreTestLinterData = true
 )
 
 // Check checks the list of files, and write to the given Report.
@@ -53,6 +59,11 @@ func Check(paths []string, factory RulesFactory, whitelist *Whitelist, report *R
 
 // fileCheck checks a file using the given rules, and write to the given Report.
 func fileCheck(path string, rules []Rule, whitelist *Whitelist, report *Report) {
+	// TODO: skip over linter tests in a principled manner for all linters
+	if IgnoreTestLinterData && strings.Contains(path, "testlinter/testdata") {
+		return
+	}
+
 	fs := token.NewFileSet()
 	astFile, err := parser.ParseFile(fs, path, nil, parser.Mode(0))
 	if err != nil {

diff --git a/tests/util/checker/testlinter/gometalinter.json b/tests/util/checker/testlinter/gometalinter.json
diff --git a/tests/util/checker/testlinter/testlinter.json b/tests/util/checker/testlinter/testlinter.json