Add golangci-lint to the Travis tests (#36)

This also fixes some issues that golangci-lint identified.

Author: bigkevmcd

.travis.yml

@@ -1,3 +1,10 @@
 language: go
 go: 1.13
 sudo: false
+
+before_install:
+  - curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s -- -b $GOPATH/bin v1.24.0
+
+script:
+  - $GOPATH/bin/golangci-lint run
+  - go test ./...

pkg/avancement/service_manager.go

@@ -29,7 +29,7 @@ const (
 
 type scmClientFactory func(token string) *scm.Client
 type repoFactory func(url, localPath string, debug bool) (git.Repo, error)
-type localFactory func(localPath string, debug bool) (git.Source, error)
+type localFactory func(localPath string, debug bool) git.Source
 type serviceOpt func(*ServiceManager)
 
 // New creates and returns a new ServiceManager.
@@ -45,9 +45,9 @@ func New(cacheDir string, author *git.Author, opts ...serviceOpt) *ServiceManage
 			r, err := git.NewRepository(url, localPath, debug)
 			return git.Repo(r), err
 		},
-		localFactory: func(localPath string, debug bool)(git.Source, error){
+		localFactory: func(localPath string, debug bool) git.Source {
 			l := &local.Local{LocalPath: localPath, Debug: debug, Logger: log.Printf}
-			return git.Source(l), nil
+			return git.Source(l)
 		},
 	}
 	for _, o := range opts {
@@ -79,9 +79,9 @@ func (s *ServiceManager) Promote(serviceName, fromURL, toURL, newBranchName stri
 	defer func(keepRepos bool, repos *[]git.Repo) {
 		if !keepRepos {
 			for _, repo := range *repos {
-				err := repo.DeleteCachedRepo()
+				err := repo.DeleteCache()
 				if err != nil {
-					fmt.Errorf("failed deleting files from cache: %w", err)
+					log.Printf("failed deleting files from cache: %s", err)
 				}
 			}
 		}
@@ -93,9 +93,9 @@ func (s *ServiceManager) Promote(serviceName, fromURL, toURL, newBranchName stri
 	}
 	reposToDelete = append(reposToDelete, destination)
 
-	copied := []string{}
+	var copied []string
 	if fromLocalRepo(fromURL) {
-		localSource, err := s.localFactory(fromURL, s.debug) 
+		localSource := s.localFactory(fromURL, s.debug)
 		copied, err = local.CopyConfig(serviceName, localSource, destination)
 		if err != nil {
 			return fmt.Errorf("failed to setup local repo: %w", err)

pkg/avancement/service_manager_test.go

@@ -1,10 +1,11 @@
 package avancement
 
 import (
-	"testing"
+	"errors"
+	"path"
 	"path/filepath"
 	"strings"
-	"path"
+	"testing"
 
 	"github.com/jenkins-x/go-scm/scm"
 	fakescm "github.com/jenkins-x/go-scm/scm/driver/fake"
@@ -29,17 +30,16 @@ func TestPromoteWithSuccessKeepCacheFalse(t *testing.T) {
 func promoteWithSuccess(t *testing.T, keepCache bool) {
 	dstBranch := "test-branch"
 	author := &git.Author{Name: "Testing User", Email: "[email protected]", Token: "test-token"}
-	client, _ := fakescm.NewDefault()
-	fakeClientFactory := func(s string) *scm.Client {
-		return client
-	}
 	devRepo, stagingRepo := mock.New("/dev", "master"), mock.New("/staging", "master")
 	repos := map[string]*mock.Repository{
 		mustAddCredentials(t, dev, author):     devRepo,
 		mustAddCredentials(t, staging, author): stagingRepo,
 	}
 	sm := New("tmp", author)
-	sm.clientFactory = fakeClientFactory
+	sm.clientFactory = func(s string) *scm.Client {
+		client, _ := fakescm.NewDefault()
+		return client
+	}
 	sm.repoFactory = func(url, _ string, _ bool) (git.Repo, error) {
 		return git.Repo(repos[url]), nil
 	}
@@ -75,20 +75,19 @@ func TestPromoteLocalWithSuccessKeepCacheTrue(t *testing.T) {
 func promoteLocalWithSuccess(t *testing.T, keepCache bool) {
 	dstBranch := "test-branch"
 	author := &git.Author{Name: "Testing User", Email: "[email protected]", Token: "test-token"}
-	client, _ := fakescm.NewDefault()
-	fakeClientFactory := func(s string) *scm.Client {
-		return client
-	}
 	stagingRepo := mock.New("/staging", "master")
 	devRepo := NewLocal("/dev")
 
 	sm := New("tmp", author)
-	sm.clientFactory = fakeClientFactory
+	sm.clientFactory = func(s string) *scm.Client {
+		client, _ := fakescm.NewDefault()
+		return client
+	}
 	sm.repoFactory = func(url, _ string, _ bool) (git.Repo, error) {
 		return git.Repo(stagingRepo), nil
 	}
-	sm.localFactory = func(path string, _ bool) (git.Source, error) {
-		return git.Source(devRepo), nil
+	sm.localFactory = func(path string, _ bool) git.Source {
+		return git.Source(devRepo)
 	}
 	sm.debug = true
 	devRepo.AddFiles("/config/myfile.yaml")
@@ -143,19 +142,19 @@ func mustAddCredentials(t *testing.T, repoURL string, a *git.Author) string {
 }
 
 func TestPromoteWithCacheDeletionFailure(t *testing.T) {
-	dstBranch := mock.FAIL_DELETING_REPO_BRANCH
+	dstBranch := "test-branch"
 	author := &git.Author{Name: "Testing User", Email: "[email protected]", Token: "test-token"}
-	client, _ := fakescm.NewDefault()
-	fakeClientFactory := func(s string) *scm.Client {
-		return client
-	}
 	devRepo, stagingRepo := mock.New("/dev", "master"), mock.New("/staging", "master")
+	stagingRepo.DeleteErr = errors.New("failed test delete")
 	repos := map[string]*mock.Repository{
 		mustAddCredentials(t, dev, author):     devRepo,
 		mustAddCredentials(t, staging, author): stagingRepo,
 	}
 	sm := New("tmp", author)
-	sm.clientFactory = fakeClientFactory
+	sm.clientFactory = func(s string) *scm.Client {
+		client, _ := fakescm.NewDefault()
+		return client
+	}
 	sm.repoFactory = func(url, _ string, _ bool) (git.Repo, error) {
 		return git.Repo(repos[url]), nil
 	}
@@ -175,7 +174,6 @@ func TestPromoteWithCacheDeletionFailure(t *testing.T) {
 	devRepo.AssertDeletedFromCache(t)
 }
 
-
 type mockSource struct {
 	files     []string
 	localPath string
@@ -193,11 +191,11 @@ func NewLocal(localPath string) *mockSource {
 // and then calls filePath.Walk() on /full/path/to/repo/services/ .
 // When CopyService() drives Walk(), 'base' is typically services/service-name
 // Thus we take each /full/path/to/file/in/mockSource.files[] and split it at 'services/' as happens in the Walk() method we're mocking.
-func (s *mockSource) Walk(base string, cb func(string, string) error) error {
+func (s *mockSource) Walk(_ string, cb func(string, string) error) error {
 	if s.files == nil {
 		return nil
 	}
-	base = filepath.Join(s.localPath, "config")
+	base := filepath.Join(s.localPath, "config")
 
 	for _, f := range s.files {
 		splitString := filepath.Dir(base) + "/"

pkg/git/interface.go

@@ -26,5 +26,5 @@ type Repo interface {
 	StageFiles(filenames ...string) error
 	Commit(msg string, author *Author) error
 	Push(branch string) error
-	DeleteCachedRepo() error
+	DeleteCache() error
 }

pkg/git/mock/mock.go

@@ -1,7 +1,6 @@
 package mock
 
 import (
-	"errors"
 	"io"
 	"path"
 	"path/filepath"
@@ -11,10 +10,6 @@ import (
 	"github.com/rhd-gitops-example/services/pkg/git"
 )
 
-/* FAIL_DELETING_REPO_BRANCH is a branch name that when used as a target branch will
-   cause the the deletion from the cache to fail (in tests only) */
-const FAIL_DELETING_REPO_BRANCH = "fail_to_delete_repo"
-
 type Repository struct {
 	currentBranch string
 	knownBranches []string
@@ -32,13 +27,13 @@ type Repository struct {
 	copyFileErr error
 
 	commits   []string
-	commitErr error
+	CommitErr error
 
 	pushedBranches []string
 	pushErr        error
 
-	deleted    bool
-	deletedErr error
+	deleted   bool
+	DeleteErr error
 }
 
 // New creates and returns a new git.Cache implementation that operates entirely
@@ -82,7 +77,7 @@ func (m *Repository) Commit(msg string, author *git.Author) error {
 		m.commits = []string{}
 	}
 	m.commits = append(m.commits, key(m.currentBranch, msg, author.Token))
-	return m.commitErr
+	return m.CommitErr
 }
 
 // Push fulfils the git.Repo interface.
@@ -148,15 +143,13 @@ func (m *Repository) AddFiles(names ...string) {
 	}
 }
 
-// DeleteCachedRepo deletes the repo from the local cache directory
-func (m *Repository) DeleteCachedRepo() error {
-	if m.currentBranch == FAIL_DELETING_REPO_BRANCH {
-		m.deleted = false
-		m.deletedErr = errors.New("MOCK FAILURE DELETING REPO MESSAGE")
-	} else {
-		m.deleted = true
+// DeleteCache deletes the repo from the local cache directory.
+func (m *Repository) DeleteCache() error {
+	if m.DeleteErr != nil {
+		return m.DeleteErr
 	}
-	return m.deletedErr
+	m.deleted = true
+	return nil
 }
 
 // AssertBranchCreated asserts that the named branch was created from the from
@@ -200,7 +193,7 @@ func (m *Repository) AssertDeletedFromCache(t *testing.T) {
 // AssertNotDeletedFromCache asserts that delete was called to remove the local repo
 func (m *Repository) AssertNotDeletedFromCache(t *testing.T) {
 	if m.deleted {
-		t.Fatalf("repo was unexpectedly deleted from the promotion cache directory : %w", m.deletedErr)
+		t.Fatal("repo was unexpectedly deleted from the promotion cache directory")
 	}
 }
 

pkg/git/repository.go

@@ -156,8 +156,8 @@ func envFromAuthor(a *Author) []string {
 		sf("GIT_AUTHOR_EMAIL", a.Email)}
 }
 
-// DeleteCachedRepo removes the local clones from the promotion cache
-func (r *Repository) DeleteCachedRepo() error {
+// DeleteCache removes the local clones from the promotion cache.
+func (r *Repository) DeleteCache() error {
 	err := os.RemoveAll(r.LocalPath)
 	if err != nil {
 		return fmt.Errorf("failed deleting `%s` : %w", r.LocalPath, err)

pkg/local/local.go

@@ -5,24 +5,22 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
 	"github.com/rhd-gitops-example/services/pkg/git"
 )
 
-
 type Local struct {
 	LocalPath string
 	Debug     bool
 	Logger    func(fmt string, v ...interface{})
 }
 
-
 // CopyConfig takes the name of a service and a Source local service root path to be copied to a Destination.
 //
-// Only files under /path/to/local/repo/config/* are copied to the destination /services/[serviceName]/base/config/* 
+// Only files under /path/to/local/repo/config/* are copied to the destination /services/[serviceName]/base/config/*
 //
 // Returns the list of files that were copied, and possibly an error.
 func CopyConfig(serviceName string, source git.Source, dest git.Destination) ([]string, error) {
-
 	copied := []string{}
 	err := source.Walk("", func(prefix, name string) error {
 		sourcePath := path.Join(prefix, name)
@@ -42,8 +40,8 @@ func pathForDestServiceConfig(serviceName, name string) string {
 	return filepath.Join("services/", serviceName, "base", name)
 }
 
-func (l *Local) Walk(base string, cb func(prefix, name string) error) error {
-	base = filepath.Join(l.LocalPath, "config")
+func (l *Local) Walk(_ string, cb func(prefix, name string) error) error {
+	base := filepath.Join(l.LocalPath, "config")
 	prefix := filepath.Dir(base) + "/"
 	return filepath.Walk(base, func(path string, info os.FileInfo, err error) error {
 		if err != nil {

pkg/local/local_test.go

@@ -31,7 +31,6 @@ func TestCopyConfig(t *testing.T) {
 	}
 }
 
-
 type mockSource struct {
 	files     []string
 	localPath string
@@ -45,11 +44,11 @@ type mockSource struct {
 // and then calls filePath.Walk() on /full/path/to/repo/services/ .
 // When CopyService() drives Walk(), 'base' is typically services/service-name
 // Thus we take each /full/path/to/file/in/mockSource.files[] and split it at 'services/' as happens in the Walk() method we're mocking.
-func (s *mockSource) Walk(base string, cb func(string, string) error) error {
+func (s *mockSource) Walk(_ string, cb func(string, string) error) error {
 	if s.files == nil {
 		return nil
 	}
-	base = filepath.Join(s.localPath, "config")
+	base := filepath.Join(s.localPath, "config")
 
 	for _, f := range s.files {
 		splitString := filepath.Dir(base) + "/"