presubmit: check import path ordering (#3636)

Add a test for this as well as it's annoying to point out in every code
review.
Fix all the import paths that are flagged by this new test.

Fixes: #3634
Signed-off-by: Miek Gieben <miek@miek.nl>

coredns.go

@@ -4,10 +4,8 @@ package main
 //go:generate go run owners_generate.go
 
 import (
+	_ "github.com/coredns/coredns/core/plugin" // Plug in CoreDNS.
 	"github.com/coredns/coredns/coremain"
-
-	// Plug in CoreDNS
-	_ "github.com/coredns/coredns/core/plugin"
 )
 
 func main() {

plugin/cache/prefech_test.go

@@ -8,8 +8,8 @@ import (
 
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
-
 	"github.com/coredns/coredns/plugin/test"
+
 	"github.com/miekg/dns"
 )
 

plugin/dnssec/dnskey.go

@@ -9,8 +9,8 @@ import (
 	"time"
 
 	"github.com/coredns/coredns/request"
-	"github.com/miekg/dns"
 
+	"github.com/miekg/dns"
 	"golang.org/x/crypto/ed25519"
 )
 

plugin/etcd/etcd.go

@@ -12,9 +12,9 @@ import (
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/etcd/msg"
 	"github.com/coredns/coredns/plugin/pkg/fall"
+	"github.com/coredns/coredns/plugin/pkg/upstream"
 	"github.com/coredns/coredns/request"
 
-	"github.com/coredns/coredns/plugin/pkg/upstream"
 	"github.com/miekg/dns"
 	etcdcv3 "go.etcd.io/etcd/clientv3"
 	"go.etcd.io/etcd/mvcc/mvccpb"

plugin/kubernetes/ns_test.go

@@ -5,8 +5,8 @@ import (
 	"testing"
 
 	"github.com/coredns/coredns/plugin/kubernetes/object"
-	"github.com/miekg/dns"
 
+	"github.com/miekg/dns"
 	api "k8s.io/api/core/v1"
 )
 

plugin/kubernetes/setup.go

@@ -19,18 +19,11 @@ import (
 	"github.com/caddyserver/caddy"
 	"github.com/miekg/dns"
 	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	// Pull this in setting klog's output to stdout
-	"k8s.io/klog"
-
-	// Excluding azure because it is failing to compile
-	// pull this in here, because we want it excluded if plugin.cfg doesn't have k8s
-	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
-	// pull this in here, because we want it excluded if plugin.cfg doesn't have k8s
-	_ "k8s.io/client-go/plugin/pkg/client/auth/oidc"
-	// pull this in here, because we want it excluded if plugin.cfg doesn't have k8s
-	_ "k8s.io/client-go/plugin/pkg/client/auth/openstack"
+	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"       // pull this in here, because we want it excluded if plugin.cfg doesn't have k8s
+	_ "k8s.io/client-go/plugin/pkg/client/auth/oidc"      // pull this in here, because we want it excluded if plugin.cfg doesn't have k8s
+	_ "k8s.io/client-go/plugin/pkg/client/auth/openstack" // pull this in here, because we want it excluded if plugin.cfg doesn't have k8s
 	"k8s.io/client-go/tools/clientcmd"
+	"k8s.io/klog"
 )
 
 var log = clog.NewWithPlugin("kubernetes")

plugin/pkg/upstream/upstream.go

@@ -5,11 +5,11 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/miekg/dns"
-
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin/pkg/nonwriter"
 	"github.com/coredns/coredns/request"
+
+	"github.com/miekg/dns"
 )
 
 // Upstream is used to resolve CNAME or other external targets via CoreDNS itself.

plugin/test/scrape.go

@@ -30,9 +30,8 @@ import (
 	"strconv"
 
 	"github.com/matttproud/golang_protobuf_extensions/pbutil"
-	"github.com/prometheus/common/expfmt"
-
 	dto "github.com/prometheus/client_model/go"
+	"github.com/prometheus/common/expfmt"
 )
 
 type (

plugin/trace/trace.go

@@ -11,16 +11,14 @@ import (
 	"github.com/coredns/coredns/plugin/metrics"
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/pkg/rcode"
-	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/opentracer"
-	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
-
-	// Plugin the trace package.
-	_ "github.com/coredns/coredns/plugin/pkg/trace"
+	_ "github.com/coredns/coredns/plugin/pkg/trace" // Plugin the trace package.
 	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
 	ot "github.com/opentracing/opentracing-go"
 	zipkin "github.com/openzipkin-contrib/zipkin-go-opentracing"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/opentracer"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 )
 
 const (

test/grpc_test.go

@@ -5,10 +5,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/coredns/coredns/pb"
+
 	"github.com/miekg/dns"
 	"google.golang.org/grpc"
-
-	"github.com/coredns/coredns/pb"
 )
 
 func TestGrpc(t *testing.T) {

test/presubmit_test.go

@@ -208,3 +208,106 @@ func hasImportTesting(path string, info os.FileInfo, _ error) error {
 	}
 	return nil
 }
+
+func TestImportOrdering(t *testing.T) {
+	err := filepath.Walk("..", hasImportOrdering)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func hasImportOrdering(path string, info os.FileInfo, _ error) error {
+	if !info.Mode().IsRegular() {
+		return nil
+	}
+	if strings.HasPrefix(path, "../.") {
+		return nil
+	}
+	if filepath.Ext(path) != ".go" {
+		return nil
+	}
+
+	fs := token.NewFileSet()
+	f, err := parser.ParseFile(fs, path, nil, parser.AllErrors)
+	if err != nil {
+		return err
+	}
+	if len(f.Imports) == 0 {
+		return nil
+	}
+
+	// 3 blocks total, if
+	// 3 blocks: std + coredns + 3rd party
+	// 2 blocks: std + coredns, std + 3rd party, coredns + 3rd party
+	// 1 block: std, coredns, 3rd party
+	// first entry in a block specifies the type (std, coredns, 3rd party)
+	// we want: std, coredns, 3rd party
+	// more than 3 blocks as an error
+	blocks := [3][]*ast.ImportSpec{}
+	prevpos := 0
+	bl := 0
+	for _, im := range f.Imports {
+		line := fs.Position(im.Path.Pos()).Line
+		if line-prevpos > 1 && prevpos > 0 {
+			bl++
+		}
+		if bl > 2 {
+			return fmt.Errorf("more than %d import blocks in %q", bl, path)
+		}
+		blocks[bl] = append(blocks[bl], im)
+		prevpos = line
+	}
+	// if it:
+	// contains strings github.com/coredns/coredns -> coredns
+	// contains dots -> 3rd
+	// no dots -> std
+	ip := [3]string{} // type per block, just string, either std, coredns, 3rd
+	for i := 0; i <= bl; i++ {
+		ip[i] = importtype(blocks[i][0].Path.Value)
+	}
+
+	// Ok, now that we have the type, let's see if all members adhere to it.
+	// After that we check if the are in the right order.
+	for i := 0; i < bl; i++ {
+		for _, p := range blocks[i] {
+			t := importtype(p.Path.Value)
+			if t != ip[i] {
+				return fmt.Errorf("import path for %s is not of the same type %q in %q", p.Path.Value, ip[i], path)
+			}
+		}
+	}
+
+	// check order
+	switch bl {
+	case 0:
+		// we don't care
+	case 1:
+		if ip[0] == "std" && ip[1] == "coredns" {
+			break // OK
+		}
+		if ip[0] == "std" && ip[1] == "3rd" {
+			break // OK
+		}
+		if ip[0] == "coredns" && ip[1] == "3rd" {
+			break // OK
+		}
+		return fmt.Errorf("import path in %q are not in the right order (std -> coredns -> 3rd)", path)
+	case 2:
+		if ip[0] == "std" && ip[1] == "coredns" && ip[2] == "3rd" {
+			break // OK
+		}
+		return fmt.Errorf("import path in %q are not in the right order (std -> coredns -> 3rd)", path)
+	}
+
+	return nil
+}
+
+func importtype(s string) string {
+	if strings.Contains(s, "github.com/coredns/coredns") {
+		return "coredns"
+	}
+	if strings.Contains(s, ".") {
+		return "3rd"
+	}
+	return "std"
+}

test/server.go

@@ -3,11 +3,9 @@ package test
 import (
 	"sync"
 
+	_ "github.com/coredns/coredns/core" // Hook in CoreDNS.
 	"github.com/coredns/coredns/core/dnsserver"
-	// Hook in CoreDNS.
-	_ "github.com/coredns/coredns/core"
-	// Load all managed plugins in github.com/coredns/coredns
-	_ "github.com/coredns/coredns/core/plugin"
+	_ "github.com/coredns/coredns/core/plugin" // Load all managed plugins in github.com/coredns/coredns.
 
 	"github.com/caddyserver/caddy"
 )