Fix local imports, add appropriate linter

1. Modify Makefile's format target to format imports in three groups:
 - standard library packages;
 - third-party packages;
 - local packages (i.e. ones with the `github.com/google/cadvisor` prefix).

   Note goimports does the same job as gofmt, plus imports sorting. It
   can't do what gofmt -s does, but it was not done before (although
   this is checked according to .golangci.yml), so let's leave it as is.

   Note we don't have to use $(pkgs) and $(cmd_pkgs) in Makefile's
   format target, as goimports works recursively given the current
   directory, and it also skips vendor dir.

2. Add goimports linter with proper configuration to check imports
   formatting in CI. Check that the linter complains.

3. Actually implements these changes by running "make format". Check
   that the linter no longer complains.

4. Fix presubmit target to make sure it checks the new formatting rules.
   Instead of modifying build/check_gofmt.sh, let's use golangci-lint
   which already does this check (and more). While at it, let's remove
   vet target, as lint (golangci-lint) already calls it.

   Now, since make presumbit is already there in test job, the whole
   lint job is superseded by it, so let's remove it. While doing it,
   make sure we use the code from lint job to run make presubmit, with
   the following two exceptions:
   - "set -e" is not required, as it is set by GHA CI;
   - GOFLAGS="$GO_FLAGS" is not required, as it is done by Makefile.

Signed-off-by: Kir Kolyshkin <[email protected]>

Author: kolyshkin

.github/workflows/test.yml

@@ -1,25 +1,6 @@
 name: Test
 on: [push, pull_request]
 jobs:
-  lint:
-    strategy:
-      matrix:
-        environment-variables: [build/config/plain.sh, build/config/libpfm4.sh, build/config/libipmctl.sh]
-    runs-on: ubuntu-20.04
-    timeout-minutes: 10
-    steps:
-    - name: Install Go
-      uses: actions/setup-go@v2
-      with:
-        go-version: 1.17
-    - name: Checkout code
-      uses: actions/checkout@v2
-    - name: Run golangci-lint
-      run: |
-        set -e
-        source ${{ matrix.environment-variables }}
-        if [[ "${BUILD_PACKAGES}" != "" ]]; then sudo apt-get update; sudo apt-get install ${BUILD_PACKAGES}; fi
-        make -e lint
   test:
     strategy:
       matrix:
@@ -36,7 +17,10 @@ jobs:
     - name: Checkout code
       uses: actions/checkout@v2
     - name: Run presubmit checks
-      run: GOFLAGS="$GO_FLAGS" make presubmit
+      run: |
+        source ${{ matrix.environment-variables }}
+        if [[ "${BUILD_PACKAGES}" != "" ]]; then sudo apt-get update; sudo apt-get install ${BUILD_PACKAGES}; fi
+        make -e presubmit
     - name: Run tests
       env:
         GOLANG_VERSION: ${{ matrix.go-versions }}

.golangci.yml

@@ -7,6 +7,8 @@ run:
       min-confidence: 0
     gofmt:
       simplify: true
+    goimports:
+      local-prefixes: github.com/google/cadvisor
 linters:
   disable-all: true
   enable:
@@ -22,6 +24,7 @@ linters:
     - typecheck
     - golint
     - gofmt
+    - goimports
 issues:
   max-issues-per-linter: 0
   max-same-issues: 0

Makefile

@@ -62,13 +62,8 @@ tidy:
 
 format:
 	@echo ">> formatting code"
-	@$(GO) fmt $(pkgs)
-	@cd cmd && $(GO) fmt $(cmd_pkgs)
-
-vet:
-	@echo ">> vetting code"
-	@$(GO) vet $(pkgs)
-	@cd cmd && $(GO) vet $(cmd_pkgs)
+	@# goimports is a superset of gofmt.
+	@goimports -w -local github.com/google/cadvisor .
 
 build: assets
 	@echo ">> building binaries"
@@ -88,9 +83,7 @@ docker-%:
 docker-build:
 	@docker run --rm -w /go/src/github.com/google/cadvisor -v ${PWD}:/go/src/github.com/google/cadvisor golang:1.17 make build
 
-presubmit: vet
-	@echo ">> checking go formatting"
-	@./build/check_gofmt.sh
+presubmit: lint
 	@echo ">> checking go mod tidy"
 	@./build/check_gotidy.sh
 	@echo ">> checking file boilerplate"
@@ -108,4 +101,4 @@ lint:
 clean:
 	@rm -f *.test cadvisor
 
-.PHONY: all build docker format release test test-integration vet lint presubmit tidy
+.PHONY: all build docker format release test test-integration lint presubmit tidy

accelerators/nvidia_test.go

@@ -14,12 +14,13 @@
 package accelerators
 
 import (
-	"github.com/google/cadvisor/stats"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"testing"
 
+	"github.com/google/cadvisor/stats"
+
 	"github.com/mindprince/gonvml"
 	"github.com/stretchr/testify/assert"
 )

build/check_gofmt.sh

@@ -28,7 +28,7 @@ import (
 	"path"
 	"strings"
 
-	"github.com/google/cadvisor/info/v1"
+	v1 "github.com/google/cadvisor/info/v1"
 
 	"k8s.io/klog/v2"
 )

client/client.go

@@ -27,7 +27,7 @@ import (
 	"strings"
 
 	v1 "github.com/google/cadvisor/info/v1"
-	"github.com/google/cadvisor/info/v2"
+	v2 "github.com/google/cadvisor/info/v2"
 )
 
 // Client represents the base URL for a cAdvisor client.

client/v2/client.go

@@ -24,9 +24,10 @@ import (
 	"testing"
 	"time"
 
-	"github.com/google/cadvisor/info/v1"
-	"github.com/google/cadvisor/info/v2"
 	"github.com/stretchr/testify/assert"
+
+	v1 "github.com/google/cadvisor/info/v1"
+	v2 "github.com/google/cadvisor/info/v2"
 )
 
 func cadvisorTestClient(path string, expectedPostObj *v1.ContainerInfoRequest, replyObj interface{}, t *testing.T) (*Client, *httptest.Server, error) {

client/v2/client_test.go

@@ -18,8 +18,9 @@ import (
 	"flag"
 	"testing"
 
-	"github.com/google/cadvisor/container"
 	"github.com/stretchr/testify/assert"
+
+	"github.com/google/cadvisor/container"
 )
 
 func TestTcpMetricsAreDisabledByDefault(t *testing.T) {

cmd/cadvisor_test.go

@@ -16,6 +16,9 @@ package mesos
 
 import (
 	"fmt"
+	"net/url"
+	"sync"
+
 	"github.com/Rican7/retry"
 	"github.com/Rican7/retry/strategy"
 	mesos "github.com/mesos/mesos-go/api/v1/lib"
@@ -24,8 +27,6 @@ import (
 	mclient "github.com/mesos/mesos-go/api/v1/lib/client"
 	"github.com/mesos/mesos-go/api/v1/lib/encoding/codecs"
 	"github.com/mesos/mesos-go/api/v1/lib/httpcli"
-	"net/url"
-	"sync"
 )
 
 const (

cmd/internal/container/mesos/client.go

@@ -22,12 +22,13 @@ import (
 	"strings"
 	"time"
 
+	"k8s.io/klog/v2"
+
 	"github.com/google/cadvisor/container"
 	"github.com/google/cadvisor/container/libcontainer"
 	"github.com/google/cadvisor/fs"
 	info "github.com/google/cadvisor/info/v1"
 	"github.com/google/cadvisor/watcher"
-	"k8s.io/klog/v2"
 )
 
 var MesosAgentAddress = flag.String("mesos_agent", "127.0.0.1:5051", "Mesos agent address")

cmd/internal/container/mesos/factory.go

@@ -15,10 +15,12 @@
 package mesos
 
 import (
-	containerlibcontainer "github.com/google/cadvisor/container/libcontainer"
+	"testing"
+
 	mesos "github.com/mesos/mesos-go/api/v1/lib"
 	"github.com/stretchr/testify/assert"
-	"testing"
+
+	containerlibcontainer "github.com/google/cadvisor/container/libcontainer"
 )
 
 func TestIsContainerName(t *testing.T) {

cmd/internal/container/mesos/factory_test.go

@@ -18,12 +18,13 @@ import (
 	"fmt"
 	"testing"
 
+	mesos "github.com/mesos/mesos-go/api/v1/lib"
+	"github.com/stretchr/testify/assert"
+
 	"github.com/google/cadvisor/container"
 	containerlibcontainer "github.com/google/cadvisor/container/libcontainer"
 	"github.com/google/cadvisor/fs"
 	info "github.com/google/cadvisor/info/v1"
-	mesos "github.com/mesos/mesos-go/api/v1/lib"
-	"github.com/stretchr/testify/assert"
 )
 
 func PopulateContainer() *mContainer {

cmd/internal/container/mesos/handler_test.go

@@ -16,9 +16,10 @@
 package install
 
 import (
+	"k8s.io/klog/v2"
+
 	"github.com/google/cadvisor/cmd/internal/container/mesos"
 	"github.com/google/cadvisor/container"
-	"k8s.io/klog/v2"
 )
 
 func init() {

cmd/internal/container/mesos/install/install.go

@@ -16,6 +16,7 @@ package mesos
 
 import (
 	"fmt"
+
 	mesos "github.com/mesos/mesos-go/api/v1/lib"
 	"github.com/mesos/mesos-go/api/v1/lib/agent"
 )

cmd/internal/container/mesos/mesos_agent.go

@@ -16,10 +16,11 @@ package mesos
 
 import (
 	"fmt"
+	"testing"
+
 	mesos "github.com/mesos/mesos-go/api/v1/lib"
 	"github.com/mesos/mesos-go/api/v1/lib/agent"
 	"github.com/stretchr/testify/assert"
-	"testing"
 )
 
 func PopulateFrameworks(fwID string) *agent.Response_GetFrameworks {

cmd/internal/container/mesos/mesos_agent_test.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/SeanDolphin/bqschema"
+
 	"github.com/google/cadvisor/cmd/internal/storage/bigquery/client"
 )
 

cmd/internal/storage/bigquery/client/example/example.go

@@ -19,7 +19,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/google/cadvisor/info/v1"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 const metricLabelPrefix = "io.cadvisor.metric."

collector/collector_manager.go

@@ -18,9 +18,9 @@ import (
 	"testing"
 	"time"
 
-	"github.com/google/cadvisor/info/v1"
-
 	"github.com/stretchr/testify/assert"
+
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 type fakeCollector struct {

collector/collector_manager_test.go

@@ -18,7 +18,8 @@ import (
 	"time"
 
 	"encoding/json"
-	"github.com/google/cadvisor/info/v1"
+
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 type Config struct {

collector/config.go

@@ -17,7 +17,7 @@ package collector
 import (
 	"time"
 
-	"github.com/google/cadvisor/info/v1"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 type FakeCollectorManager struct {

collector/fakes.go

@@ -25,7 +25,7 @@ import (
 	"time"
 
 	"github.com/google/cadvisor/container"
-	"github.com/google/cadvisor/info/v1"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 type GenericCollector struct {

collector/generic_collector.go

@@ -22,10 +22,10 @@ import (
 	"os"
 	"testing"
 
-	"github.com/google/cadvisor/info/v1"
+	"github.com/stretchr/testify/assert"
 
 	containertest "github.com/google/cadvisor/container/testing"
-	"github.com/stretchr/testify/assert"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 func TestEmptyConfig(t *testing.T) {

collector/generic_collector_test.go

@@ -28,7 +28,7 @@ import (
 	"github.com/prometheus/common/model"
 
 	"github.com/google/cadvisor/container"
-	"github.com/google/cadvisor/info/v1"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 type PrometheusCollector struct {

collector/prometheus_collector.go

@@ -21,11 +21,11 @@ import (
 	"net/http/httptest"
 	"testing"
 
-	"github.com/google/cadvisor/info/v1"
-
-	containertest "github.com/google/cadvisor/container/testing"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	containertest "github.com/google/cadvisor/container/testing"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 func TestPrometheus(t *testing.T) {

collector/prometheus_collector_test.go

@@ -17,7 +17,7 @@ package collector
 import (
 	"time"
 
-	"github.com/google/cadvisor/info/v1"
+	v1 "github.com/google/cadvisor/info/v1"
 )
 
 // TODO(vmarmol): Export to a custom metrics type when that is available.

collector/types.go

@@ -24,14 +24,15 @@ import (
 	"strings"
 	"time"
 
-	"github.com/google/cadvisor/container"
-	info "github.com/google/cadvisor/info/v1"
-	"github.com/google/cadvisor/utils"
 	"github.com/karrick/godirwalk"
 	"github.com/opencontainers/runc/libcontainer/cgroups"
 	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 
+	"github.com/google/cadvisor/container"
+	info "github.com/google/cadvisor/info/v1"
+	"github.com/google/cadvisor/utils"
+
 	"k8s.io/klog/v2"
 )