Fix apply all with project name bug

Previously, if you were using project names and you had two projects
with the same directory and workspace but different workflows, running
"atlantis apply" would fail. This was because we weren't figuring out
what the project name was for each pending plan.

This change fixes this by extracting the project name from the planfile.
We also handle /'s in the project name by substituting them with '::'.
This allows us to convert them back to /'s when we're figuring out the
project names from the filenames. It's safe to do this because : isn't
an allowed character for project names.

We also remove a final substitution of invalid filename characters
because we rely on the validation of the project names instead which
happens when validating the atlantis.yaml file.

server/events/pending_plan_finder.go

@@ -1,6 +1,7 @@
 package events
 
 import (
+	"github.com/runatlantis/atlantis/server/events/runtime"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -29,7 +30,8 @@ type PendingPlan struct {
 	// the plan is for.
 	RepoRelDir string
 	// Workspace is the workspace this plan should execute in.
-	Workspace string
+	Workspace   string
+	ProjectName string
 }
 
 // Find finds all pending plans in pullDir. pullDir should be the working
@@ -67,11 +69,15 @@ func (p *DefaultPendingPlanFinder) findWithAbsPaths(pullDir string) ([]PendingPl
 					continue
 				}
 
-				repoRelDir := filepath.Dir(file)
+				projectName, err := runtime.ProjectNameFromPlanfile(workspace, filepath.Base(file))
+				if err != nil {
+					return nil, nil, err
+				}
 				plans = append(plans, PendingPlan{
-					RepoDir:    repoDir,
-					RepoRelDir: repoRelDir,
-					Workspace:  workspace,
+					RepoDir:     repoDir,
+					RepoRelDir:  filepath.Dir(file),
+					Workspace:   workspace,
+					ProjectName: projectName,
 				})
 				absPaths = append(absPaths, filepath.Join(repoDir, file))
 			}

server/events/pending_plan_finder_test.go

@@ -54,9 +54,26 @@ func TestPendingPlanFinder_Find(t *testing.T) {
 			},
 			[]events.PendingPlan{
 				{
-					RepoDir:    "???/default",
-					RepoRelDir: ".",
-					Workspace:  "default",
+					RepoDir:     "???/default",
+					RepoRelDir:  ".",
+					Workspace:   "default",
+					ProjectName: "projectname",
+				},
+			},
+		},
+		{
+			"root dir project plan with slashes",
+			map[string]interface{}{
+				"default": map[string]interface{}{
+					"project::name-default.tfplan": nil,
+				},
+			},
+			[]events.PendingPlan{
+				{
+					RepoDir:     "???/default",
+					RepoRelDir:  ".",
+					Workspace:   "default",
+					ProjectName: "project/name",
 				},
 			},
 		},

server/events/project_command_builder.go

@@ -206,7 +206,7 @@ func (p *DefaultProjectCommandBuilder) buildApplyAllCommands(ctx *CommandContext
 
 	var cmds []models.ProjectCommandContext
 	for _, plan := range plans {
-		cmd, err := p.buildProjectCommandCtx(ctx, models.ApplyCommand, commentCmd.ProjectName, commentCmd.Flags, plan.RepoDir, plan.RepoRelDir, plan.Workspace, commentCmd.Verbose)
+		cmd, err := p.buildProjectCommandCtx(ctx, models.ApplyCommand, plan.ProjectName, commentCmd.Flags, plan.RepoDir, plan.RepoRelDir, plan.Workspace, commentCmd.Verbose)
 		if err != nil {
 			return nil, errors.Wrapf(err, "building command for dir %q", plan.RepoRelDir)
 		}

server/events/runtime/runtime.go

@@ -4,17 +4,22 @@ package runtime
 
 import (
 	"fmt"
+	"github.com/pkg/errors"
 	"regexp"
+	"strings"
 
-	version "github.com/hashicorp/go-version"
+	"github.com/hashicorp/go-version"
 	"github.com/runatlantis/atlantis/server/events/models"
 	"github.com/runatlantis/atlantis/server/events/terraform"
 	"github.com/runatlantis/atlantis/server/logging"
 )
 
-// lineBeforeRunURL is the line output during a remote operation right before
-// a link to the run url will be output.
-const lineBeforeRunURL = "To view this run in a browser, visit:"
+const (
+	// lineBeforeRunURL is the line output during a remote operation right before
+	// a link to the run url will be output.
+	lineBeforeRunURL     = "To view this run in a browser, visit:"
+	planfileSlashReplace = "::"
+)
 
 // TerraformExec brings the interface from TerraformClient into this package
 // without causing circular imports.
@@ -51,19 +56,28 @@ func MustConstraint(constraint string) version.Constraints {
 	return c
 }
 
-// invalidFilenameChars matches chars that are invalid for linux and windows
-// filenames.
-// From https://www.oreilly.com/library/view/regular-expressions-cookbook/9781449327453/ch08s25.html
-var invalidFilenameChars = regexp.MustCompile(`[\\/:"*?<>|]`)
-
 // GetPlanFilename returns the filename (not the path) of the generated tf plan
 // given a workspace and project name.
 func GetPlanFilename(workspace string, projName string) string {
-	var unescapedFilename string
 	if projName == "" {
-		unescapedFilename = fmt.Sprintf("%s.tfplan", workspace)
-	} else {
-		unescapedFilename = fmt.Sprintf("%s-%s.tfplan", projName, workspace)
+		return fmt.Sprintf("%s.tfplan", workspace)
+	}
+	projName = strings.Replace(projName, "/", planfileSlashReplace, -1)
+	return fmt.Sprintf("%s-%s.tfplan", projName, workspace)
+}
+
+// ProjectNameFromPlanfile returns the project name that a planfile with name
+// filename is for. If filename is for a project without a name then it will
+// return an empty string. workspace is the workspace this project is in.
+func ProjectNameFromPlanfile(workspace string, filename string) (string, error) {
+	r, err := regexp.Compile(fmt.Sprintf(`(.*?)-%s\.tfplan`, workspace))
+	if err != nil {
+		return "", errors.Wrap(err, "compiling project name regex, this is a bug")
+	}
+	projMatch := r.FindAllStringSubmatch(filename, 1)
+	if projMatch == nil {
+		return "", nil
 	}
-	return invalidFilenameChars.ReplaceAllLiteralString(unescapedFilename, "-")
+	rawProjName := projMatch[0][1]
+	return strings.Replace(rawProjName, planfileSlashReplace, "/", -1), nil
 }

server/events/runtime/runtime_test.go

@@ -19,11 +19,6 @@ func TestGetPlanFilename(t *testing.T) {
 			"",
 			"workspace.tfplan",
 		},
-		{
-			"workspace",
-			"",
-			"workspace.tfplan",
-		},
 		{
 			"workspace",
 			"project",
@@ -32,7 +27,7 @@ func TestGetPlanFilename(t *testing.T) {
 		{
 			"workspace",
 			"project/with/slash",
-			"project-with-slash-workspace.tfplan",
+			"project::with::slash-workspace.tfplan",
 		},
 		{
 			"workspace",
@@ -44,10 +39,14 @@ func TestGetPlanFilename(t *testing.T) {
 			"project😀",
 			"project😀-workspace😀.tfplan",
 		},
+		// Previously we replaced invalid chars with -'s, however we now
+		// rely on validation of the atlantis.yaml file to ensure the name's
+		// don't contain chars that need to be url encoded. So now these
+		// chars shouldn't get replaced.
 		{
 			"default",
 			`all.invalid.chars \/"*?<>`,
-			"all.invalid.chars --------default.tfplan",
+			"all.invalid.chars \\::\"*?<>-default.tfplan",
 		},
 	}
 
@@ -57,3 +56,40 @@ func TestGetPlanFilename(t *testing.T) {
 		})
 	}
 }
+
+func TestProjectNameFromPlanfile(t *testing.T) {
+	cases := []struct {
+		workspace string
+		filename  string
+		exp       string
+	}{
+		{
+			"workspace",
+			"workspace.tfplan",
+			"",
+		},
+		{
+			"workspace",
+			"project-workspace.tfplan",
+			"project",
+		},
+		{
+			"workspace",
+			"project-workspace-workspace.tfplan",
+			"project-workspace",
+		},
+		{
+			"workspace",
+			"project::with::slashes::-workspace.tfplan",
+			"project/with/slashes/",
+		},
+	}
+
+	for i, c := range cases {
+		t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
+			act, err := runtime.ProjectNameFromPlanfile(c.workspace, c.filename)
+			Ok(t, err)
+			Equals(t, c.exp, act)
+		})
+	}
+}

server/events/yaml/valid/repo_cfg.go

@@ -13,10 +13,10 @@ type RepoCfg struct {
 	Automerge bool
 }
 
-func (r RepoCfg) FindProjectsByDirWorkspace(dir string, workspace string) []Project {
+func (r RepoCfg) FindProjectsByDirWorkspace(repoRelDir string, workspace string) []Project {
 	var ps []Project
 	for _, p := range r.Projects {
-		if p.Dir == dir && p.Workspace == workspace {
+		if p.Dir == repoRelDir && p.Workspace == workspace {
 			ps = append(ps, p)
 		}
 	}