diff --git a/locking/boltdb/boltdb.go b/locking/boltdb/boltdb.go index 766b60aebb..41c1352d66 100644 --- a/locking/boltdb/boltdb.go +++ b/locking/boltdb/boltdb.go @@ -48,17 +48,11 @@ func NewWithDB(db *bolt.DB, bucket string) (*Backend, error) { return &Backend{db, []byte(bucket)}, nil } -func (b *Backend) TryLock(project models.Project, env string, pullNum int) (bool, int, error) { +func (b *Backend) TryLock(newLock models.ProjectLock) (bool, models.ProjectLock, error) { // return variables var lockAcquired bool - var lockingPullNum int - key := b.key(project, env) - newLock := models.ProjectLock{ - PullNum: pullNum, - Project: project, - Time: time.Now(), - Env: env, - } + var currLock models.ProjectLock + key := b.key(newLock.Project, newLock.Env) newLockSerialized, _ := json.Marshal(newLock) transactionErr := b.db.Update(func(tx *bolt.Tx) error { bucket := tx.Bucket(b.bucket) @@ -68,25 +62,23 @@ func (b *Backend) TryLock(project models.Project, env string, pullNum int) (bool if currLockSerialized == nil { bucket.Put([]byte(key), newLockSerialized) // not a readonly bucketName so okay to ignore error lockAcquired = true - lockingPullNum = pullNum + currLock = newLock return nil } // otherwise the lock fails, return to caller the run that's holding the lock - var currLock models.ProjectLock if err := json.Unmarshal(currLockSerialized, &currLock); err != nil { return errors.Wrap(err, "failed to deserialize current lock") } lockAcquired = false - lockingPullNum = currLock.PullNum return nil }) if transactionErr != nil { - return false, lockingPullNum, errors.Wrap(transactionErr, "DB transaction failed") + return false, currLock, errors.Wrap(transactionErr, "DB transaction failed") } - return lockAcquired, lockingPullNum, nil + return lockAcquired, currLock, nil } func (b Backend) Unlock(p models.Project, env string) error { @@ -137,7 +129,7 @@ func (b Backend) UnlockByPull(repoFullName string, pullNum int) error { if err := json.Unmarshal(v, &lock); err != nil { return errors.Wrapf(err, "failed to deserialize lock at key %q", string(k)) } - if lock.PullNum == pullNum { + if lock.Pull.Num == pullNum { locks = append(locks, lock) } } diff --git a/locking/boltdb/boltdb_test.go b/locking/boltdb/boltdb_test.go index 1efce5955e..972cd44d2f 100644 --- a/locking/boltdb/boltdb_test.go +++ b/locking/boltdb/boltdb_test.go @@ -11,12 +11,24 @@ import ( "github.com/hootsuite/atlantis/locking/boltdb" "github.com/hootsuite/atlantis/models" + "time" ) var lockBucket = "bucket" var project = models.NewProject("owner/repo", "parent/child") var env = "default" var pullNum = 1 +var lock = models.ProjectLock{ + Pull: models.PullRequest{ + Num: pullNum, + }, + User: models.User{ + Username: "lkysow", + }, + Env: env, + Project: project, + Time: time.Now(), +} func TestListNoLocks(t *testing.T) { t.Log("listing locks when there are none should return an empty list") @@ -31,7 +43,7 @@ func TestListOneLock(t *testing.T) { t.Log("listing locks when there is one should return it") db, b := newTestDB() defer cleanupDB(db) - _, _, err := b.TryLock(project, env, pullNum) + _, _, err := b.TryLock(lock) Ok(t, err) ls, err := b.List() Ok(t, err) @@ -52,7 +64,9 @@ func TestListMultipleLocks(t *testing.T) { } for _, r := range repos { - _, _, err := b.TryLock(models.NewProject(r, "path"), env, pullNum) + newLock := lock + newLock.Project = models.NewProject(r, "path") + _, _, err := b.TryLock(newLock) Ok(t, err) } ls, err := b.List() @@ -73,7 +87,7 @@ func TestListAddRemove(t *testing.T) { t.Log("listing after adding and removing should return none") db, b := newTestDB() defer cleanupDB(db) - _, _, err := b.TryLock(project, env, pullNum) + _, _, err := b.TryLock(lock) Ok(t, err) b.Unlock(project, env) @@ -86,50 +100,57 @@ func TestLockingNoLocks(t *testing.T) { t.Log("with no locks yet, lock should succeed") db, b := newTestDB() defer cleanupDB(db) - acquired, lockingPullNum, err := b.TryLock(project, env, pullNum) + acquired, currLock, err := b.TryLock(lock) Ok(t, err) Equals(t, true, acquired) - Equals(t, pullNum, lockingPullNum) + Equals(t, lock, currLock) } func TestLockingExistingLock(t *testing.T) { t.Log("if there is an existing lock, lock should...") db, b := newTestDB() defer cleanupDB(db) - _, _, err := b.TryLock(project, env, pullNum) + _, _, err := b.TryLock(lock) Ok(t, err) t.Log("...succeed if the new project has a different path") { - acquired, lockingPull, err := b.TryLock(models.NewProject(project.RepoFullName, "different/path"), env, pullNum) + newLock := lock + newLock.Project = models.NewProject(project.RepoFullName, "different/path") + acquired, currLock, err := b.TryLock(newLock) Ok(t, err) Equals(t, true, acquired) - Equals(t, pullNum, lockingPull) + Equals(t, pullNum, currLock.Pull.Num) } t.Log("...succeed if the new project has a different environment") { - acquired, lockingPull, err := b.TryLock(project, "different-env", pullNum) + newLock := lock + newLock.Env = "different-env" + acquired, currLock, err := b.TryLock(newLock) Ok(t, err) Equals(t, true, acquired) - Equals(t, pullNum, lockingPull) + Equals(t, newLock, currLock) } t.Log("...succeed if the new project has a different repoName") { - acquired, lockingPull, err := b.TryLock(models.NewProject("different/repo", project.Path), env, pullNum) + newLock := lock + newLock.Project = models.NewProject("different/repo", project.Path) + acquired, currLock, err := b.TryLock(newLock) Ok(t, err) Equals(t, true, acquired) - Equals(t, pullNum, lockingPull) + Equals(t, newLock, currLock) } t.Log("...not succeed if the new project only has a different pullNum") { - newPull := pullNum + 1 - acquired, lockingPull, err := b.TryLock(project, env, newPull) + newLock := lock + newLock.Pull.Num = lock.Pull.Num + 1 + acquired, currLock, err := b.TryLock(newLock) Ok(t, err) Equals(t, false, acquired) - Equals(t, lockingPull, pullNum) + Equals(t, currLock.Pull.Num, pullNum) } } @@ -146,7 +167,7 @@ func TestUnlocking(t *testing.T) { db, b := newTestDB() defer cleanupDB(db) - b.TryLock(project, env, pullNum) + b.TryLock(lock) Ok(t, b.Unlock(project, env)) // should be no locks listed @@ -155,11 +176,12 @@ func TestUnlocking(t *testing.T) { Equals(t, 0, len(ls)) // should be able to re-lock that repo with a new pull num - newPull := pullNum + 1 - acquired, lockingPull, err := b.TryLock(project, env, newPull) + newLock := lock + newLock.Pull.Num = lock.Pull.Num + 1 + acquired, currLock, err := b.TryLock(newLock) Ok(t, err) Equals(t, true, acquired) - Equals(t, newPull, lockingPull) + Equals(t, newLock, currLock) } func TestUnlockingMultiple(t *testing.T) { @@ -167,23 +189,24 @@ func TestUnlockingMultiple(t *testing.T) { db, b := newTestDB() defer cleanupDB(db) - b.TryLock(project, env, pullNum) + b.TryLock(lock) - new := project - new.RepoFullName = "new/repo" - b.TryLock(project, env, pullNum) + new := lock + new.Project.RepoFullName = "new/repo" + b.TryLock(new) - new2 := project - new2.Path = "new/path" - b.TryLock(project, env, pullNum) + new2 := lock + new2.Project.Path = "new/path" + b.TryLock(new2) - new3Env := "new-env" - b.TryLock(project, new3Env, pullNum) + new3 := lock + new3.Env = "new-env" + b.TryLock(new3) // now try and unlock them - Ok(t, b.Unlock(project, new3Env)) - Ok(t, b.Unlock(new2, env)) - Ok(t, b.Unlock(new, env)) + Ok(t, b.Unlock(new3.Project, new3.Env)) + Ok(t, b.Unlock(new2.Project, env)) + Ok(t, b.Unlock(new.Project, env)) Ok(t, b.Unlock(project, env)) // should be none left @@ -205,7 +228,7 @@ func TestUnlockByPullOne(t *testing.T) { t.Log("with one lock, UnlockByPull should...") db, b := newTestDB() defer cleanupDB(db) - _, _, err := b.TryLock(project, env, pullNum) + _, _, err := b.TryLock(lock) Ok(t, err) t.Log("...delete nothing when its the same repo but a different pull") @@ -238,7 +261,7 @@ func TestUnlockByPullAfterUnlock(t *testing.T) { t.Log("after locking and unlocking, UnlockByPull should be successful") db, b := newTestDB() defer cleanupDB(db) - _, _, err := b.TryLock(project, env, pullNum) + _, _, err := b.TryLock(lock) Ok(t, err) Ok(t, b.Unlock(project, env)) @@ -253,15 +276,17 @@ func TestUnlockByPullMatching(t *testing.T) { t.Log("UnlockByPull should delete all locks in that repo and pull num") db, b := newTestDB() defer cleanupDB(db) - _, _, err := b.TryLock(project, env, pullNum) + _, _, err := b.TryLock(lock) Ok(t, err) // add additional locks with the same repo and pull num but different paths/envs - new := project - new.Path = "dif/path" - _, _, err = b.TryLock(new, env, pullNum) + new := lock + new.Project.Path = "dif/path" + _, _, err = b.TryLock(new) Ok(t, err) - _, _, err = b.TryLock(project, "new-env", pullNum) + new2 := lock + new2.Env = "new-env" + _, _, err = b.TryLock(new2) Ok(t, err) // there should now be 3 diff --git a/locking/dynamodb/dynamodb.go b/locking/dynamodb/dynamodb.go index a35ef3033d..39b65b4805 100644 --- a/locking/dynamodb/dynamodb.go +++ b/locking/dynamodb/dynamodb.go @@ -18,15 +18,21 @@ type Backend struct { LockTable string } -// dynamoLock duplicates the fields of models.ProjectLock and adds LocksKey -// so everything is a top-level field for serialization and then querying -// in DynamodB and also so any changes to models.ProjectLock won't affect -// how we're storing our data or will at least cause a compile error +// dynamoLock duplicates the fields of some models and adds LocksKey. +// This is so everything is a top-level field for serialization and querying +// and also so any changes to models won't affect +// how we're storing our data (or will at least cause a compile error) type dynamoLock struct { LockKey string RepoFullName string Path string - PullNum int + PullNum int + PullHeadCommit string + PullBaseCommit string + PullURL string + PullBranch string + PullAuthor string + UserUsername string Env string Time time.Time } @@ -42,19 +48,13 @@ func (b Backend) key(project models.Project, env string) string { return fmt.Sprintf("%s/%s/%s", project.RepoFullName, project.Path, env) } -func (b Backend) TryLock(project models.Project, env string, pullNum int) (bool, int, error) { - key := b.key(project, env) - newDynamoLock := dynamoLock{ - LockKey: key, - RepoFullName: project.RepoFullName, - Path: project.Path, - PullNum: pullNum, - Env: env, - Time: time.Now(), - } +func (b Backend) TryLock(newLock models.ProjectLock) (bool, models.ProjectLock, error) { + var currLock models.ProjectLock + key := b.key(newLock.Project, newLock.Env) + newDynamoLock := b.toDynamo(key, newLock) newLockSerialized, err := dynamodbattribute.MarshalMap(newDynamoLock) if err != nil { - return false, 0, errors.Wrap(err, "serializing") + return false, currLock, errors.Wrap(err, "serializing") } // check if there is an existing lock @@ -69,16 +69,16 @@ func (b Backend) TryLock(project models.Project, env string, pullNum int) (bool, } item, err := b.DB.GetItem(getItemParams) if err != nil { - return false, 0, errors.Wrap(err, "checking if lock exists") + return false, currLock, errors.Wrap(err, "checking if lock exists") } // if there is already a lock then we can't acquire a lock. Return the existing lock - var currLock dynamoLock + var currDynamoLock dynamoLock if len(item.Item) != 0 { - if err := dynamodbattribute.UnmarshalMap(item.Item, &currLock); err != nil { - return false, 0, errors.Wrap(err, "found an existing lock at that key but it could not be deserialized. We suggest manually deleting this key from DynamoDB") + if err := dynamodbattribute.UnmarshalMap(item.Item, &currDynamoLock); err != nil { + return false, currLock, errors.Wrap(err, "found an existing lock at that key but it could not be deserialized. We suggest manually deleting this key from DynamoDB") } - return false, currLock.PullNum, nil + return false, b.fromDynamo(currDynamoLock), nil } // else we should be able to lock @@ -90,9 +90,9 @@ func (b Backend) TryLock(project models.Project, env string, pullNum int) (bool, ConditionExpression: aws.String("attribute_not_exists(LockKey)"), } if _, err := b.DB.PutItem(putItem); err != nil { - return false, 0, errors.Wrap(err, "writing lock") + return false, currLock, errors.Wrap(err, "writing lock") } - return true, pullNum, nil + return true, newLock, nil } func (b Backend) Unlock(project models.Project, env string) error { @@ -120,12 +120,7 @@ func (b Backend) List() ([]models.ProjectLock, error) { return false } for _, lock := range dynamoLocks { - locks = append(locks, models.ProjectLock{ - PullNum: lock.PullNum, - Project: models.NewProject(lock.RepoFullName, lock.Path), - Env: lock.Env, - Time: lock.Time, - }) + locks = append(locks, b.fromDynamo(lock)) } return lastPage }) @@ -175,3 +170,42 @@ func (b Backend) UnlockByPull(repoFullName string, pullNum int) error { } return nil } + +func (b Backend) toDynamo(key string, l models.ProjectLock) dynamoLock { + return dynamoLock{ + LockKey: key, + RepoFullName: l.Project.RepoFullName, + Path: l.Project.Path, + PullNum: l.Pull.Num, + PullHeadCommit: l.Pull.HeadCommit, + PullBaseCommit: l.Pull.BaseCommit, + PullURL: l.Pull.URL, + PullBranch: l.Pull.Branch, + PullAuthor: l.Pull.Author, + UserUsername: l.User.Username, + Env: l.Env, + Time: time.Now(), + } +} + +func (b Backend) fromDynamo(d dynamoLock) models.ProjectLock { + return models.ProjectLock{ + Pull: models.PullRequest{ + Author: d.PullAuthor, + Branch: d.PullBranch, + URL: d.PullURL, + BaseCommit: d.PullBaseCommit, + HeadCommit: d.PullHeadCommit, + Num: d.PullNum, + }, + User: models.User{ + Username: d.UserUsername, + }, + Project: models.Project{ + RepoFullName: d.RepoFullName, + Path: d.Path, + }, + Time: d.Time, + Env: d.Env, + } +} diff --git a/locking/locking.go b/locking/locking.go index 43152d6634..41121b9c51 100644 --- a/locking/locking.go +++ b/locking/locking.go @@ -5,10 +5,11 @@ import ( "fmt" "github.com/hootsuite/atlantis/models" "regexp" + "time" ) type Backend interface { - TryLock(project models.Project, env string, pullNum int) (bool, int, error) + TryLock(lock models.ProjectLock) (bool, models.ProjectLock, error) Unlock(project models.Project, env string) error List() ([]models.ProjectLock, error) UnlockByPull(repoFullName string, pullNum int) error @@ -16,7 +17,7 @@ type Backend interface { type TryLockResponse struct { LockAcquired bool - LockingPullNum int + CurrLock models.ProjectLock LockKey string } @@ -33,12 +34,19 @@ func NewClient(backend Backend) *Client { // keyRegex matches and captures {repoFullName}/{path}/{env} where path can have multiple /'s in it var keyRegex = regexp.MustCompile(`^(.*?\/.*?)\/(.*)\/(.*)$`) -func (c *Client) TryLock(p models.Project, env string, pullNum int) (TryLockResponse, error) { - lockAcquired, lockingPullNum, err := c.backend.TryLock(p, env, pullNum) +func (c *Client) TryLock(p models.Project, env string, pull models.PullRequest, user models.User) (TryLockResponse, error) { + lock := models.ProjectLock{ + Env: env, + Time: time.Now(), + Project: p, + User: user, + Pull: pull, + } + lockAcquired, currLock, err := c.backend.TryLock(lock) if err != nil { return TryLockResponse{}, err } - return TryLockResponse{lockAcquired, lockingPullNum, c.key(p, env)}, nil + return TryLockResponse{lockAcquired, currLock, c.key(p, env)}, nil } func (c *Client) Unlock(key string) error { diff --git a/models/models.go b/models/models.go index 5f0cefb0b2..151cfb5990 100644 --- a/models/models.go +++ b/models/models.go @@ -16,7 +16,7 @@ type PullRequest struct { Num int HeadCommit string BaseCommit string - Link string + URL string Branch string Author string } @@ -27,7 +27,8 @@ type User struct { type ProjectLock struct { Project Project - PullNum int + Pull PullRequest + User User Env string Time time.Time } diff --git a/server/apply_executor.go b/server/apply_executor.go index e26a2c1d81..a196ffd6f3 100644 --- a/server/apply_executor.go +++ b/server/apply_executor.go @@ -156,17 +156,17 @@ func (a *ApplyExecutor) apply(ctx *CommandContext, repoDir string, plan plan.Pla tfEnv = "default" } - lockAttempt, err := a.lockingClient.TryLock(plan.Project, tfEnv, ctx.Pull.Num) + lockAttempt, err := a.lockingClient.TryLock(plan.Project, tfEnv, ctx.Pull, ctx.User) if err != nil { return PathResult{ Status: Error, Result: GeneralError{fmt.Errorf("failed to acquire lock: %s", err)}, } } - if lockAttempt.LockAcquired != true && lockAttempt.LockingPullNum != ctx.Pull.Num { + if lockAttempt.LockAcquired != true && lockAttempt.CurrLock.Pull.Num != ctx.Pull.Num { return PathResult{ Status: Error, - Result: GeneralError{fmt.Errorf("failed to acquire lock: lock held by pull request #%d", lockAttempt.LockingPullNum)}, + Result: GeneralError{fmt.Errorf("failed to acquire lock: lock held by pull request #%d", lockAttempt.CurrLock.Pull.Num)}, } } } diff --git a/server/event_parser.go b/server/event_parser.go index 4d4515246c..f5173f4b4e 100644 --- a/server/event_parser.go +++ b/server/event_parser.go @@ -106,8 +106,8 @@ func (e *EventParser) ExtractPullData(pull *github.PullRequest) (models.PullRequ if base == "" { return pullModel, errors.New("base.sha is null") } - pullLink := pull.GetHTMLURL() - if pullLink == "" { + url := pull.GetHTMLURL() + if url == "" { return pullModel, errors.New("html_url is null") } branch := pull.Head.GetRef() @@ -127,7 +127,7 @@ func (e *EventParser) ExtractPullData(pull *github.PullRequest) (models.PullRequ Author: authorUsername, Branch: branch, HeadCommit: commit, - Link: pullLink, + URL: url, Num: num, }, nil } diff --git a/server/plan_executor.go b/server/plan_executor.go index 429b5c8981..e9ae5fde8c 100644 --- a/server/plan_executor.go +++ b/server/plan_executor.go @@ -211,7 +211,7 @@ func (p *PlanExecutor) plan( if tfEnv == "" { tfEnv = "default" } - lockAttempt, err := p.lockingClient.TryLock(project, ctx.Command.environment, ctx.Pull.Num) + lockAttempt, err := p.lockingClient.TryLock(project, ctx.Command.environment, ctx.Pull, ctx.User) if err != nil { return PathResult{ Status: Failure, @@ -220,10 +220,10 @@ func (p *PlanExecutor) plan( } // the run is locked unless the locking run is the same pull id as this run - if lockAttempt.LockAcquired == false && lockAttempt.LockingPullNum != ctx.Pull.Num { + if lockAttempt.LockAcquired == false && lockAttempt.CurrLock.Pull.Num != ctx.Pull.Num { return PathResult{ Status: Failure, - Result: RunLockedFailure{lockAttempt.LockingPullNum}, + Result: RunLockedFailure{lockAttempt.CurrLock.Pull.Num}, } } diff --git a/server/server.go b/server/server.go index 9bb62f8491..309f67d027 100644 --- a/server/server.go +++ b/server/server.go @@ -257,7 +257,7 @@ func (s *Server) index(w http.ResponseWriter, r *http.Request) { results = append(results, lock{ UnlockURL: u.String(), RepoFullName: v.Project.RepoFullName, - PullNum: v.PullNum, + PullNum: v.Pull.Num, Time: v.Time, }) }