Add gometalint. Fix errors (runatlantis#176)

.circleci/config.yml

@@ -13,8 +13,7 @@ jobs:
       - run: make deps
       - run: make test-coverage
       - run: make check-fmt
-      - run: make check-govet
-      - run: make megacheck
+      - run: make check-gometalint
       - run:
           name: post coverage to codecov.io
           command: bash <(curl -s https://codecov.io/bash)

CONTRIBUTING.md

@@ -70,7 +70,7 @@ This is easier to read and more consistent
 ### Testing
 - place tests under `{package under test}_test` to enforce testing the external interfaces
 - if you need to test internally i.e. access non-exported stuff, call the file `{file under test}_internal_test.go`
-- use `testing_util` for easier-to-read assertions: `import . "github.com/hootsuite/atlantis/testing_util"`
+- use our testing utility for easier-to-read assertions: `import . "github.com/hootsuite/atlantis/testing"` and then use `Assert()`, `Equals()` and `Ok()`
 - don't try to describe the whole test by its function name. Instead use `t.Log` statements:
 ```go
 // don't do this

Makefile

@@ -47,17 +47,22 @@ release: ## Create packages for a release
 fmt: ## Run goimports (which also formats)
 	goimports -w $$(find . -type f -name '*.go' ! -path "./vendor/*" ! -path "./server/static/bindata_assetfs.go" ! -path "**/mocks/*")
 
+gometalint: ## Run every linter ever
+	# gotype and gotypex are disabled because they don't pass on CI and https://github.com/alecthomas/gometalinter/issues/206
+	# gocyclo is temporarily disabled because we don't pass it right now
+	# golint is temporarily disabled because we need to add comments everywhere first
+	gometalinter --disable gotype --disable gotypex --disable=gocyclo --disable golint --enable=megacheck --enable=unparam --deadline=120s --vendor -t --line-length=120 $$(find . -type f -name '*.go' ! -path "./vendor/*" ! -path "./server/static/bindata_assetfs.go" ! -path "**/mocks/*" | xargs -I '{}' dirname '{}' | sort -u)
+
+gometalint-install: ## Install gometalint
+	go get -u github.com/alecthomas/gometalinter
+	gometalinter --install
+
+check-gometalint: gometalint-install gometalint
+
 check-fmt: ## Fail if not formatted
 	go get golang.org/x/tools/cmd/goimports
 	goimports -d $$(find . -type f -name '*.go' ! -path "./vendor/*" ! -path "./server/static/bindata_assetfs.go" ! -path "**/mocks/*")
 
-check-govet: ## Fail if go vet finds errors
-	go vet $$(go list ./... | grep -v vendor | grep -v static)
-
-megacheck: ## Fail if megacheck finds errors
-	go get honnef.co/go/tools/cmd/megacheck
-	megacheck $$(go list ./... | grep -v vendor | grep -v static)
-
 end-to-end-deps: ## Install e2e dependencies
 	./scripts/e2e-deps.sh
 

bootstrap/bootstrap.go

@@ -38,6 +38,8 @@ var pullRequestBody = "In this pull request we will learn how to use atlantis. T
 	"* Now lets apply that plan. Type `atlantis apply` in the comments. This will run a `terraform apply`.\n" +
 	"\nThank you for trying out atlantis. For more info on running atlantis in production see https://github.com/hootsuite/atlantis"
 
+// Start begins the bootstrap process
+// nolint: errcheck
 func Start() error {
 	s := spinner.New(spinner.CharSets[14], 100*time.Millisecond)
 	colorstring.Println(bootstrapDescription)
@@ -88,13 +90,14 @@ Follow these instructions to create a token (we don't store any tokens):
 		colorstring.Printf("[white]=> downloading terraform ")
 		s.Start()
 		terraformDownloadURL := fmt.Sprintf("%s/terraform/%s/terraform_%s_%s_%s.zip", hashicorpReleasesURL, terraformVersion, terraformVersion, runtime.GOOS, runtime.GOARCH)
-		if err := downloadAndUnzip(terraformDownloadURL, "/tmp/terraform.zip", "/tmp"); err != nil {
+		if err = downloadAndUnzip(terraformDownloadURL, "/tmp/terraform.zip", "/tmp"); err != nil {
 			return errors.Wrapf(err, "downloading and unzipping terraform")
 		}
 		colorstring.Println("\n[green]=> downloaded terraform successfully!")
 		s.Stop()
 
-		terraformCmd, err := executeCmd("mv", []string{"/tmp/terraform", "/usr/local/bin/"})
+		var terraformCmd *exec.Cmd
+		terraformCmd, err = executeCmd("mv", []string{"/tmp/terraform", "/usr/local/bin/"})
 		if err != nil {
 			return errors.Wrapf(err, "moving terraform binary into /usr/local/bin")
 		}
@@ -108,7 +111,7 @@ Follow these instructions to create a token (we don't store any tokens):
 	colorstring.Printf("[white]=> downloading ngrok  ")
 	s.Start()
 	ngrokURL := fmt.Sprintf("%s/ngrok-stable-%s-%s.zip", ngrokDownloadURL, runtime.GOOS, runtime.GOARCH)
-	if err := downloadAndUnzip(ngrokURL, "/tmp/ngrok.zip", "/tmp"); err != nil {
+	if err = downloadAndUnzip(ngrokURL, "/tmp/ngrok.zip", "/tmp"); err != nil {
 		return errors.Wrapf(err, "downloading and unzipping ngrok")
 	}
 	s.Stop()

bootstrap/utils.go

@@ -17,10 +17,10 @@ import (
 var hashicorpReleasesURL = "https://releases.hashicorp.com"
 var terraformVersion = "0.9.11"
 var ngrokDownloadURL = "https://bin.equinox.io/c/4VmDzA7iaHb"
-var ngrokApiURL = "http://localhost:4040"
+var ngrokAPIURL = "http://localhost:4040"
 
 func readPassword() (string, error) {
-	password, err := terminal.ReadPassword(int(syscall.Stdin))
+	password, err := terminal.ReadPassword(syscall.Stdin)
 	return string(password), err
 }
 
@@ -29,13 +29,13 @@ func downloadFile(url string, path string) error {
 	if err != nil {
 		return err
 	}
-	defer output.Close()
+	defer output.Close() // nolint: errcheck
 
 	response, err := http.Get(url)
 	if err != nil {
 		return err
 	}
-	defer response.Body.Close()
+	defer response.Body.Close() // nolint: errcheck
 
 	_, err = io.Copy(output, response.Body)
 	return err
@@ -50,21 +50,23 @@ func unzip(archive, target string) error {
 	for _, file := range reader.File {
 		path := filepath.Join(target, file.Name)
 		if file.FileInfo().IsDir() {
-			os.MkdirAll(path, file.Mode())
+			if err := os.MkdirAll(path, file.Mode()); err != nil {
+				return err
+			}
 			continue
 		}
 
 		fileReader, err := file.Open()
 		if err != nil {
 			return err
 		}
-		defer fileReader.Close()
+		defer fileReader.Close() // nolint: errcheck
 
 		targetFile, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode())
 		if err != nil {
 			return err
 		}
-		defer targetFile.Close()
+		defer targetFile.Close() // nolint: errcheck
 
 		if _, err := io.Copy(targetFile, fileReader); err != nil {
 			return err
@@ -75,11 +77,11 @@ func unzip(archive, target string) error {
 }
 
 func getTunnelAddr() (string, error) {
-	response, err := http.Get(fmt.Sprintf("%s/api/tunnels", ngrokApiURL))
+	response, err := http.Get(fmt.Sprintf("%s/api/tunnels", ngrokAPIURL))
 	if err != nil {
 		return "", err
 	}
-	defer response.Body.Close()
+	defer response.Body.Close() // nolint: errcheck
 
 	type tunnel struct {
 		Name      string `json:"name"`
@@ -106,6 +108,7 @@ func getTunnelAddr() (string, error) {
 	return t.Tunnels[1].PublicURL, nil
 }
 
+// nolint: unparam
 func downloadAndUnzip(url string, path string, target string) error {
 	if err := downloadFile(url, path); err != nil {
 		return err

cmd/server.go

@@ -15,7 +15,7 @@ import (
 
 // To add a new flag you must:
 // 1. Add a const with the flag name (in alphabetic order).
-// 2. Add a new field to server.ServerConfig and set the mapstructure tag equal to the flag name.
+// 2. Add a new field to server.Config and set the mapstructure tag equal to the flag name.
 // 3. Add your flag's description etc. to the stringFlags, intFlags, or boolFlags slices.
 const (
 	AtlantisURLFlag     = "atlantis-url"
@@ -114,7 +114,7 @@ type ServerCmd struct {
 // ServerCreator creates servers.
 // It's an abstraction to help us test.
 type ServerCreator interface {
-	NewServer(config server.ServerConfig) (ServerStarter, error)
+	NewServer(config server.Config) (ServerStarter, error)
 }
 
 // DefaultServerCreator is the concrete implementation of ServerCreator.
@@ -127,7 +127,7 @@ type ServerStarter interface {
 }
 
 // NewServer returns the real Atlantis server object.
-func (d *DefaultServerCreator) NewServer(config server.ServerConfig) (ServerStarter, error) {
+func (d *DefaultServerCreator) NewServer(config server.Config) (ServerStarter, error) {
 	return server.NewServer(config)
 }
 
@@ -160,21 +160,21 @@ Config file values are overridden by environment variables which in turn are ove
 	for _, f := range stringFlags {
 		c.Flags().String(f.name, f.value, f.description)
 		if f.env != "" {
-			s.Viper.BindEnv(f.name, f.env)
+			s.Viper.BindEnv(f.name, f.env) // nolint: errcheck
 		}
-		s.Viper.BindPFlag(f.name, c.Flags().Lookup(f.name))
+		s.Viper.BindPFlag(f.name, c.Flags().Lookup(f.name)) // nolint: errcheck
 	}
 
 	// Set int flags.
 	for _, f := range intFlags {
 		c.Flags().Int(f.name, f.value, f.description)
-		s.Viper.BindPFlag(f.name, c.Flags().Lookup(f.name))
+		s.Viper.BindPFlag(f.name, c.Flags().Lookup(f.name)) // nolint: errcheck
 	}
 
 	// Set bool flags.
 	for _, f := range boolFlags {
 		c.Flags().Bool(f.name, f.value, f.description)
-		s.Viper.BindPFlag(f.name, c.Flags().Lookup(f.name))
+		s.Viper.BindPFlag(f.name, c.Flags().Lookup(f.name)) // nolint: errcheck
 	}
 
 	return c
@@ -193,7 +193,7 @@ func (s *ServerCmd) preRun() error {
 }
 
 func (s *ServerCmd) run() error {
-	var config server.ServerConfig
+	var config server.Config
 	if err := s.Viper.Unmarshal(&config); err != nil {
 		return err
 	}
@@ -216,7 +216,7 @@ func (s *ServerCmd) run() error {
 	return server.Start()
 }
 
-func validate(config server.ServerConfig) error {
+func validate(config server.Config) error {
 	logLevel := config.LogLevel
 	if logLevel != "debug" && logLevel != "info" && logLevel != "warn" && logLevel != "error" {
 		return errors.New("invalid log level: not one of debug, info, warn, error")
@@ -231,7 +231,7 @@ func validate(config server.ServerConfig) error {
 }
 
 // setAtlantisURL sets the externally accessible URL for atlantis.
-func setAtlantisURL(config *server.ServerConfig) error {
+func setAtlantisURL(config *server.Config) error {
 	if config.AtlantisURL == "" {
 		hostname, err := os.Hostname()
 		if err != nil {
@@ -245,7 +245,7 @@ func setAtlantisURL(config *server.ServerConfig) error {
 // setDataDir checks if ~ was used in data-dir and converts it to the actual
 // home directory. If we don't do this, we'll create a directory called "~"
 // instead of actually using home.
-func setDataDir(config *server.ServerConfig) error {
+func setDataDir(config *server.Config) error {
 	if strings.HasPrefix(config.DataDir, "~/") {
 		expanded, err := homedir.Expand(config.DataDir)
 		if err != nil {
@@ -257,7 +257,7 @@ func setDataDir(config *server.ServerConfig) error {
 }
 
 // sanitizeGithubUser trims @ from the front of the github username if it exists.
-func sanitizeGithubUser(config *server.ServerConfig) {
+func sanitizeGithubUser(config *server.Config) {
 	config.GithubUser = strings.TrimPrefix(config.GithubUser, "@")
 }
 

cmd/server_test.go

@@ -8,19 +8,19 @@ import (
 
 	"github.com/hootsuite/atlantis/cmd"
 	"github.com/hootsuite/atlantis/server"
-	. "github.com/hootsuite/atlantis/testing_util"
+	. "github.com/hootsuite/atlantis/testing"
 	"github.com/mitchellh/go-homedir"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 )
 
 // passedConfig is set to whatever config ended up being passed to NewServer.
 // Used for testing.
-var passedConfig server.ServerConfig
+var passedConfig server.Config
 
 type ServerCreatorMock struct{}
 
-func (s *ServerCreatorMock) NewServer(config server.ServerConfig) (cmd.ServerStarter, error) {
+func (s *ServerCreatorMock) NewServer(config server.Config) (cmd.ServerStarter, error) {
 	passedConfig = config
 	return &ServerStarterMock{}, nil
 }
@@ -67,7 +67,7 @@ func TestExecute_ConfigFileMissing(t *testing.T) {
 func TestExecute_ConfigFileExists(t *testing.T) {
 	t.Log("If the config file exists then there should be no error.")
 	tmpFile := tempFile(t, "")
-	defer os.Remove(tmpFile)
+	defer os.Remove(tmpFile) // nolint: errcheck
 	c := setup(map[string]interface{}{
 		cmd.ConfigFlag:  tmpFile,
 		cmd.GHUserFlag:  "user",
@@ -80,7 +80,7 @@ func TestExecute_ConfigFileExists(t *testing.T) {
 func TestExecute_InvalidConfig(t *testing.T) {
 	t.Log("If the config file contains invalid yaml there should be an error.")
 	tmpFile := tempFile(t, "invalidyaml")
-	defer os.Remove(tmpFile)
+	defer os.Remove(tmpFile) // nolint: errcheck
 	c := setup(map[string]interface{}{
 		cmd.ConfigFlag:  tmpFile,
 		cmd.GHUserFlag:  "user",
@@ -222,7 +222,7 @@ gh-webhook-secret: "secret"
 log-level: "debug"
 port: 8181
 require-approval: true`)
-	defer os.Remove(tmpFile)
+	defer os.Remove(tmpFile) // nolint: errcheck
 	c := setup(map[string]interface{}{
 		cmd.ConfigFlag: tmpFile,
 	})
@@ -243,8 +243,8 @@ require-approval: true`)
 func TestExecute_EnvironmentOverride(t *testing.T) {
 	t.Log("Environment variables should override config file flags.")
 	tmpFile := tempFile(t, "gh-user: config\ngh-token: config2")
-	defer os.Remove(tmpFile)
-	os.Setenv("ATLANTIS_GH_TOKEN", "override")
+	defer os.Remove(tmpFile)                   // nolint: errcheck
+	os.Setenv("ATLANTIS_GH_TOKEN", "override") // nolint: errcheck
 	c := setup(map[string]interface{}{
 		cmd.ConfigFlag: tmpFile,
 	})
@@ -255,7 +255,7 @@ func TestExecute_EnvironmentOverride(t *testing.T) {
 
 func TestExecute_FlagConfigOverride(t *testing.T) {
 	t.Log("Flags should override config file flags.")
-	os.Setenv("ATLANTIS_GH_TOKEN", "env-var")
+	os.Setenv("ATLANTIS_GH_TOKEN", "env-var") // nolint: errcheck
 	c := setup(map[string]interface{}{
 		cmd.GHUserFlag:  "user",
 		cmd.GHTokenFlag: "override",
@@ -268,7 +268,7 @@ func TestExecute_FlagConfigOverride(t *testing.T) {
 func TestExecute_FlagEnvVarOverride(t *testing.T) {
 	t.Log("Flags should override environment variables.")
 	tmpFile := tempFile(t, "gh-user: config\ngh-token: config2")
-	defer os.Remove(tmpFile)
+	defer os.Remove(tmpFile) // nolint: errcheck
 	c := setup(map[string]interface{}{
 		cmd.ConfigFlag:  tmpFile,
 		cmd.GHTokenFlag: "override",
@@ -298,6 +298,6 @@ func tempFile(t *testing.T, contents string) string {
 	newName := f.Name() + ".yaml"
 	err = os.Rename(f.Name(), newName)
 	Ok(t, err)
-	ioutil.WriteFile(newName, []byte(contents), 0644)
+	ioutil.WriteFile(newName, []byte(contents), 0644) // nolint: errcheck
 	return newName
 }