Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: clone repository before getting working dir #3867

Merged
merged 11 commits into from
Dec 12, 2023
90 changes: 74 additions & 16 deletions server/controllers/events/events_controller_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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]())
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -1538,6 +1595,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("*")
Expand Down
Original file line number Diff line number Diff line change
@@ -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`
Original file line number Diff line number Diff line change
@@ -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`
30 changes: 19 additions & 11 deletions server/events/project_command_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary to clone DefaultWorkspace at this point and not workspace

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)
Expand Down Expand Up @@ -580,17 +593,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
}
Comment on lines +596 to +601
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block is unnecessary because this call happens later anyway, but I kept it just in case it's needed for cases that I didn't notice

}

repoRelDir := DefaultRepoRelDir
Expand Down