Carefully fixing style (google#2509)

* Use golangci-lint to add lint presubmit test, and fix linter errors

Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@intel.com>

.github/workflows/test.yml

@@ -1,6 +1,35 @@
 name: Test
 on: [push, pull_request]
 jobs:
+  lint:
+    strategy:
+      matrix:
+        go-versions: [1.14]
+        platform: [ubuntu-latest]
+        environment-variables: [build/config/plain.sh, build/config/libpfm4.sh]
+    runs-on: ${{ matrix.platform }}
+    timeout-minutes: 10
+    steps:
+    - name: Install Go
+      uses: actions/setup-go@v1
+      with:
+        go-version: ${{ matrix.go-versions }}
+#    This is not the most elegant way of installing golangci-lint but I have filed a PR
+#    and I will fix it as soon as it's merged: https://github.com/golangci/golangci-lint/pull/1036
+#    cAdvisor has existed for a while without golint and we need to figure out how to
+#    fix issues affecting exported identifiers that are part of public API. Let's avoid breaking Kubernetes.
+    - name: Install golangci-lint
+      run: |
+        go get -d github.com/golangci/golangci-lint/cmd/golangci-lint &&
+        cd $(go env GOPATH)/src/github.com/golangci/golangci-lint/ &&
+        git remote add iwankgb https://github.com/iwankgb/golangci-lint.git &&
+        git fetch iwankgb &&
+        git reset --hard iwankgb/exclude-case-sensitive &&
+        go install -i github.com/golangci/golangci-lint/cmd/golangci-lint
+    - name: Checkout code
+      uses: actions/checkout@v2
+    - name: Run golangci-lint
+      run: source ${{ matrix.environment-variables }} && make lint
   test:
     strategy:
       matrix:
@@ -31,19 +60,19 @@ jobs:
     runs-on: ${{ matrix.platform }}
     timeout-minutes: 30
     steps:
-      - name: Checkout code
-        uses: actions/checkout@v2
-        with:
-          fetch-depth: 1
-          path: go/src/github.com/google/cadvisor
-      - name: Run integration tests
-        env:
-          GOLANG_VERSION: ${{ matrix.go-versions }}
-        run: |
-          cd $GITHUB_WORKSPACE/go/src/github.com/google/cadvisor && source ${{ matrix.environment-variables }} && make docker-test-integration
-      - name: Upload cAdvisor log file
-        uses: actions/upload-artifact@v1
-        if: failure()
-        with:
-          name: cadvisor.log
-          path: ${{ github.workspace }}/go/src/github.com/google/cadvisor/cadvisor.log
+    - name: Checkout code
+      uses: actions/checkout@v2
+      with:
+        fetch-depth: 1
+        path: go/src/github.com/google/cadvisor
+    - name: Run integration tests
+      env:
+        GOLANG_VERSION: ${{ matrix.go-versions }}
+      run: |
+        cd $GITHUB_WORKSPACE/go/src/github.com/google/cadvisor && source ${{ matrix.environment-variables }} && make docker-test-integration
+    - name: Upload cAdvisor log file
+      uses: actions/upload-artifact@v1
+      if: failure()
+      with:
+        name: cadvisor.log
+        path: ${{ github.workspace }}/go/src/github.com/google/cadvisor/cadvisor.log

.golangci.yml

@@ -0,0 +1,60 @@
+run:
+  linters-settings:
+    govet:
+      enable-all: true
+    golint:
+      min-confidence: 0
+linters:
+  disable-all: true
+  enable:
+    - govet
+    - errcheck
+    - staticcheck
+    - unused
+    - gosimple
+    - structcheck
+    - varcheck
+    - ineffassign
+    - deadcode
+    - typecheck
+    - golint
+issues:
+  max-issues-per-linter: 0
+  max-same-issues: 0
+  exclude-case-sensitive: true
+  exclude:
+#   integration/tests/api/event_test.go:66:6: func `waitForStaticEvent` is unused (unused)
+#   Flaky test skipped.
+    - waitForStaticEvent
+#   Initialism or acronyms for fields, vars and types:
+    - "(struct field|var|type|const) `[A-Z].*` should be `.*`"
+#   Initialism or acronyms - renaming exported methods and functions can be tricky:
+    - "(method|func) [A-Z].* should be .*"
+#   Stuttering affects exported names:
+    - "type name will be used as .*\\.[A-Z]{1}.* by other packages, and that stutters"
+#   We would have to change exported function return type:
+    - "exported func .* returns unexported type"
+  exclude-rules:
+#   container/containerd/client.go:67:4: SA1019: grpc.WithDialer is deprecated: use WithContextDialer instead.  Will be supported throughout 1.x.  (staticcheck)
+#   There are more similar issues in following lines.
+    - path: container/containerd/client.go
+      text: "SA1019:"
+#   utils/cpuload/netlink/netlink.go:102:15: Error return value of `binary.Write` is not checked (errcheck)
+#   There are more similar issues in this file
+    - path: utils/cpuload/netlink/netlink.go
+      text: "Error return value of `binary.Write` is not checked"
+#   utils/cloudinfo/aws/aws.go:60:28: SA1019: session.New is deprecated: Use NewSession functions to create sessions instead. NewSession has the same functionality as New except an error can be returned when the func is called instead of waiting to receive an error until a request is made.  (staticcheck)
+    - path: utils/cloudinfo/aws/aws.go
+      text: "SA1019:"
+#   events/handler.go:151:51: exported func NewEventManager returns unexported type *github.com/google/cadvisor/events.events, which can be annoying to use (golint)
+    - path: events/handler.go
+      text: "exported func NewEventManager returns unexported type .{1}github.com/google/cadvisor/events.events, which can be annoying to use"
+#   manager/manager_test.go:277:29: Error return value of `(*github.com/google/cadvisor/container/testing.MockContainerHandler).GetSpec` is not checked (errcheck)
+    - path: manager/manager_test.go
+      text: "Error return value of `.{2}github.com/google/cadvisor/container/testing.MockContainerHandler.{1}.GetSpec` is not checked"
+#   utils/sysinfo/sysinfo.go:208:7: ineffectual assignment to `err` (ineffassign)
+    - path: utils/sysinfo/sysinfo.go
+      text: "ineffectual assignment to `err`|SA4006:"
+#   cache/memory/memory_test.go:81:23: Error return value of `memoryCache.AddStats` is not checked (errcheck)
+    - path: cache/memory/memory_test.go
+      text: "Error return value of `memoryCache.AddStats` is not checked"

Makefile

@@ -16,6 +16,7 @@ GO := go
 pkgs     = $(shell $(GO) list ./... | grep -v vendor)
 cmd_pkgs = $(shell cd cmd && $(GO) list ./... | grep -v vendor)
 arch ?= $(shell go env GOARCH)
+go_path = $(shell go env GOPATH)
 
 ifeq ($(arch), amd64)
   Dockerfile_tag := ''
@@ -86,6 +87,10 @@ presubmit: vet
 	@echo ">> checking file boilerplate"
 	@./build/check_boilerplate.sh
 
+lint:
+	@echo ">> running golangci-lint using configuration at .golangci.yml"
+	@$(go_path)/bin/golangci-lint run
+
 clean:
 	@rm -f *.test cadvisor
 

accelerators/nvidia.go

@@ -46,7 +46,7 @@ type nvidiaManager struct {
 
 var sysFsPCIDevicesPath = "/sys/bus/pci/devices/"
 
-const nvidiaVendorId = "0x10de"
+const nvidiaVendorID = "0x10de"
 
 func NewNvidiaManager() stats.Manager {
 	manager := &nvidiaManager{}
@@ -61,8 +61,8 @@ func NewNvidiaManager() stats.Manager {
 
 // setup initializes NVML if nvidia devices are present on the node.
 func (nm *nvidiaManager) setup() error {
-	if !detectDevices(nvidiaVendorId) {
-		return fmt.Errorf("No NVIDIA devices found.")
+	if !detectDevices(nvidiaVendorID) {
+		return fmt.Errorf("no NVIDIA devices found")
 	}
 
 	nm.devicesPresent = true
@@ -71,7 +71,7 @@ func (nm *nvidiaManager) setup() error {
 }
 
 // detectDevices returns true if a device with given pci id is present on the node.
-func detectDevices(vendorId string) bool {
+func detectDevices(vendorID string) bool {
 	devices, err := ioutil.ReadDir(sysFsPCIDevicesPath)
 	if err != nil {
 		klog.Warningf("Error reading %q: %v", sysFsPCIDevicesPath, err)
@@ -85,8 +85,8 @@ func detectDevices(vendorId string) bool {
 			klog.V(4).Infof("Error while reading %q: %v", vendorPath, err)
 			continue
 		}
-		if strings.EqualFold(strings.TrimSpace(string(content)), vendorId) {
-			klog.V(3).Infof("Found device with vendorId %q", vendorId)
+		if strings.EqualFold(strings.TrimSpace(string(content)), vendorID) {
+			klog.V(3).Infof("Found device with vendorID %q", vendorID)
 			return true
 		}
 	}

cache/memory/memory.go

@@ -38,19 +38,19 @@ type containerCache struct {
 	lock        sync.RWMutex
 }
 
-func (self *containerCache) AddStats(stats *info.ContainerStats) error {
-	self.lock.Lock()
-	defer self.lock.Unlock()
+func (c *containerCache) AddStats(stats *info.ContainerStats) error {
+	c.lock.Lock()
+	defer c.lock.Unlock()
 
 	// Add the stat to storage.
-	self.recentStats.Add(stats.Timestamp, stats)
+	c.recentStats.Add(stats.Timestamp, stats)
 	return nil
 }
 
-func (self *containerCache) RecentStats(start, end time.Time, maxStats int) ([]*info.ContainerStats, error) {
-	self.lock.RLock()
-	defer self.lock.RUnlock()
-	result := self.recentStats.InTimeRange(start, end, maxStats)
+func (c *containerCache) RecentStats(start, end time.Time, maxStats int) ([]*info.ContainerStats, error) {
+	c.lock.RLock()
+	defer c.lock.RUnlock()
+	result := c.recentStats.InTimeRange(start, end, maxStats)
 	converted := make([]*info.ContainerStats, len(result))
 	for i, el := range result {
 		converted[i] = el.(*info.ContainerStats)
@@ -73,20 +73,20 @@ type InMemoryCache struct {
 	backend           []storage.StorageDriver
 }
 
-func (self *InMemoryCache) AddStats(cInfo *info.ContainerInfo, stats *info.ContainerStats) error {
+func (c *InMemoryCache) AddStats(cInfo *info.ContainerInfo, stats *info.ContainerStats) error {
 	var cstore *containerCache
 	var ok bool
 
 	func() {
-		self.lock.Lock()
-		defer self.lock.Unlock()
-		if cstore, ok = self.containerCacheMap[cInfo.ContainerReference.Name]; !ok {
-			cstore = newContainerStore(cInfo.ContainerReference, self.maxAge)
-			self.containerCacheMap[cInfo.ContainerReference.Name] = cstore
+		c.lock.Lock()
+		defer c.lock.Unlock()
+		if cstore, ok = c.containerCacheMap[cInfo.ContainerReference.Name]; !ok {
+			cstore = newContainerStore(cInfo.ContainerReference, c.maxAge)
+			c.containerCacheMap[cInfo.ContainerReference.Name] = cstore
 		}
 	}()
 
-	for _, backend := range self.backend {
+	for _, backend := range c.backend {
 		// TODO(monnand): To deal with long delay write operations, we
 		// may want to start a pool of goroutines to do write
 		// operations.
@@ -97,13 +97,13 @@ func (self *InMemoryCache) AddStats(cInfo *info.ContainerInfo, stats *info.Conta
 	return cstore.AddStats(stats)
 }
 
-func (self *InMemoryCache) RecentStats(name string, start, end time.Time, maxStats int) ([]*info.ContainerStats, error) {
+func (c *InMemoryCache) RecentStats(name string, start, end time.Time, maxStats int) ([]*info.ContainerStats, error) {
 	var cstore *containerCache
 	var ok bool
 	err := func() error {
-		self.lock.RLock()
-		defer self.lock.RUnlock()
-		if cstore, ok = self.containerCacheMap[name]; !ok {
+		c.lock.RLock()
+		defer c.lock.RUnlock()
+		if cstore, ok = c.containerCacheMap[name]; !ok {
 			return ErrDataNotFound
 		}
 		return nil
@@ -115,17 +115,17 @@ func (self *InMemoryCache) RecentStats(name string, start, end time.Time, maxSta
 	return cstore.RecentStats(start, end, maxStats)
 }
 
-func (self *InMemoryCache) Close() error {
-	self.lock.Lock()
-	self.containerCacheMap = make(map[string]*containerCache, 32)
-	self.lock.Unlock()
+func (c *InMemoryCache) Close() error {
+	c.lock.Lock()
+	c.containerCacheMap = make(map[string]*containerCache, 32)
+	c.lock.Unlock()
 	return nil
 }
 
-func (self *InMemoryCache) RemoveContainer(containerName string) error {
-	self.lock.Lock()
-	delete(self.containerCacheMap, containerName)
-	self.lock.Unlock()
+func (c *InMemoryCache) RemoveContainer(containerName string) error {
+	c.lock.Lock()
+	delete(c.containerCacheMap, containerName)
+	c.lock.Unlock()
 	return nil
 }