From 6b0eece5c9457d45fdaa5e8cd4c20abd4d41f440 Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Tue, 20 Feb 2018 12:54:10 -0800 Subject: [PATCH] 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 | 2 +- server/events/mocks/mock_project_finder.go | 28 ++++--- server/events/plan_executor.go | 11 +-- server/events/project_finder.go | 84 ++++++++++++++++---- server/events/project_finder_test.go | 92 +++++++++++++++++++--- 5 files changed, 173 insertions(+), 44 deletions(-) diff --git a/server/events/apply_executor.go b/server/events/apply_executor.go index 0466115a74..47cfa658b4 100644 --- a/server/events/apply_executor.go +++ b/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) diff --git a/server/events/mocks/mock_project_finder.go b/server/events/mocks/mock_project_finder.go index a666438389..dbd269cbdb 100644 --- a/server/events/mocks/mock_project_finder.go +++ b/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 } diff --git a/server/events/plan_executor.go b/server/events/plan_executor.go index fcc679e835..17583a9ebf 100644 --- a/server/events/plan_executor.go +++ b/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) diff --git a/server/events/project_finder.go b/server/events/project_finder.go index 41124dc16e..9b6e278b7b 100644 --- a/server/events/project_finder.go +++ b/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 { diff --git a/server/events/project_finder_test.go b/server/events/project_finder_test.go index 0aee87cfda..ab344f7aaa 100644 --- a/server/events/project_finder_test.go +++ b/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 if that dir has a main.tf", + []string{"project1/modules/main.tf"}, + []string{"project1"}, + nestedModules1, }, { - "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{"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 not plan in the parent directory from modules if that dir does not have a main.tf", + []string{"modules/main.tf"}, + []string{}, + topLevelModules, }, { - "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", "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.