Run gometalinter (#179)

Includes fixes for linter issues reported.

Makefile

@@ -26,17 +26,14 @@ build:
 	go fmt $$(go list ./... | grep -v /vendor/)
 	CGO_ENABLED=$(CGO_ENABLE) go build
 
-rebuild: prechecks build test
+rebuild: prechecks build test lint
 
 check: test
 	# tests should fail if go fmt results in uncommitted code
 	git status --porcelain
 	/bin/bash -c 'test $$(git status --porcelain | wc -l) == 0'
 test:
-	go get github.com/golang/lint/golint
-	go test -v -race $$(go list ./... | grep -v /vendor/)
-	go vet $$(go list ./... | grep -v /vendor/)
-	go list ./... | grep -v /vendor/ | xargs -n1 golint
+	go test -tags=system -v -race $$(go list ./... | grep -v /vendor/)
 
 dev-test:
 	go test -race $$(go list ./... | grep -v /vendor/)
@@ -47,3 +44,9 @@ tc-worker-env:
 tc-worker:
 	CGO_ENABLED=$(CGO_ENABLE) GOARCH=amd64 go build
 	docker build -t taskcluster/tc-worker -f tc-worker.Dockerfile .
+
+lint:
+	go get github.com/alecthomas/gometalinter
+	gometalinter --install
+	# not enabled: aligncheck, deadcode, dupl, errcheck, gas, gocyclo, structcheck, unused, varcheck
+	$(GOPATH)/bin/gometalinter --deadline=10m --line-length=180 --vendor --vendored-linters --disable-all --enable=goconst --enable=gofmt --enable=goimports --enable=golint --enable=gosimple --enable=gotype --enable=ineffassign --enable=interfacer --enable=lll --enable=misspell --enable=staticcheck --enable=test --enable=testify --enable=unconvert --enable=vet --enable=vetshadow ./...

commands/qemu-build/buildimage.go

@@ -49,7 +49,7 @@ func buildImage(
 
 		// Construct MutableImage
 		monitor.Info("Creating MutableImage")
-		img, err2 = image.NewMutableImage(tempFolder, int(size), machine)
+		img, err2 = image.NewMutableImage(tempFolder, size, machine)
 		if err2 != nil {
 			monitor.Error("Failed to create image, error: ", err2)
 			return err2

commands/qemu-guest-tools/guesttools.go

@@ -12,9 +12,6 @@ import (
 	goruntime "runtime"
 	"time"
 
-	"gopkg.in/djherbis/buffer.v1"
-	"gopkg.in/djherbis/nio.v2"
-
 	"github.com/gorilla/websocket"
 	"github.com/taskcluster/go-got"
 	"github.com/taskcluster/taskcluster-worker/engines/qemu/metaservice"
@@ -24,6 +21,8 @@ import (
 	"github.com/taskcluster/taskcluster-worker/runtime/ioext"
 	"golang.org/x/net/context"
 	"golang.org/x/net/context/ctxhttp"
+	buffer "gopkg.in/djherbis/buffer.v1"
+	nio "gopkg.in/djherbis/nio.v2"
 )
 
 type guestTools struct {

config/load.go

@@ -113,8 +113,7 @@ func LoadFromFile(filename string) (interface{}, error) {
 	// Read config file
 	configFile, err := ioutil.ReadFile(filename)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to read config file: '%s' error: %s\n",
-			filename, err)
+		return nil, fmt.Errorf("failed to read config file '%s': %s", filename, err)
 	}
 
 	return Load(configFile)

engines/enginetest/artifacts.go

@@ -75,7 +75,7 @@ func (c *ArtifactTestCase) TestExtractFolderNotFound() {
 	err := r.resultSet.ExtractFolder(c.FolderNotFoundPath, func(
 		path string, reader ioext.ReadSeekCloser,
 	) error {
-		return errors.New("File was found, didn't expect that!!!")
+		return errors.New("file was found, didn't expect that")
 	})
 	assert(err == engines.ErrResourceNotFound, "Expected ErrResourceNotFound ",
 		"but got :", err)

engines/enginetest/testutils.go

@@ -232,8 +232,8 @@ func (r *run) OpenLogReader() {
 
 func (r *run) ReadLog() string {
 	reader, err := r.context.NewLogReader()
-	defer reader.Close()
 	nilOrPanic(err, "Failed to open log reader")
+	defer reader.Close()
 	data, err := ioutil.ReadAll(reader)
 	nilOrPanic(err, "Failed to read log")
 	return string(data)

engines/mock/mocknet/errors.go

@@ -2,11 +2,11 @@ package mocknet
 
 import "errors"
 
-// ErrListenerClosed is returned when a listener have been closed
-var ErrListenerClosed = errors.New("Listener have been closed!")
+// ErrListenerClosed is returned when a listener has been closed
+var ErrListenerClosed = errors.New("listener has been closed")
 
 // ErrAddressInUse is returned if the address already is in use
-var ErrAddressInUse = errors.New("Address is already in use by another listener")
+var ErrAddressInUse = errors.New("address is already in use by another listener")
 
 // ErrConnRefused is returned if the connection is refused
-var ErrConnRefused = errors.New("Connection refused, no listener for the given address")
+var ErrConnRefused = errors.New("connection refused, no listener for the given address")

engines/osxnative/engine.go

@@ -5,15 +5,15 @@ package osxnative
 import (
 	"sync"
 
-	"github.com/Sirupsen/logrus"
 	schematypes "github.com/taskcluster/go-schematypes"
 	"github.com/taskcluster/taskcluster-worker/engines"
+	"github.com/taskcluster/taskcluster-worker/runtime"
 )
 
 type engine struct {
 	engines.EngineBase
-	config *configType
-	log    *logrus.Entry
+	config  *configType
+	monitor runtime.Monitor
 }
 
 type engineProvider struct {
@@ -23,13 +23,13 @@ type engineProvider struct {
 func (e engineProvider) NewEngine(options engines.EngineOptions) (engines.Engine, error) {
 	var c configType
 	if err := schematypes.MustMap(configSchema, options.Config, &c); err != nil {
-		options.Log.WithError(err).Error("Invalid configuration")
+		options.Monitor.ReportError(err, "Invalid configuration")
 		return nil, engines.ErrContractViolation
 	}
 
 	return &engine{
-		config: &c,
-		log:    options.Log,
+		config:  &c,
+		monitor: options.Monitor,
 	}, nil
 }
 

engines/osxnative/resultset.go

@@ -14,6 +14,7 @@ import (
 )
 
 type resultset struct {
+	monitor runtime.Monitor
 	engines.ResultSetBase
 	taskUser user
 	context  *runtime.TaskContext
@@ -110,10 +111,10 @@ func (r resultset) ExtractFolder(path string, handler engines.FileHandler) error
 func (r resultset) Dispose() error {
 	err := r.taskUser.delete()
 	if err != nil {
-		r.engine.log.WithField("user", r.taskUser.name).WithError(err).Error("Error removing user")
+		r.monitor.Error("Error removing user: ", err)
 		exitError, ok := err.(*exec.ExitError)
 		if ok {
-			r.engine.log.Error(string(exitError.Stderr))
+			r.monitor.Error(string(exitError.Stderr))
 		}
 
 		return engines.ErrNonFatalInternalError

engines/osxnative/resultset_test.go

@@ -10,11 +10,11 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/Sirupsen/logrus"
 	assert "github.com/stretchr/testify/require"
 	"github.com/taskcluster/taskcluster-worker/engines"
 	"github.com/taskcluster/taskcluster-worker/runtime"
 	"github.com/taskcluster/taskcluster-worker/runtime/ioext"
+	"github.com/taskcluster/taskcluster-worker/runtime/mocks"
 )
 
 type testCase struct {
@@ -38,8 +38,8 @@ func makeResultSet(t *testing.T) resultset {
 	}
 
 	e := engine{
-		config: &config,
-		log:    logrus.New().WithField("component", "test"),
+		config:  &config,
+		monitor: mocks.NewMockMonitor(true),
 	}
 
 	return resultset{

engines/osxnative/sandbox.go

@@ -44,6 +44,7 @@ type sandbox struct {
 	env         []string
 	aborted     bool
 	engine      *engine
+	monitor     runtime.Monitor
 }
 
 func newSandbox(context *runtime.TaskContext, taskPayload *payloadType, env []string, engine *engine) *sandbox {
@@ -53,6 +54,7 @@ func newSandbox(context *runtime.TaskContext, taskPayload *payloadType, env []st
 		env:         env,
 		aborted:     false,
 		engine:      engine,
+		monitor:     engine.monitor,
 	}
 }
 
@@ -93,7 +95,6 @@ func downloadLink(destdir string, link string) (string, error) {
 }
 
 func (s *sandbox) WaitForResult() (engines.ResultSet, error) {
-	log := s.engine.log
 
 	if s.aborted {
 		return nil, engines.ErrSandboxAborted
@@ -118,18 +119,18 @@ func (s *sandbox) WaitForResult() (engines.ResultSet, error) {
 
 	userInfo, err := osuser.Current()
 	if err != nil {
-		log.WithError(err).Error("Error getting user information: ")
+		s.monitor.Error("Error getting user information: ", err)
 		return nil, engines.NewInternalError(err.Error())
 	}
 
 	u := newUser(s.engine.config.Sudo)
 
 	if s.engine.config.CreateUser {
 		if err = u.create(s.engine.config.UserGroups); err != nil {
-			log.WithError(err).Error("Could not create user")
+			s.monitor.Error("Could not create user", err)
 			exitError, ok := err.(*exec.ExitError)
 			if ok {
-				log.Error(string(exitError.Stderr))
+				s.monitor.Error(string(exitError.Stderr))
 			}
 
 			return nil, engines.NewInternalError("Could not create temporary user")
@@ -143,21 +144,21 @@ func (s *sandbox) WaitForResult() (engines.ResultSet, error) {
 
 		userInfo, err = osuser.Lookup(u.name)
 		if err != nil {
-			log.WithError(err).Error("Error looking up for user \"" + u.name + "\"")
+			s.monitor.Error("Error looking up for user \""+u.name+"\"", err)
 			return nil, engines.NewInternalError(err.Error())
 		}
 
 		var uid uint64
 		uid, err = strconv.ParseUint(userInfo.Uid, 10, 32)
 		if err != nil {
-			log.WithError(err).Error("ParseUint failed to convert ", userInfo.Uid)
+			s.monitor.Error("ParseUint failed to convert ", userInfo.Uid, err)
 			return nil, engines.ErrNonFatalInternalError
 		}
 
 		var gid uint64
 		gid, err = strconv.ParseUint(userInfo.Gid, 10, 32)
 		if err != nil {
-			log.WithError(err).Error("ParseUint failed to convert ", userInfo.Gid)
+			s.monitor.Error("ParseUint failed to convert ", userInfo.Gid, err)
 			return nil, engines.ErrNonFatalInternalError
 		}
 
@@ -195,7 +196,7 @@ func (s *sandbox) WaitForResult() (engines.ResultSet, error) {
 		defer os.Remove(filename)
 
 		if err = os.Chmod(filename, 0777); err != nil {
-			log.WithError(err).Error("Could not set permissions in the file")
+			s.monitor.Error("Could not set permissions in the file", err)
 			return nil, engines.ErrNonFatalInternalError
 		}
 	}

engines/osxnative/sandbox_test.go

@@ -10,10 +10,10 @@ import (
 	"os"
 	"testing"
 
-	"github.com/Sirupsen/logrus"
 	assert "github.com/stretchr/testify/require"
 	"github.com/taskcluster/taskcluster-worker/engines"
 	"github.com/taskcluster/taskcluster-worker/runtime"
+	"github.com/taskcluster/taskcluster-worker/runtime/mocks"
 )
 
 // Simple HTTP server for tests
@@ -79,7 +79,7 @@ func newTestSandbox(taskPayload *payloadType, env []string) (*sandbox, error) {
 	e := engine{
 		EngineBase: engines.EngineBase{},
 		config:     &config,
-		log:        logrus.New().WithField("component", "test"),
+		monitor:    mocks.NewMockMonitor(true),
 	}
 
 	return newSandbox(context, taskPayload, env, &e), nil

engines/qemu/image/mutableimage.go

@@ -105,7 +105,7 @@ func (img *MutableImage) Machine() vm.Machine {
 	return *img.machine
 }
 
-// Package will write an zstd compressed tar archieve of the image to targetFile.
+// Package will write an zstd compressed tar archive of the image to targetFile.
 // This method cannot be called the image is in-use.
 func (img *MutableImage) Package(targetFile string) error {
 	img.m.Lock()

engines/qemu/image/util.go

@@ -10,35 +10,36 @@ import (
 	"github.com/taskcluster/taskcluster-worker/engines"
 )
 
-// copyFile copies source to destination.
-func copyFile(source, target string) error {
-	// Open input file
-	input, err := os.Open(source)
+// copyFile copies source to destination, and returns an error if one occurs
+// during opening input or output file, copying data from input to output, or
+// closing input or output file
+func copyFile(source, target string) (err error) {
+	var input *os.File
+	var output *os.File
+	input, err = os.Open(source)
 	if err != nil {
-		return err
+		return
 	}
-	defer input.Close()
 
-	// Create target file
-	output, err := os.Create(target)
-	if err != nil {
-		return err
+	closeFile := func(f *os.File) {
+		err2 := f.Close()
+		if err == nil {
+			err = err2
+		}
 	}
 
-	// Copy data
-	_, err = io.Copy(output, input)
-	if err != nil {
-		output.Close()
-		return err
-	}
+	defer closeFile(input)
 
-	// Close output file
-	err = output.Close()
+	// Create target file
+	output, err = os.Create(target)
 	if err != nil {
-		return err
+		return
 	}
+	defer closeFile(output)
 
-	return nil
+	// Copy data
+	_, err = io.Copy(output, input)
+	return
 }
 
 const maxRetries = 7

engines/qemu/metaservice/metaservice_test.go

@@ -283,9 +283,9 @@ func TestMetaServiceShell(t *testing.T) {
 		messageType, m, err2 := ws.ReadMessage()
 		nilOrFatal(t, err2, "ReadMessage failed")
 		assert(t, messageType == websocket.BinaryMessage, "expected BinaryMessage")
-		assert(t, bytes.Compare(m, []byte{
+		assert(t, bytes.Equal(m, []byte{
 			shellconsts.MessageTypeData, shellconsts.StreamStdin, 'h', 'i',
-		}) == 0, "expected 'hi' on stdin")
+		}), "expected 'hi' on stdin")
 
 		debug("guest-tool: Ack: 'hi' from stdin")
 		err2 = ws.WriteMessage(websocket.BinaryMessage, []byte{
@@ -303,9 +303,9 @@ func TestMetaServiceShell(t *testing.T) {
 		messageType, m, err2 = ws.ReadMessage()
 		nilOrFatal(t, err2, "Failed to ReadMessage")
 		assert(t, messageType == websocket.BinaryMessage, "expected BinaryMessage")
-		assert(t, bytes.Compare(m, []byte{
+		assert(t, bytes.Equal(m, []byte{
 			shellconsts.MessageTypeAck, shellconsts.StreamStdout, 0, 0, 0, 5,
-		}) == 0, "expected ack for 5 on stdout")
+		}), "expected ack for 5 on stdout")
 
 		debug("guest-tool: Send: close on stdout")
 		err2 = ws.WriteMessage(websocket.BinaryMessage, []byte{
@@ -317,9 +317,9 @@ func TestMetaServiceShell(t *testing.T) {
 		messageType, m, err2 = ws.ReadMessage()
 		nilOrFatal(t, err2, "Failed to ReadMessage")
 		assert(t, messageType == websocket.BinaryMessage, "expected BinaryMessage")
-		assert(t, bytes.Compare(m, []byte{
+		assert(t, bytes.Equal(m, []byte{
 			shellconsts.MessageTypeData, shellconsts.StreamStdin,
-		}) == 0, "expected stdin to be closed")
+		}), "expected stdin to be closed")
 
 		debug("guest-tool: Send: exit success")
 		err2 = ws.WriteMessage(websocket.BinaryMessage, []byte{

engines/qemu/network/pool.go

@@ -20,7 +20,7 @@ import (
 
 const metaDataIP = "169.254.169.254"
 
-var remoteAddrPattern = regexp.MustCompile("^(192\\.168\\.\\d{1,3})\\.\\d{1,3}:\\d{1,5}$")
+var remoteAddrPattern = regexp.MustCompile(`^(192\.168\.\d{1,3})\.\d{1,3}:\d{1,5}$`)
 
 // Pool manages a static set of networks (TAP devices).
 type Pool struct {