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

feat: Add logic for project name into lock #4192

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions server/core/db/boltdb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (

var lockBucket = "bucket"
var configBucket = "configBucket"
var project = models.NewProject("owner/repo", "parent/child")
var project = models.NewProject("owner/repo", "parent/child", "")
var workspace = "default"
var pullNum = 1
var lock = models.ProjectLock{
Expand Down Expand Up @@ -155,7 +155,7 @@ func TestListMultipleLocks(t *testing.T) {

for _, r := range repos {
newLock := lock
newLock.Project = models.NewProject(r, "path")
newLock.Project = models.NewProject(r, "path", "")
_, _, err := b.TryLock(newLock)
Ok(t, err)
}
Expand Down Expand Up @@ -207,7 +207,7 @@ func TestLockingExistingLock(t *testing.T) {
t.Log("...succeed if the new project has a different path")
{
newLock := lock
newLock.Project = models.NewProject(project.RepoFullName, "different/path")
newLock.Project = models.NewProject(project.RepoFullName, "different/path", "")
acquired, currLock, err := b.TryLock(newLock)
Ok(t, err)
Equals(t, true, acquired)
Expand All @@ -227,12 +227,24 @@ func TestLockingExistingLock(t *testing.T) {
t.Log("...succeed if the new project has a different repoName")
{
newLock := lock
newLock.Project = models.NewProject("different/repo", project.Path)
newLock.Project = models.NewProject("different/repo", project.Path, "")
acquired, currLock, err := b.TryLock(newLock)
Ok(t, err)
Equals(t, true, acquired)
Equals(t, newLock, currLock)
}
// TODO: How should we handle different name?
/*
t.Log("...succeed if the new project has a different name")
{
newLock := lock
newLock.Project = models.NewProject(project.RepoFullName, project.Path, "different-name")
acquired, currLock, err := b.TryLock(newLock)
Ok(t, err)
Equals(t, true, acquired)
Equals(t, newLock, currLock)
}
*/

t.Log("...not succeed if the new project only has a different pullNum")
{
Expand Down
2 changes: 1 addition & 1 deletion server/core/locking/locking_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
. "github.com/runatlantis/atlantis/testing"
)

var project = models.NewProject("owner/repo", "path")
var project = models.NewProject("owner/repo", "path", "")
var workspace = "workspace"
var pull = models.PullRequest{}
var user = models.User{}
Expand Down
21 changes: 17 additions & 4 deletions server/core/redis/redis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
. "github.com/runatlantis/atlantis/testing"
)

var project = models.NewProject("owner/repo", "parent/child")
var project = models.NewProject("owner/repo", "parent/child", "")
var workspace = "default"
var pullNum = 1
var lock = models.ProjectLock{
Expand Down Expand Up @@ -191,7 +191,7 @@ func TestListMultipleLocks(t *testing.T) {

for _, r := range repos {
newLock := lock
newLock.Project = models.NewProject(r, "path")
newLock.Project = models.NewProject(r, "path", "")
_, _, err := rdb.TryLock(newLock)
Ok(t, err)
}
Expand Down Expand Up @@ -243,7 +243,7 @@ func TestLockingExistingLock(t *testing.T) {
t.Log("...succeed if the new project has a different path")
{
newLock := lock
newLock.Project = models.NewProject(project.RepoFullName, "different/path")
newLock.Project = models.NewProject(project.RepoFullName, "different/path", "")
acquired, currLock, err := rdb.TryLock(newLock)
Ok(t, err)
Equals(t, true, acquired)
Expand All @@ -263,13 +263,26 @@ func TestLockingExistingLock(t *testing.T) {
t.Log("...succeed if the new project has a different repoName")
{
newLock := lock
newLock.Project = models.NewProject("different/repo", project.Path)
newLock.Project = models.NewProject("different/repo", project.Path, "")
acquired, currLock, err := rdb.TryLock(newLock)
Ok(t, err)
Equals(t, true, acquired)
Equals(t, newLock, currLock)
}

// TODO: How should we handle different name?
/*
t.Log("...succeed if the new project has a different name")
{
newLock := lock
newLock.Project = models.NewProject(project.RepoFullName, project.Path, "different-name")
acquired, currLock, err := rdb.TryLock(newLock)
Ok(t, err)
Equals(t, true, acquired)
Equals(t, newLock, currLock)
}
*/

t.Log("...not succeed if the new project only has a different pullNum")
{
newLock := lock
Expand Down
10 changes: 2 additions & 8 deletions server/events/delete_lock_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,7 @@ func (l *DefaultDeleteLockCommand) DeleteLock(id string) (*models.ProjectLock, e
return nil, nil
}

// The locks controller currently has no implementation of Atlantis project names, so this is hardcoded to an empty string.
projectName := ""

removeErr := l.WorkingDir.DeletePlan(lock.Pull.BaseRepo, lock.Pull, lock.Workspace, lock.Project.Path, projectName)
removeErr := l.WorkingDir.DeletePlan(lock.Pull.BaseRepo, lock.Pull, lock.Workspace, lock.Project.Path, lock.Project.ProjectName)
if removeErr != nil {
l.Logger.Warn("Failed to delete plan: %s", removeErr)
return nil, removeErr
Expand All @@ -57,13 +54,10 @@ func (l *DefaultDeleteLockCommand) DeleteLocksByPull(repoFullName string, pullNu
return numLocks, nil
}

// The locks controller currently has no implementation of Atlantis project names, so this is hardcoded to an empty string.
projectName := ""

for i := 0; i < numLocks; i++ {
lock := locks[i]

err := l.WorkingDir.DeletePlan(lock.Pull.BaseRepo, lock.Pull, lock.Workspace, lock.Project.Path, projectName)
err := l.WorkingDir.DeletePlan(lock.Pull.BaseRepo, lock.Pull, lock.Workspace, lock.Project.Path, lock.Project.ProjectName)
if err != nil {
l.Logger.Warn("Failed to delete plan: %s", err)
return numLocks, err
Expand Down
3 changes: 2 additions & 1 deletion server/events/delete_lock_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func TestDeleteLocksByPull_SingleSuccess(t *testing.T) {
pullNum := 2
path := "."
workspace := "default"
projectName := ""
projectName := "projectname"

RegisterMockTestingT(t)
l := lockmocks.NewMockLocker()
Expand All @@ -135,6 +135,7 @@ func TestDeleteLocksByPull_SingleSuccess(t *testing.T) {
Project: models.Project{
Path: path,
RepoFullName: pull.BaseRepo.FullName,
ProjectName: projectName,
},
},
}, nil,
Expand Down
6 changes: 5 additions & 1 deletion server/events/models/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,8 @@ type ProjectLock struct {
// Terraform projects in a single repo we also include Path to the project
// root relative to the repo root.
type Project struct {
// ProjectName of the project
ProjectName string
// RepoFullName is the owner and repo name, ex. "runatlantis/atlantis"
RepoFullName string
// Path to project root in the repo.
Expand All @@ -256,6 +258,7 @@ type Project struct {
}

func (p Project) String() string {
// TODO: Incorporate ProjectName?
return fmt.Sprintf("repofullname=%s path=%s", p.RepoFullName, p.Path)
}

Expand All @@ -271,12 +274,13 @@ type Plan struct {

// NewProject constructs a Project. Use this constructor because it
// sets Path correctly.
func NewProject(repoFullName string, path string) Project {
func NewProject(repoFullName string, path string, projectName string) Project {
path = paths.Clean(path)
if path == "/" {
path = "."
}
return Project{
ProjectName: projectName,
RepoFullName: repoFullName,
Path: path,
}
Expand Down
42 changes: 31 additions & 11 deletions server/events/models/models_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,27 +168,47 @@ func TestProject_String(t *testing.T) {

func TestNewProject(t *testing.T) {
cases := []struct {
path string
expPath string
repo string
path string
name string
expProject models.Project
}{
{
"/",
".",
repo: "foo/bar",
path: "/",
name: "",
expProject: models.Project{
ProjectName: "",
RepoFullName: "foo/bar",
Path: ".",
},
},
{
"./another/path",
"another/path",
repo: "baz/foo",
path: "./another/path",
name: "somename",
expProject: models.Project{
ProjectName: "somename",
RepoFullName: "baz/foo",
Path: "another/path",
},
},
{
".",
".",
repo: "baz/foo",
path: ".",
name: "somename",
expProject: models.Project{
ProjectName: "somename",
RepoFullName: "baz/foo",
Path: ".",
},
},
}

for _, c := range cases {
t.Run(c.path, func(t *testing.T) {
p := models.NewProject("repo/owner", c.path)
Equals(t, c.expPath, p.Path)
t.Run(fmt.Sprintf("%s_%s", c.name, c.path), func(t *testing.T) {
p := models.NewProject(c.repo, c.path, c.name)
Equals(t, c.expProject, p)
})
}
}
Expand Down
10 changes: 5 additions & 5 deletions server/events/project_command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ func (p *DefaultProjectCommandRunner) StateRm(ctx command.ProjectContext) comman

func (p *DefaultProjectCommandRunner) doApprovePolicies(ctx command.ProjectContext) (*models.PolicyCheckResults, string, error) {
// Acquire Atlantis lock for this repo/dir/workspace.
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir), ctx.RepoLocking)
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocking)
if err != nil {
return nil, "", errors.Wrap(err, "acquiring lock")
}
Expand Down Expand Up @@ -417,7 +417,7 @@ func (p *DefaultProjectCommandRunner) doPolicyCheck(ctx command.ProjectContext)
// we will attempt to capture the lock here but fail to get the working directory
// at which point we will unlock again to preserve functionality
// If we fail to capture the lock here (super unlikely) then we error out and the user is forced to replan
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir), ctx.RepoLocking)
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocking)

if err != nil {
return nil, "", errors.Wrap(err, "acquiring lock")
Expand Down Expand Up @@ -536,7 +536,7 @@ func (p *DefaultProjectCommandRunner) doPolicyCheck(ctx command.ProjectContext)

func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*models.PlanSuccess, string, error) {
// Acquire Atlantis lock for this repo/dir/workspace.
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir), ctx.RepoLocking)
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocking)
if err != nil {
return nil, "", errors.Wrap(err, "acquiring lock")
}
Expand Down Expand Up @@ -682,7 +682,7 @@ func (p *DefaultProjectCommandRunner) doImport(ctx command.ProjectContext) (out
}

// Acquire Atlantis lock for this repo/dir/workspace.
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir), ctx.RepoLocking)
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocking)
if err != nil {
return nil, "", errors.Wrap(err, "acquiring lock")
}
Expand Down Expand Up @@ -723,7 +723,7 @@ func (p *DefaultProjectCommandRunner) doStateRm(ctx command.ProjectContext) (out
}

// Acquire Atlantis lock for this repo/dir/workspace.
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir), ctx.RepoLocking)
lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocking)
if err != nil {
return nil, "", errors.Wrap(err, "acquiring lock")
}
Expand Down
6 changes: 5 additions & 1 deletion server/events/project_finder.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,11 @@ func (p *DefaultProjectFinder) DetermineProjects(log logging.SimpleLogging, modi
exists := p.removeNonExistingDirs(uniqueDirs, absRepoDir)

for _, p := range exists {
projects = append(projects, models.NewProject(repoFullName, p))
// It's unclear how we are supposed to determine the project name at this point
// For now, we'll just add the default projectName
// TODO: Add support for non-default projectName
projectName := ""
projects = append(projects, models.NewProject(repoFullName, p, projectName))
jamengual marked this conversation as resolved.
Show resolved Hide resolved
}
log.Info("there are %d modified project(s) at path(s): %v",
len(projects), strings.Join(exists, ", "))
Expand Down
29 changes: 20 additions & 9 deletions server/events/pull_closed_executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,28 @@ func TestCleanUpPullComments(t *testing.T) {
"single lock, empty path",
[]models.ProjectLock{
{
Project: models.NewProject("owner/repo", ""),
Project: models.NewProject("owner/repo", "", ""),
Workspace: "default",
},
},
"- dir: `.` workspace: `default`",
},
{
"single lock, named project",
[]models.ProjectLock{
{
Project: models.NewProject("owner/repo", "", "projectname"),
Workspace: "default",
},
},
// TODO: Should project name be included in output?
"- dir: `.` workspace: `default`",
},
{
"single lock, non-empty path",
[]models.ProjectLock{
{
Project: models.NewProject("owner/repo", "path"),
Project: models.NewProject("owner/repo", "path", ""),
Workspace: "default",
},
},
Expand All @@ -127,11 +138,11 @@ func TestCleanUpPullComments(t *testing.T) {
"single path, multiple workspaces",
[]models.ProjectLock{
{
Project: models.NewProject("owner/repo", "path"),
Project: models.NewProject("owner/repo", "path", ""),
Workspace: "workspace1",
},
{
Project: models.NewProject("owner/repo", "path"),
Project: models.NewProject("owner/repo", "path", ""),
Workspace: "workspace2",
},
},
Expand All @@ -141,19 +152,19 @@ func TestCleanUpPullComments(t *testing.T) {
"multiple paths, multiple workspaces",
[]models.ProjectLock{
{
Project: models.NewProject("owner/repo", "path"),
Project: models.NewProject("owner/repo", "path", ""),
Workspace: "workspace1",
},
{
Project: models.NewProject("owner/repo", "path"),
Project: models.NewProject("owner/repo", "path", ""),
Workspace: "workspace2",
},
{
Project: models.NewProject("owner/repo", "path2"),
Project: models.NewProject("owner/repo", "path2", ""),
Workspace: "workspace1",
},
{
Project: models.NewProject("owner/repo", "path2"),
Project: models.NewProject("owner/repo", "path2", ""),
Workspace: "workspace2",
},
},
Expand Down Expand Up @@ -260,7 +271,7 @@ func TestCleanUpLogStreaming(t *testing.T) {

locks := []models.ProjectLock{
{
Project: models.NewProject(testdata.GithubRepo.FullName, ""),
Project: models.NewProject(testdata.GithubRepo.FullName, "", ""),
Workspace: "default",
},
}
Expand Down
Loading