Fix bug with top-level modules/ directory.

Fixes #12. Previously we would assume that if there was a change
in any modules/ directory, we should run plan in the parent. Now
we first check whether the parent has a main.tf file. If it doesn't
then we assume it's not actually the root of a project.

server/events/apply_executor.go

@@ -72,7 +72,7 @@ func (a *ApplyExecutor) Execute(ctx *CommandContext) CommandResponse {
 	}
 	ctx.Log.Info("found %d plan(s) in our workspace: %v", len(plans), paths)
 
-	results := []ProjectResult{}
+	var results []ProjectResult
 	for _, plan := range plans {
 		ctx.Log.Info("running apply for project at path %q", plan.Project.Path)
 		result := a.apply(ctx, repoDir, plan)

server/events/mocks/mock_project_finder.go

@@ -19,9 +19,9 @@ func NewMockProjectFinder() *MockProjectFinder {
 	return &MockProjectFinder{fail: pegomock.GlobalFailHandler}
 }
 
-func (mock *MockProjectFinder) FindModified(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string) []models.Project {
-	params := []pegomock.Param{log, modifiedFiles, repoFullName}
-	result := pegomock.GetGenericMockFrom(mock).Invoke("FindModified", params, []reflect.Type{reflect.TypeOf((*[]models.Project)(nil)).Elem()})
+func (mock *MockProjectFinder) DetermineProjects(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string, repoDir string) []models.Project {
+	params := []pegomock.Param{log, modifiedFiles, repoFullName, repoDir}
+	result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineProjects", params, []reflect.Type{reflect.TypeOf((*[]models.Project)(nil)).Elem()})
 	var ret0 []models.Project
 	if len(result) != 0 {
 		if result[0] != nil {
@@ -49,23 +49,23 @@ type VerifierProjectFinder struct {
 	inOrderContext         *pegomock.InOrderContext
 }
 
-func (verifier *VerifierProjectFinder) FindModified(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string) *ProjectFinder_FindModified_OngoingVerification {
-	params := []pegomock.Param{log, modifiedFiles, repoFullName}
-	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "FindModified", params)
-	return &ProjectFinder_FindModified_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
+func (verifier *VerifierProjectFinder) DetermineProjects(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string, repoDir string) *ProjectFinder_DetermineProjects_OngoingVerification {
+	params := []pegomock.Param{log, modifiedFiles, repoFullName, repoDir}
+	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "DetermineProjects", params)
+	return &ProjectFinder_DetermineProjects_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
 }
 
-type ProjectFinder_FindModified_OngoingVerification struct {
+type ProjectFinder_DetermineProjects_OngoingVerification struct {
 	mock              *MockProjectFinder
 	methodInvocations []pegomock.MethodInvocation
 }
 
-func (c *ProjectFinder_FindModified_OngoingVerification) GetCapturedArguments() (*logging.SimpleLogger, []string, string) {
-	log, modifiedFiles, repoFullName := c.GetAllCapturedArguments()
-	return log[len(log)-1], modifiedFiles[len(modifiedFiles)-1], repoFullName[len(repoFullName)-1]
+func (c *ProjectFinder_DetermineProjects_OngoingVerification) GetCapturedArguments() (*logging.SimpleLogger, []string, string, string) {
+	log, modifiedFiles, repoFullName, repoDir := c.GetAllCapturedArguments()
+	return log[len(log)-1], modifiedFiles[len(modifiedFiles)-1], repoFullName[len(repoFullName)-1], repoDir[len(repoDir)-1]
 }
 
-func (c *ProjectFinder_FindModified_OngoingVerification) GetAllCapturedArguments() (_param0 []*logging.SimpleLogger, _param1 [][]string, _param2 []string) {
+func (c *ProjectFinder_DetermineProjects_OngoingVerification) GetAllCapturedArguments() (_param0 []*logging.SimpleLogger, _param1 [][]string, _param2 []string, _param3 []string) {
 	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
 	if len(params) > 0 {
 		_param0 = make([]*logging.SimpleLogger, len(params[0]))
@@ -80,6 +80,10 @@ func (c *ProjectFinder_FindModified_OngoingVerification) GetAllCapturedArguments
 		for u, param := range params[2] {
 			_param2[u] = param.(string)
 		}
+		_param3 = make([]string, len(params[3]))
+		for u, param := range params[3] {
+			_param3[u] = param.(string)
+		}
 	}
 	return
 }

server/events/plan_executor.go

@@ -57,17 +57,18 @@ func (p *PlanExecutor) Execute(ctx *CommandContext) CommandResponse {
 	if err != nil {
 		return CommandResponse{Error: errors.Wrap(err, "getting modified files")}
 	}
-	ctx.Log.Info("found %d files modified in this pull request", len(modifiedFiles))
-	projects := p.ProjectFinder.FindModified(ctx.Log, modifiedFiles, ctx.BaseRepo.FullName)
-	if len(projects) == 0 {
-		return CommandResponse{Failure: "No Terraform files were modified."}
-	}
 
 	cloneDir, err := p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, ctx.Command.Workspace)
 	if err != nil {
 		return CommandResponse{Error: err}
 	}
 
+	ctx.Log.Info("found %d files modified in this pull request", len(modifiedFiles))
+	projects := p.ProjectFinder.DetermineProjects(ctx.Log, modifiedFiles, ctx.BaseRepo.FullName, cloneDir)
+	if len(projects) == 0 {
+		return CommandResponse{Failure: "No Terraform files were modified."}
+	}
+
 	var results []ProjectResult
 	for _, project := range projects {
 		ctx.Log.Info("running plan for project at path %q", project.Path)

server/events/project_finder.go

@@ -1,7 +1,9 @@
 package events
 
 import (
+	"os"
 	"path"
+	"path/filepath"
 	"strings"
 
 	"github.com/runatlantis/atlantis/server/events/models"
@@ -12,9 +14,9 @@ import (
 
 // ProjectFinder determines what are the terraform project(s) within a repo.
 type ProjectFinder interface {
-	// FindModified returns the list of projects that were modified based on
+	// DetermineProjects returns the list of projects that were modified based on
 	// the modifiedFiles. The list will be de-duplicated.
-	FindModified(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string) []models.Project
+	DetermineProjects(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string, repoDir string) []models.Project
 }
 
 // DefaultProjectFinder implements ProjectFinder.
@@ -22,9 +24,9 @@ type DefaultProjectFinder struct{}
 
 var excludeList = []string{"terraform.tfstate", "terraform.tfstate.backup"}
 
-// FindModified returns the list of projects that were modified based on
+// DetermineProjects returns the list of projects that were modified based on
 // the modifiedFiles. The list will be de-duplicated.
-func (p *DefaultProjectFinder) FindModified(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string) []models.Project {
+func (p *DefaultProjectFinder) DetermineProjects(log *logging.SimpleLogger, modifiedFiles []string, repoFullName string, repoDir string) []models.Project {
 	var projects []models.Project
 
 	modifiedTerraformFiles := p.filterToTerraform(modifiedFiles)
@@ -36,7 +38,10 @@ func (p *DefaultProjectFinder) FindModified(log *logging.SimpleLogger, modifiedF
 
 	var paths []string
 	for _, modifiedFile := range modifiedTerraformFiles {
-		paths = append(paths, p.getProjectPath(modifiedFile))
+		projectPath := p.getProjectPath(modifiedFile, repoDir)
+		if projectPath != "" {
+			paths = append(paths, projectPath)
+		}
 	}
 	uniquePaths := p.unique(paths)
 	for _, uniquePath := range uniquePaths {
@@ -66,23 +71,68 @@ func (p *DefaultProjectFinder) isInExcludeList(fileName string) bool {
 	return false
 }
 
-// getProjectPath returns the path to the project relative to the repo root
-// if the project is at the root returns ".".
-func (p *DefaultProjectFinder) getProjectPath(modifiedFilePath string) string {
+// getProjectPath attempts to determine based on the location of a modified
+// file, where the root of the Terraform project is. It also attempts to verify
+// if the root is valid by looking for a main.tf file. It returns a relative
+// path. If the project is at the root returns ".". If modified file doesn't
+// lead to a valid project path, returns an empty string.
+func (p *DefaultProjectFinder) getProjectPath(modifiedFilePath string, repoDir string) string {
 	dir := path.Dir(modifiedFilePath)
 	if path.Base(dir) == "env" {
 		// If the modified file was inside an env/ directory, we treat this
-		// specially and run plan one level up.
+		// specially and run plan one level up. This supports directory structures
+		// like:
+		// root/
+		//   main.tf
+		//   env/
+		//     dev.tfvars
+		//     staging.tfvars
 		return path.Dir(dir)
 	}
-	// Need to add a trailing slash before splitting on modules/ because if
-	// the input was modules/file.tf then path.Dir will be "modules" and so our
-	// split on "modules/" will fail.
-	dirWithTrailingSlash := dir + "/"
-	// It's safe to do this split even if there is no modules/ in the path
-	// because SplitN will just return the original string.
-	modulesSplit := strings.SplitN(dirWithTrailingSlash, "modules/", 2)
-	return path.Clean(modulesSplit[0])
+
+	// Surrounding dir with /'s so we can match on /modules/ even if dir is
+	// "modules" or "project1/modules"
+	if strings.Contains("/"+dir+"/", "/modules/") {
+		// We treat changes inside modules/ folders specially. There are two cases:
+		// 1. modules folder inside project:
+		// root/
+		//   main.tf
+		//     modules/
+		//       ...
+		// In this case, if we detect a change in modules/, we will determine
+		// the project root to be at root/.
+		//
+		// 2. shared top-level modules folder
+		// root/
+		//  project1/
+		//    main.tf # uses modules via ../modules
+		//  project2/
+		//    main.tf # uses modules via ../modules
+		//  modules/
+		//    ...
+		// In this case, if we detect a change in modules/ we don't know which
+		// project was using this module so we can't suggest a project root, but we
+		// also detect that there's no main.tf in the parent folder of modules/
+		// so we won't suggest that as a project. So in this case we return nothing.
+		// The code below makes this happen.
+
+		// Need to add a trailing slash before splitting on modules/ because if
+		// the input was modules/file.tf then path.Dir will be "modules" and so our
+		// split on "modules/" will fail.
+		dirWithTrailingSlash := dir + "/"
+		modulesSplit := strings.SplitN(dirWithTrailingSlash, "modules/", 2)
+		modulesParent := modulesSplit[0]
+
+		// Now we check whether there is a main.tf in the parent.
+		if _, err := os.Stat(filepath.Join(repoDir, modulesParent, "main.tf")); os.IsNotExist(err) {
+			return ""
+		}
+		return path.Clean(modulesParent)
+	}
+
+	// If it wasn't a modules directory, we assume we're in a project and return
+	// this directory.
+	return dir
 }
 
 func (p *DefaultProjectFinder) unique(strs []string) []string {

server/events/project_finder_test.go

@@ -1,6 +1,9 @@
 package events_test
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/runatlantis/atlantis/server/events"
@@ -11,72 +14,143 @@ import (
 var noopLogger = logging.NewNoopLogger()
 var modifiedRepo = "owner/repo"
 var m = events.DefaultProjectFinder{}
+var nestedModules1 string
+var nestedModules2 string
+var topLevelModules string
+
+func setupTmpRepos(t *testing.T) {
+	// Create different repo structures for testing.
+
+	// 1. Nested modules directory inside a project
+	// project1/
+	//   main.tf
+	//   modules/
+	//     main.tf
+	var err error
+	nestedModules1, err = ioutil.TempDir("", "")
+	Ok(t, err)
+	err = os.MkdirAll(filepath.Join(nestedModules1, "project1/modules"), 0700)
+	Ok(t, err)
+	_, err = os.Create(filepath.Join(nestedModules1, "project1/main.tf"))
+	Ok(t, err)
+	_, err = os.Create(filepath.Join(nestedModules1, "project1/modules/main.tf"))
+	Ok(t, err)
+
+	// 2. Nested modules dir inside top-level project
+	// main.tf
+	//  modules/
+	//    main.tf
+	// We can just re-use part of the previous dir structure.
+	nestedModules2 = filepath.Join(nestedModules1, "project1")
+
+	// 3. Top-level modules
+	//  modules/
+	//    main.tf
+	//  project1/
+	//    main.tf
+	//  project2/
+	//    main.tf
+	topLevelModules, err = ioutil.TempDir("", "")
+	Ok(t, err)
+	for _, path := range []string{"modules", "project1", "project2"} {
+		err = os.MkdirAll(filepath.Join(topLevelModules, path), 0700)
+		Ok(t, err)
+		_, err = os.Create(filepath.Join(topLevelModules, path, "main.tf"))
+		Ok(t, err)
+	}
+}
+
+func TestDetermineProjects(t *testing.T) {
+	setupTmpRepos(t)
 
-func TestGetModified(t *testing.T) {
 	cases := []struct {
 		description     string
 		files           []string
 		expProjectPaths []string
+		repoDir         string
 	}{
 		{
 			"If no files were modified then should return an empty list",
 			nil,
 			nil,
+			"",
 		},
 		{
 			"Should ignore non .tf files and return an empty list",
 			[]string{"non-tf"},
 			nil,
+			"",
 		},
 		{
-			"Should plan in the parent directory from modules",
-			[]string{"modules/file.tf"},
+			"Should plan in the parent directory from modules if that dir has a main.tf",
+			[]string{"project1/modules/main.tf"},
+			[]string{"project1"},
+			nestedModules1,
+		},
+		{
+			"Should plan in the parent directory from modules if that dir has a main.tf",
+			[]string{"modules/main.tf"},
 			[]string{"."},
+			nestedModules2,
 		},
 		{
-			"Should plan in the parent directory from modules when module is in a subdir",
-			[]string{"modules/subdir/file.tf"},
+			"Should plan in the parent directory from modules when module is in a subdir if that dir has a main.tf",
+			[]string{"modules/subdir/main.tf"},
 			[]string{"."},
+			nestedModules2,
 		},
 		{
-			"Should plan in the parent directory from modules when project is in its own dir",
-			[]string{"projectdir/modules/file.tf"},
-			[]string{"projectdir"},
+			"Should not plan in the parent directory from modules if that dir does not have a main.tf",
+			[]string{"modules/main.tf"},
+			[]string{},
+			topLevelModules,
+		},
+		{
+			"Should not plan in the parent directory from modules if that dir does not have a main.tf",
+			[]string{"modules/main.tf", "project1/main.tf"},
+			[]string{"project1"},
+			topLevelModules,
 		},
 		{
 			"Should ignore tfstate files and return an empty list",
 			[]string{"terraform.tfstate", "terraform.tfstate.backup", "parent/terraform.tfstate", "parent/terraform.tfstate.backup"},
 			nil,
+			"",
 		},
 		{
 			"Should ignore tfstate files and return an empty list",
 			[]string{"terraform.tfstate", "terraform.tfstate.backup", "parent/terraform.tfstate", "parent/terraform.tfstate.backup"},
 			nil,
+			"",
 		},
 		{
 			"Should return '.' when changed file is at root",
 			[]string{"a.tf"},
 			[]string{"."},
+			"",
 		},
 		{
 			"Should return directory when changed file is in a dir",
 			[]string{"parent/a.tf"},
 			[]string{"parent"},
+			"",
 		},
 		{
 			"Should return parent dir when changed file is in an env/ dir",
 			[]string{"env/a.tfvars"},
 			[]string{"."},
+			"",
 		},
 		{
 			"Should de-duplicate when multiple files changed in the same dir",
 			[]string{"root.tf", "env/env.tfvars", "parent/parent.tf", "parent/parent2.tf", "parent/child/child.tf", "parent/child/env/env.tfvars"},
 			[]string{".", "parent", "parent/child"},
+			"",
 		},
 	}
 	for _, c := range cases {
 		t.Log(c.description)
-		projects := m.FindModified(noopLogger, c.files, modifiedRepo)
+		projects := m.DetermineProjects(noopLogger, c.files, modifiedRepo, c.repoDir)
 
 		// Extract the paths from the projects. We use a slice here instead of a
 		// map so we can test whether there are duplicates returned.