fix: clone repository before getting working dir

[ghaiszaher]

what

• Fixes Planning a project fails when using new disable autoplan label #3855
• As described in the issue, when running atlantis plan on specific projects, the repository is not cloned first
• This is reproducible when the first command you run in a PR is atlantis plan -d <dir> or atlantis plan -p <project> in either one of the following situations:
  • ATLANTIS_DISABLE_AUTOPLAN = true is used
  • ATLANTIS_DISABLE_AUTOPLAN_LABEL = "no-autoplan" is used and the pr is labeled with no-autoplan
  • A draft PR
  • A PR that was opened while atlantis/webhook is not working
• Solution: clone the PR earlier

why

• It is necessary to make sure the plan command clones the repo at all times and shouldn't expect it to be already cloned (e.g. if the local directory had to be cleared for some reason)

tests

I have done the tests by running the service locally and using a webhook in a private GitHub project:

• sudo rm -r .atlantis
• Restart atlantis
• Comment atlantis plan -d my-dir

Before	After

references

• Planning a project fails when using new disable autoplan label #3855

[nicolst]

I don't know all the inner workings of Atlantis very well, but the comment above the lock for buildAllCommandsByCfg suggests that the workspace should be locked before cloning?

atlantis/server/events/project_command_builder.go

Lines 371 to 375 in 793aafb

	// Need to lock the workspace we're about to clone to.
	workspace := DefaultWorkspace
	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
	if err != nil {

Is there any downside to moving the TryLock up as well?

[ghaiszaher]

I don't know all the inner workings of Atlantis very well, but the comment above the lock for buildAllCommandsByCfg suggests that the workspace should be locked before cloning?

atlantis/server/events/project_command_builder.go

Lines 371 to 375 in 793aafb

	// Need to lock the workspace we're about to clone to.
	workspace := DefaultWorkspace
	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
	if err != nil {

Is there any downside to moving the TryLock up as well?

I missed this, thank you. I don't see any downsides: ab78b2b. I'll test it again and report if I see any issues.

[ghaiszaher]

I'll test it again and report if I see any issues.

No issues

[nicolst]

Tested creating a PR with a no-autoplan label now, seems to work! Thanks for the quick fix!

[nitrocode]

Nice work fixing this. Thank you for contributing the pr.

Would it be possible to add a golang test for this as well to ensure this feature doesn't break in the future?

[ghaiszaher]

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

[ghaiszaher]

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

[ghaiszaher]

Nice work fixing this. Thank you for contributing the pr.

Would it be possible to add a golang test for this as well to ensure this feature doesn't break in the future?

@nitrocode I adjusted the e2e tests here: 9f4d3e1
I added 2 test cases with disabled auto-plan and with no pre-workflow webhooks (because pre-workflow webhooks are already cloning the default workspace).

Please also check my comments in server/events/project_command_builder.go

[nitrocode]

Thanks @ghaiszaher . Approved.

Ill defer to a follow up review before merging.