Fix repository.Manager unit tests (#20)

Follow up to #19

Description of changes:
- Fix `repository.Manager` unit tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: a-hilaly

cmd/ackdev/cmd/add_repo.go

@@ -14,20 +14,24 @@
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
+	"github.com/spf13/cobra"
+
 	"github.com/aws-controllers-k8s/dev-tools/pkg/config"
 	"github.com/aws-controllers-k8s/dev-tools/pkg/repository"
 	"github.com/aws-controllers-k8s/dev-tools/pkg/util"
-	"github.com/spf13/cobra"
 )
 
 var (
-	ErrRepositoryAlreadyAdded = errors.New("repository has already been added")
+	optAddRepoType string
 )
 
+func init() {
+	addRepositoryCmd.PersistentFlags().StringVarP(&optAddRepoType, "type", "t", "controller", "repository type")
+}
+
 var addRepositoryCmd = &cobra.Command{
 	Use:     "repository <service> ...",
 	Aliases: []string{"repo", "repos", "repository", "repositories"},
@@ -36,33 +40,39 @@ var addRepositoryCmd = &cobra.Command{
 }
 
 func addRepository(cmd *cobra.Command, args []string) error {
-	for _, service := range args {
-		service = strings.ToLower(args[0])
+	cfg, err := config.Load(ackConfigPath)
+	if err != nil {
+		return err
+	}
 
-		ctx := cmd.Context()
+	repoManager, err := repository.NewManager(cfg)
+	if err != nil {
+		return err
+	}
 
-		cfg, err := config.Load(ackConfigPath)
-		if err != nil {
-			return err
-		}
+	for _, service := range args {
+		service = strings.ToLower(service)
 
 		// Check it doesn't already exist in the configuration
 		if util.InStrings(service, cfg.Repositories.Services) {
-			return ErrRepositoryAlreadyAdded
+			fmt.Printf("repository for service %s has already been added\n", service)
+			continue
 		}
 
-		repoManager, err := repository.NewManager(cfg)
+		_, err := repoManager.AddRepository(service, repository.GetRepositoryTypeFromString(optAddRepoType))
 		if err != nil {
 			return err
 		}
 
-		serviceRepoName := fmt.Sprintf("%s-controller", service)
-		if err := repoManager.EnsureRepository(ctx, serviceRepoName); err != nil {
+		ctx := cmd.Context()
+		if err := repoManager.EnsureRepository(ctx, service); err != nil {
 			return err
 		}
 
 		cfg.Repositories.Services = append(cfg.Repositories.Services, service)
-		config.Save(cfg, ackConfigPath)
+		if err := config.Save(cfg, ackConfigPath); err != nil {
+			return err
+		}
 	}
 
 	return nil

pkg/repository/filter.go

@@ -82,7 +82,7 @@ func NamePrefixFilter(namePrefix string) Filter {
 // The only two possible types are 'controller' and 'core'
 func TypeFilter(t string) Filter {
 	return func(r *Repository) bool {
-		return r.Type == repositoryTypeFromString(t)
+		return r.Type == GetRepositoryTypeFromString(t)
 	}
 }
 

pkg/repository/manager.go

@@ -29,6 +29,7 @@ import (
 	"github.com/aws-controllers-k8s/dev-tools/pkg/config"
 	ackdevgit "github.com/aws-controllers-k8s/dev-tools/pkg/git"
 	"github.com/aws-controllers-k8s/dev-tools/pkg/github"
+	"github.com/aws-controllers-k8s/dev-tools/pkg/util"
 )
 
 const (
@@ -89,26 +90,42 @@ type Manager struct {
 }
 
 // LoadRepository loads information about a single local repository
-func (m *Manager) LoadRepository(name string) (*Repository, error) {
+func (m *Manager) LoadRepository(name string, t RepositoryType) (*Repository, error) {
 	// check repo cache
 	repo, err := m.getRepository(name)
 	if err == nil {
 		return repo, nil
 	}
 
-	repoName := name
+	switch t {
+	case RepositoryTypeCore:
+		if !util.InStrings(name, m.cfg.Repositories.Core) {
+			return nil, ErrUnconfiguredRepository
+		}
+	case RepositoryTypeController:
+		if !util.InStrings(name, m.cfg.Repositories.Services) {
+			return nil, ErrUnconfiguredRepository
+		}
+	}
+
+	return m.AddRepository(name, t)
+}
+
+// AddRepository creates a new Repository object and adds it to the cache.
+func (m *Manager) AddRepository(name string, t RepositoryType) (*Repository, error) {
+	repo := NewRepository(name, t)
 
 	// set expected fork name
-	expectedForkName := repoName
+	expectedForkName := repo.Name
 	if m.cfg.Github.ForkPrefix != "" {
-		expectedForkName = fmt.Sprintf("%s%s", m.cfg.Github.ForkPrefix, repoName)
+		expectedForkName = fmt.Sprintf("%s%s", m.cfg.Github.ForkPrefix, repo.Name)
 	}
 
 	var gitHead string
 	var gitRepo *git.Repository
-	fullPath := filepath.Join(m.cfg.RootDirectory, repoName)
+	fullPath := filepath.Join(m.cfg.RootDirectory, repo.Name)
 
-	gitRepo, err = m.git.Open(fullPath)
+	gitRepo, err := m.git.Open(fullPath)
 	if err != nil && err != git.ErrRepositoryNotExists {
 		return nil, err
 	} else if err == nil {
@@ -120,20 +137,10 @@ func (m *Manager) LoadRepository(name string) (*Repository, error) {
 		gitHead = head.Name().Short()
 	}
 
-	t := RepositoryTypeCore
-	if strings.HasSuffix(name, "-controller") {
-		t = RepositoryTypeController
-	}
-
-	repo = &Repository{
-		Name:             repoName,
-		Type:             t,
-		gitRepo:          gitRepo,
-		GitHead:          gitHead,
-		FullPath:         fullPath,
-		ExpectedForkName: expectedForkName,
-	}
-
+	repo.gitRepo = gitRepo
+	repo.GitHead = gitHead
+	repo.FullPath = fullPath
+	repo.ExpectedForkName = expectedForkName
 	// cache repository
 	m.repoCache[name] = repo
 	return repo, nil
@@ -144,14 +151,14 @@ func (m *Manager) LoadRepository(name string) (*Repository, error) {
 func (m *Manager) LoadAll() error {
 	// collect repositories from config
 	for _, coreRepo := range m.cfg.Repositories.Core {
-		_, err := m.LoadRepository(coreRepo)
+		_, err := m.LoadRepository(coreRepo, RepositoryTypeCore)
 		if err != nil {
 			return err
 		}
 	}
 	for _, serviceName := range m.cfg.Repositories.Services {
 		serviceRepoName := fmt.Sprintf("%s-controller", serviceName)
-		_, err := m.LoadRepository(serviceRepoName)
+		_, err := m.LoadRepository(serviceRepoName, RepositoryTypeController)
 		if err != nil {
 			return err
 		}
@@ -174,7 +181,7 @@ func (m *Manager) List(filters ...Filter) []*Repository {
 	repoNames := append(m.cfg.Repositories.Core, m.cfg.Repositories.Services...)
 mainLoop:
 	for _, repoName := range repoNames {
-		repo, err := m.LoadRepository(repoName)
+		repo, err := m.getRepository(repoName)
 		if err != nil {
 			continue
 		}
@@ -190,7 +197,10 @@ mainLoop:
 
 // Clone clones a known repository to the config root directory
 func (m *Manager) clone(ctx context.Context, repoName string) error {
-	repo, err := m.LoadRepository(repoName)
+	// TODO(a-hilaly) ideally we need to store all repository names (service name, fork name,
+	// local clone name etc...)
+	repoName = strings.TrimSuffix(repoName, "-controller")
+	repo, err := m.getRepository(repoName)
 	if err != nil {
 		return fmt.Errorf("cannot clone repository %s: %v", repoName, err)
 	}
@@ -277,7 +287,7 @@ func (m *Manager) EnsureClone(ctx context.Context, repo *Repository) error {
 // EnsureRepository ensures the current user owns a fork of the given repository
 // and has cloned it.
 func (m *Manager) EnsureRepository(ctx context.Context, name string) error {
-	repo, err := m.LoadRepository(name)
+	repo, err := m.getRepository(name)
 	if err != nil && err != ErrRepositoryDoesntExist {
 		return err
 	}

pkg/repository/manager_test.go

@@ -164,7 +164,7 @@ func TestManager_LoadAll(t *testing.T) {
 				git:       fakeGit,
 				repoCache: make(map[string]*Repository),
 			},
-			wantErr: false,
+			wantErr: true,
 		},
 		{
 			name: "unexpected repository error",
@@ -210,19 +210,19 @@ func TestManager_clone(t *testing.T) {
 		"Clone",
 		testingCtx,
 		"https://github.com/ack-bot/ack-ecr-controller.git",
-		"ack-ecr-controller",
+		"ecr-controller",
 	).Return(transport.ErrAuthenticationRequired)
 	fakeGit.On(
 		"Clone",
 		testingCtx,
 		"https://github.com/ack-bot/ack-mq-controller.git",
-		"ack-mq-controller",
+		"mq-controller",
 	).Return(gitconfig.ErrRemoteConfigNotFound)
 	fakeGit.On(
 		"Clone",
 		testingCtx,
 		"https://github.com/ack-bot/ack-sagemaker-controller.git",
-		"ack-sagemaker-controller",
+		"sagemaker-controller",
 	).Return(nil)
 
 	type fields struct {

pkg/repository/repository.go

@@ -21,6 +21,9 @@ import (
 
 // NewRepository returns a pointer to a new repository.
 func NewRepository(name string, repoType RepositoryType) *Repository {
+	if repoType == RepositoryTypeController {
+		name = fmt.Sprintf("%s-controller", name)
+	}
 	return &Repository{
 		Name: name,
 		Type: repoType,

pkg/repository/type.go

@@ -36,14 +36,14 @@ func (rt RepositoryType) String() string {
 	}
 }
 
-// repositoryTypeFromString casts a string to a RepositoryType
-func repositoryTypeFromString(s string) RepositoryType {
+// GetRepositoryTypeFromString casts a string to a RepositoryType
+func GetRepositoryTypeFromString(s string) RepositoryType {
 	switch s {
 	case "core":
 		return RepositoryTypeCore
 	case "controller":
 		return RepositoryTypeController
 	default:
-		panic("unsupported repository type")
+		panic("unsupported repository type: " + s)
 	}
 }