fix: clone repository before getting working dir (#3867)

* fix: clone repository before getting working dir

* unrelated change

* move TryLock up

* fix workspace and add e2e tests

* fix description

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>

server/controllers/events/events_controller_e2e_test.go

@@ -93,6 +93,10 @@ func TestGitHubWorkflow(t *testing.T) {
 		ApplyLock bool
 		// AllowCommands flag what kind of atlantis commands are available.
 		AllowCommands []command.Name
+		// DisableAutoplan flag disable auto plans when any pull request is opened.
+		DisableAutoplan bool
+		// DisablePreWorkflowHooks if set to true, pre-workflow hooks will be disabled
+		DisablePreWorkflowHooks bool
 		// ExpAutomerge is true if we expect Atlantis to automerge.
 		ExpAutomerge bool
 		// ExpAutoplan is true if we expect Atlantis to autoplan.
@@ -247,6 +251,25 @@ func TestGitHubWorkflow(t *testing.T) {
 				{"exp-output-merge.txt"},
 			},
 		},
+		{
+			Description:             "simple with atlantis.yaml - autoplan disabled",
+			RepoDir:                 "simple-yaml",
+			ModifiedFiles:           []string{"main.tf"},
+			DisableAutoplan:         true,
+			DisablePreWorkflowHooks: true,
+			ExpAutoplan:             false,
+			Comments: []string{
+				"atlantis plan -w staging",
+				"atlantis plan -w default",
+				"atlantis apply -w staging",
+			},
+			ExpReplies: [][]string{
+				{"exp-output-plan-staging.txt"},
+				{"exp-output-plan-default.txt"},
+				{"exp-output-apply-staging.txt"},
+				{"exp-output-merge.txt"},
+			},
+		},
 		{
 			Description:   "simple with atlantis.yaml and apply all",
 			RepoDir:       "simple-yaml",
@@ -293,6 +316,23 @@ func TestGitHubWorkflow(t *testing.T) {
 				{"exp-output-merge-only-staging.txt"},
 			},
 		},
+		{
+			Description:             "modules staging only - autoplan disabled",
+			RepoDir:                 "modules",
+			ModifiedFiles:           []string{"staging/main.tf"},
+			DisableAutoplan:         true,
+			DisablePreWorkflowHooks: true,
+			ExpAutoplan:             false,
+			Comments: []string{
+				"atlantis plan -d staging",
+				"atlantis apply -d staging",
+			},
+			ExpReplies: [][]string{
+				{"exp-output-plan-staging.txt"},
+				{"exp-output-apply-staging.txt"},
+				{"exp-output-merge-only-staging.txt"},
+			},
+		},
 		{
 			Description:   "modules modules only",
 			RepoDir:       "modules",
@@ -590,7 +630,12 @@ func TestGitHubWorkflow(t *testing.T) {
 			userConfig = server.UserConfig{}
 			userConfig.DisableApply = c.DisableApply
 
-			opt := setupOption{repoConfigFile: c.RepoConfigFile, allowCommands: c.AllowCommands}
+			opt := setupOption{
+				repoConfigFile:          c.RepoConfigFile,
+				allowCommands:           c.AllowCommands,
+				disableAutoplan:         c.DisableAutoplan,
+				disablePreWorkflowHooks: c.DisablePreWorkflowHooks,
+			}
 			ctrl, vcsClient, githubGetter, atlantisWorkspace := setupE2E(t, c.RepoDir, opt)
 			// Set the repo to be cloned through the testing backdoor.
 			repoDir, headSHA := initializeRepo(t, c.RepoDir)
@@ -630,10 +675,16 @@ func TestGitHubWorkflow(t *testing.T) {
 			ctrl.Post(w, pullClosedReq)
 			ResponseContains(t, w, 200, "Pull request cleaned successfully")
 
-			expNumHooks := len(c.Comments) + 1 - c.ExpParseFailedCount
+			expNumHooks := len(c.Comments) - c.ExpParseFailedCount
+			// if auto plan is disabled, hooks will not be called on pull request opened event
+			if !c.DisableAutoplan {
+				expNumHooks++
+			}
 			// Let's verify the pre-workflow hook was called for each comment including the pull request opened event
-			mockPreWorkflowHookRunner.VerifyWasCalled(Times(expNumHooks)).Run(Any[models.WorkflowHookCommandContext](),
-				Eq("some dummy command"), Any[string](), Any[string](), Any[string]())
+			if !c.DisablePreWorkflowHooks {
+				mockPreWorkflowHookRunner.VerifyWasCalled(Times(expNumHooks)).Run(Any[models.WorkflowHookCommandContext](),
+					Eq("some dummy command"), Any[string](), Any[string](), Any[string]())
+			}
 			// Let's verify the post-workflow hook was called for each comment including the pull request opened event
 			mockPostWorkflowHookRunner.VerifyWasCalled(Times(expNumHooks)).Run(Any[models.WorkflowHookCommandContext](),
 				Eq("some post dummy command"), Any[string](), Any[string](), Any[string]())
@@ -1212,8 +1263,10 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) {
 }
 
 type setupOption struct {
-	repoConfigFile string
-	allowCommands  []command.Name
+	repoConfigFile          string
+	allowCommands           []command.Name
+	disableAutoplan         bool
+	disablePreWorkflowHooks bool
 }
 
 func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers.VCSEventsController, *vcsmocks.MockClient, *mocks.MockGithubPullGetter, *events.FileWorkspace) {
@@ -1266,22 +1319,26 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers
 		TestingOverrideHeadCloneURL: "override-me",
 		Logger:                      logger,
 	}
+	var preWorkflowHooks []*valid.WorkflowHook
+	if !opt.disablePreWorkflowHooks {
+		preWorkflowHooks = []*valid.WorkflowHook{
+			{
+				StepName:   "global_hook",
+				RunCommand: "some dummy command",
+			},
+		}
+	}
 
 	defaultTFVersion := terraformClient.DefaultVersion()
 	locker := events.NewDefaultWorkingDirLocker()
 	parser := &config.ParserValidator{}
 
 	globalCfgArgs := valid.GlobalCfgArgs{
-		RepoConfigFile: opt.repoConfigFile,
-		AllowRepoCfg:   true,
-		MergeableReq:   false,
-		ApprovedReq:    false,
-		PreWorkflowHooks: []*valid.WorkflowHook{
-			{
-				StepName:   "global_hook",
-				RunCommand: "some dummy command",
-			},
-		},
+		RepoConfigFile:   opt.repoConfigFile,
+		AllowRepoCfg:     true,
+		MergeableReq:     false,
+		ApprovedReq:      false,
+		PreWorkflowHooks: preWorkflowHooks,
 		PostWorkflowHooks: []*valid.WorkflowHook{
 			{
 				StepName:   "global_hook",
@@ -1539,6 +1596,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers
 		PreWorkflowHooksCommandRunner:  preWorkflowHooksCommandRunner,
 		PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner,
 		PullStatusFetcher:              backend,
+		DisableAutoplan:                opt.disableAutoplan,
 	}
 
 	repoAllowlistChecker, err := events.NewRepoAllowlistChecker("*")

server/controllers/events/testdata/test-repos/simple-yaml/exp-output-plan-default.txt

@@ -0,0 +1,41 @@
+Ran Plan for dir: `.` workspace: `default`
+
+<details><summary>Show Output</summary>
+
+```diff
+preinit
+
+
+Terraform used the selected providers to generate the following execution
+plan. Resource actions are indicated with the following symbols:
++ create
+
+Terraform will perform the following actions:
+
+  # null_resource.simple[0] will be created
++ resource "null_resource" "simple" {
+      + id = (known after apply)
+    }
+
+Plan: 1 to add, 0 to change, 0 to destroy.
+
+Changes to Outputs:
++ var       = "fromconfig"
++ workspace = "default"
+
+postplan
+```
+
+* :arrow_forward: To **apply** this plan, comment:
+    * `atlantis apply -d .`
+* :put_litter_in_its_place: To **delete** this plan click [here](lock-url)
+* :repeat: To **plan** this project again, comment:
+    * `atlantis plan -d .`
+</details>
+Plan: 1 to add, 0 to change, 0 to destroy.
+
+---
+* :fast_forward: To **apply** all unapplied plans from this pull request, comment:
+    * `atlantis apply`
+* :put_litter_in_its_place: To delete all plans and locks for the PR, comment:
+    * `atlantis unlock`

server/controllers/events/testdata/test-repos/simple-yaml/exp-output-plan-staging.txt

@@ -0,0 +1,36 @@
+Ran Plan for dir: `.` workspace: `staging`
+
+<details><summary>Show Output</summary>
+
+```diff
+Terraform used the selected providers to generate the following execution
+plan. Resource actions are indicated with the following symbols:
++ create
+
+Terraform will perform the following actions:
+
+  # null_resource.simple[0] will be created
++ resource "null_resource" "simple" {
+      + id = (known after apply)
+    }
+
+Plan: 1 to add, 0 to change, 0 to destroy.
+
+Changes to Outputs:
++ var       = "fromfile"
++ workspace = "staging"
+```
+
+* :arrow_forward: To **apply** this plan, comment:
+    * `atlantis apply -w staging`
+* :put_litter_in_its_place: To **delete** this plan click [here](lock-url)
+* :repeat: To **plan** this project again, comment:
+    * `atlantis plan -w staging`
+</details>
+Plan: 1 to add, 0 to change, 0 to destroy.
+
+---
+* :fast_forward: To **apply** all unapplied plans from this pull request, comment:
+    * `atlantis apply`
+* :put_litter_in_its_place: To delete all plans and locks for the PR, comment:
+    * `atlantis unlock`

server/events/project_command_builder.go

@@ -566,6 +566,19 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *command.Cont
 
 	var pcc []command.ProjectContext
 
+	ctx.Log.Debug("building plan command")
+	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
+	if err != nil {
+		return pcc, err
+	}
+	defer unlockFn()
+
+	ctx.Log.Debug("cloning repository")
+	_, _, err = p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, DefaultWorkspace)
+	if err != nil {
+		return pcc, err
+	}
+
 	// use the default repository workspace because it is the only one guaranteed to have an atlantis.yaml,
 	// other workspaces will not have the file if they are using pre_workflow_hooks to generate it dynamically
 	defaultRepoDir, err := p.WorkingDir.GetWorkingDir(ctx.Pull.BaseRepo, ctx.Pull, DefaultWorkspace)
@@ -637,17 +650,12 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *command.Cont
 		}
 	}
 
-	ctx.Log.Debug("building plan command")
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
-	if err != nil {
-		return pcc, err
-	}
-	defer unlockFn()
-
-	ctx.Log.Debug("cloning repository")
-	_, _, err = p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, workspace)
-	if err != nil {
-		return pcc, err
+	if DefaultWorkspace != workspace {
+		ctx.Log.Debug("cloning repository with workspace %s", workspace)
+		_, _, err = p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, workspace)
+		if err != nil {
+			return pcc, err
+		}
 	}
 
 	repoRelDir := DefaultRepoRelDir