Merge pull request #140 from runatlantis/mohit

Comment back on pull request when lock is discarded from Atlantis UI

server/events/command_handler.go

@@ -86,7 +86,7 @@ func (c *CommandHandler) ExecuteCommand(baseRepo models.Repo, headRepo models.Re
 	case models.Github:
 		pull, headRepo, err = c.getGithubData(baseRepo, pullNum)
 	case models.Gitlab:
-		pull, err = c.getGitlabData(baseRepo.FullName, pullNum)
+		pull, err = c.getGitlabData(baseRepo, pullNum)
 	default:
 		err = errors.New("Unknown VCS type, this is a bug!")
 	}
@@ -120,15 +120,15 @@ func (c *CommandHandler) getGithubData(baseRepo models.Repo, pullNum int) (model
 	return pull, repo, nil
 }
 
-func (c *CommandHandler) getGitlabData(repoFullName string, pullNum int) (models.PullRequest, error) {
+func (c *CommandHandler) getGitlabData(baseRepo models.Repo, pullNum int) (models.PullRequest, error) {
 	if c.GitlabMergeRequestGetter == nil {
 		return models.PullRequest{}, errors.New("Atlantis not configured to support GitLab")
 	}
-	mr, err := c.GitlabMergeRequestGetter.GetMergeRequest(repoFullName, pullNum)
+	mr, err := c.GitlabMergeRequestGetter.GetMergeRequest(baseRepo.FullName, pullNum)
 	if err != nil {
 		return models.PullRequest{}, errors.Wrap(err, "making merge request API call to GitLab")
 	}
-	pull := c.EventParser.ParseGitlabMergeRequest(mr)
+	pull := c.EventParser.ParseGitlabMergeRequest(mr, baseRepo)
 	return pull, nil
 }
 

server/events/event_parser.go

@@ -48,7 +48,7 @@ type EventParsing interface {
 	ParseGithubRepo(ghRepo *github.Repository) (models.Repo, error)
 	ParseGitlabMergeEvent(event gitlab.MergeEvent) (models.PullRequest, models.Repo, error)
 	ParseGitlabMergeCommentEvent(event gitlab.MergeCommentEvent) (baseRepo models.Repo, headRepo models.Repo, user models.User, err error)
-	ParseGitlabMergeRequest(mr *gitlab.MergeRequest) models.PullRequest
+	ParseGitlabMergeRequest(mr *gitlab.MergeRequest, baseRepo models.Repo) models.PullRequest
 }
 
 type EventParser struct {
@@ -104,7 +104,11 @@ func (e *EventParser) ParseGithubPull(pull *github.PullRequest) (models.PullRequ
 		return pullModel, headRepoModel, errors.New("number is null")
 	}
 
-	headRepoModel, err := e.ParseGithubRepo(pull.Head.Repo)
+	baseRepoModel, err := e.ParseGithubRepo(pull.Base.Repo)
+	if err != nil {
+		return pullModel, headRepoModel, err
+	}
+	headRepoModel, err = e.ParseGithubRepo(pull.Head.Repo)
 	if err != nil {
 		return pullModel, headRepoModel, err
 	}
@@ -121,6 +125,7 @@ func (e *EventParser) ParseGithubPull(pull *github.PullRequest) (models.PullRequ
 		URL:        url,
 		Num:        num,
 		State:      pullState,
+		BaseRepo:   baseRepoModel,
 	}, headRepoModel, nil
 }
 
@@ -136,6 +141,7 @@ func (e *EventParser) ParseGitlabMergeEvent(event gitlab.MergeEvent) (models.Pul
 	// GitLab also has a "merged" state, but we map that to Closed so we don't
 	// need to check for it.
 
+	repo, err := models.NewRepo(models.Gitlab, event.Project.PathWithNamespace, event.Project.GitHTTPURL, e.GitlabUser, e.GitlabToken)
 	pull := models.PullRequest{
 		URL:        event.ObjectAttributes.URL,
 		Author:     event.User.Username,
@@ -143,9 +149,9 @@ func (e *EventParser) ParseGitlabMergeEvent(event gitlab.MergeEvent) (models.Pul
 		HeadCommit: event.ObjectAttributes.LastCommit.ID,
 		Branch:     event.ObjectAttributes.SourceBranch,
 		State:      modelState,
+		BaseRepo:   repo,
 	}
 
-	repo, err := models.NewRepo(models.Gitlab, event.Project.PathWithNamespace, event.Project.GitHTTPURL, e.GitlabUser, e.GitlabToken)
 	return pull, repo, err
 }
 
@@ -169,7 +175,11 @@ func (e *EventParser) ParseGitlabMergeCommentEvent(event gitlab.MergeCommentEven
 	return
 }
 
-func (e *EventParser) ParseGitlabMergeRequest(mr *gitlab.MergeRequest) models.PullRequest {
+// ParseGitlabMergeRequest parses the merge requests and returns a pull request
+// model. We require passing in baseRepo because although can't get this information
+// from the merge request, the only caller of this function already has that
+// data. This means we can construct the pull request object correctly.
+func (e *EventParser) ParseGitlabMergeRequest(mr *gitlab.MergeRequest, baseRepo models.Repo) models.PullRequest {
 	pullState := models.Closed
 	if mr.State == gitlabPullOpened {
 		pullState = models.Open
@@ -184,5 +194,6 @@ func (e *EventParser) ParseGitlabMergeRequest(mr *gitlab.MergeRequest) models.Pu
 		HeadCommit: mr.SHA,
 		Branch:     mr.SourceBranch,
 		State:      pullState,
+		BaseRepo:   baseRepo,
 	}
 }

server/events/event_parser_test.go

@@ -128,7 +128,7 @@ func TestParseGithubPull(t *testing.T) {
 	_, _, err = parser.ParseGithubPull(&testPull)
 	ErrEquals(t, "number is null", err)
 
-	pullRes, repoRes, err := parser.ParseGithubPull(&Pull)
+	pullRes, _, err := parser.ParseGithubPull(&Pull)
 	Ok(t, err)
 	Equals(t, models.PullRequest{
 		URL:        Pull.GetHTMLURL(),
@@ -137,19 +137,18 @@ func TestParseGithubPull(t *testing.T) {
 		HeadCommit: Pull.Head.GetSHA(),
 		Num:        Pull.GetNumber(),
 		State:      models.Open,
-	}, pullRes)
-
-	Equals(t, models.Repo{
-		Owner:             "owner",
-		FullName:          "owner/repo",
-		CloneURL:          "https://github-user:github-token@github.com/owner/repo.git",
-		SanitizedCloneURL: Repo.GetCloneURL(),
-		Name:              "repo",
-		VCSHost: models.VCSHost{
-			Hostname: "github.com",
-			Type:     models.Github,
+		BaseRepo: models.Repo{
+			Owner:             "owner",
+			FullName:          "owner/repo",
+			CloneURL:          "https://github-user:github-token@github.com/owner/repo.git",
+			SanitizedCloneURL: Repo.GetCloneURL(),
+			Name:              "repo",
+			VCSHost: models.VCSHost{
+				Hostname: "github.com",
+				Type:     models.Github,
+			},
 		},
-	}, repoRes)
+	}, pullRes)
 }
 
 func TestParseGitlabMergeEvent(t *testing.T) {
@@ -159,16 +158,8 @@ func TestParseGitlabMergeEvent(t *testing.T) {
 	Ok(t, err)
 	pull, repo, err := parser.ParseGitlabMergeEvent(*event)
 	Ok(t, err)
-	Equals(t, models.PullRequest{
-		URL:        "http://example.com/diaspora/merge_requests/1",
-		Author:     "root",
-		Num:        1,
-		HeadCommit: "da1560886d4f094c3e6c9ef40349f7d38b5d27d7",
-		Branch:     "ms-viewport",
-		State:      models.Open,
-	}, pull)
 
-	Equals(t, models.Repo{
+	expRepo := models.Repo{
 		FullName:          "gitlabhq/gitlab-test",
 		Name:              "gitlab-test",
 		SanitizedCloneURL: "https://example.com/gitlabhq/gitlab-test.git",
@@ -178,7 +169,19 @@ func TestParseGitlabMergeEvent(t *testing.T) {
 			Hostname: "example.com",
 			Type:     models.Gitlab,
 		},
-	}, repo)
+	}
+
+	Equals(t, models.PullRequest{
+		URL:        "http://example.com/diaspora/merge_requests/1",
+		Author:     "root",
+		Num:        1,
+		HeadCommit: "da1560886d4f094c3e6c9ef40349f7d38b5d27d7",
+		Branch:     "ms-viewport",
+		State:      models.Open,
+		BaseRepo:   expRepo,
+	}, pull)
+
+	Equals(t, expRepo, repo)
 
 	t.Log("If the state is closed, should set field correctly.")
 	event.ObjectAttributes.State = "closed"
@@ -192,7 +195,18 @@ func TestParseGitlabMergeRequest(t *testing.T) {
 	var event *gitlab.MergeRequest
 	err := json.Unmarshal([]byte(mergeRequestJSON), &event)
 	Ok(t, err)
-	pull := parser.ParseGitlabMergeRequest(event)
+	repo := models.Repo{
+		FullName:          "gitlabhq/gitlab-test",
+		Name:              "gitlab-test",
+		SanitizedCloneURL: "https://example.com/gitlabhq/gitlab-test.git",
+		Owner:             "gitlabhq",
+		CloneURL:          "https://gitlab-user:gitlab-token@example.com/gitlabhq/gitlab-test.git",
+		VCSHost: models.VCSHost{
+			Hostname: "example.com",
+			Type:     models.Gitlab,
+		},
+	}
+	pull := parser.ParseGitlabMergeRequest(event, repo)
 	Equals(t, models.PullRequest{
 		URL:        "https://gitlab.com/lkysow/atlantis-example/merge_requests/8",
 		Author:     "lkysow",
@@ -200,11 +214,12 @@ func TestParseGitlabMergeRequest(t *testing.T) {
 		HeadCommit: "0b4ac85ea3063ad5f2974d10cd68dd1f937aaac2",
 		Branch:     "abc",
 		State:      models.Open,
+		BaseRepo:   repo,
 	}, pull)
 
 	t.Log("If the state is closed, should set field correctly.")
 	event.State = "closed"
-	pull = parser.ParseGitlabMergeRequest(event)
+	pull = parser.ParseGitlabMergeRequest(event, repo)
 	Equals(t, models.Closed, pull.State)
 }
 

server/events/mocks/mock_event_parsing.go

@@ -124,8 +124,8 @@ func (mock *MockEventParsing) ParseGitlabMergeCommentEvent(event go_gitlab.Merge
 	return ret0, ret1, ret2, ret3
 }
 
-func (mock *MockEventParsing) ParseGitlabMergeRequest(mr *go_gitlab.MergeRequest) models.PullRequest {
-	params := []pegomock.Param{mr}
+func (mock *MockEventParsing) ParseGitlabMergeRequest(mr *go_gitlab.MergeRequest, baseRepo models.Repo) models.PullRequest {
+	params := []pegomock.Param{mr, baseRepo}
 	result := pegomock.GetGenericMockFrom(mock).Invoke("ParseGitlabMergeRequest", params, []reflect.Type{reflect.TypeOf((*models.PullRequest)(nil)).Elem()})
 	var ret0 models.PullRequest
 	if len(result) != 0 {
@@ -289,8 +289,8 @@ func (c *EventParsing_ParseGitlabMergeCommentEvent_OngoingVerification) GetAllCa
 	return
 }
 
-func (verifier *VerifierEventParsing) ParseGitlabMergeRequest(mr *go_gitlab.MergeRequest) *EventParsing_ParseGitlabMergeRequest_OngoingVerification {
-	params := []pegomock.Param{mr}
+func (verifier *VerifierEventParsing) ParseGitlabMergeRequest(mr *go_gitlab.MergeRequest, baseRepo models.Repo) *EventParsing_ParseGitlabMergeRequest_OngoingVerification {
+	params := []pegomock.Param{mr, baseRepo}
 	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "ParseGitlabMergeRequest", params)
 	return &EventParsing_ParseGitlabMergeRequest_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
 }
@@ -300,18 +300,22 @@ type EventParsing_ParseGitlabMergeRequest_OngoingVerification struct {
 	methodInvocations []pegomock.MethodInvocation
 }
 
-func (c *EventParsing_ParseGitlabMergeRequest_OngoingVerification) GetCapturedArguments() *go_gitlab.MergeRequest {
-	mr := c.GetAllCapturedArguments()
-	return mr[len(mr)-1]
+func (c *EventParsing_ParseGitlabMergeRequest_OngoingVerification) GetCapturedArguments() (*go_gitlab.MergeRequest, models.Repo) {
+	mr, baseRepo := c.GetAllCapturedArguments()
+	return mr[len(mr)-1], baseRepo[len(baseRepo)-1]
 }
 
-func (c *EventParsing_ParseGitlabMergeRequest_OngoingVerification) GetAllCapturedArguments() (_param0 []*go_gitlab.MergeRequest) {
+func (c *EventParsing_ParseGitlabMergeRequest_OngoingVerification) GetAllCapturedArguments() (_param0 []*go_gitlab.MergeRequest, _param1 []models.Repo) {
 	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
 	if len(params) > 0 {
 		_param0 = make([]*go_gitlab.MergeRequest, len(params[0]))
 		for u, param := range params[0] {
 			_param0[u] = param.(*go_gitlab.MergeRequest)
 		}
+		_param1 = make([]models.Repo, len(params[1]))
+		for u, param := range params[1] {
+			_param1[u] = param.(models.Repo)
+		}
 	}
 	return
 }

server/events/models/models.go

@@ -111,6 +111,8 @@ type PullRequest struct {
 	// Gitlab supports an additional "merged" state but Github doesn't so we map
 	// merged to Closed.
 	State PullRequestState
+	// BaseRepo is the repository that the pull request will be merged into.
+	BaseRepo Repo
 }
 
 type PullRequestState int

server/events/vcs/fixtures/fixtures.go

@@ -22,7 +22,8 @@ var Pull = github.PullRequest{
 		Repo: &Repo,
 	},
 	Base: &github.PullRequestBranch{
-		SHA: github.String("sha256"),
+		SHA:  github.String("sha256"),
+		Repo: &Repo,
 	},
 	HTMLURL: github.String("html-url"),
 	User: &github.User{

server/locks_controller.go

@@ -0,0 +1,123 @@
+package server
+
+import (
+	"fmt"
+	"net/http"
+	"net/url"
+	"strings"
+
+	"github.com/gorilla/mux"
+	"github.com/runatlantis/atlantis/server/events/locking"
+	"github.com/runatlantis/atlantis/server/events/models"
+	"github.com/runatlantis/atlantis/server/events/vcs"
+	"github.com/runatlantis/atlantis/server/logging"
+)
+
+// LocksController handles all requests relating to Atlantis locks.
+type LocksController struct {
+	AtlantisVersion    string
+	Locker             locking.Locker
+	Logger             *logging.SimpleLogger
+	VCSClient          vcs.ClientProxy
+	LockDetailTemplate TemplateWriter
+}
+
+// GetLockRoute is the GET /locks/{id} route. It renders the lock detail view.
+func (l *LocksController) GetLockRoute(w http.ResponseWriter, r *http.Request) {
+	id, ok := mux.Vars(r)["id"]
+	if !ok {
+		l.respond(w, logging.Warn, http.StatusBadRequest, "No lock id in request")
+		return
+	}
+
+	l.GetLock(w, id)
+}
+
+// GetLock handles a lock detail page view. GetLockRoute is expected to
+// be called before. This function was extracted to make it testable.
+func (l *LocksController) GetLock(w http.ResponseWriter, id string) {
+	idUnencoded, err := url.QueryUnescape(id)
+	if err != nil {
+		l.respond(w, logging.Warn, http.StatusBadRequest, "Invalid lock id: %s", err)
+		return
+	}
+	lock, err := l.Locker.GetLock(idUnencoded)
+	if err != nil {
+		l.respond(w, logging.Error, http.StatusInternalServerError, "Failed getting lock: %s", err)
+		return
+	}
+	if lock == nil {
+		l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id %q", idUnencoded)
+		return
+	}
+
+	// Extract the repo owner and repo name.
+	repo := strings.Split(lock.Project.RepoFullName, "/")
+	viewData := LockDetailData{
+		LockKeyEncoded:  id,
+		LockKey:         idUnencoded,
+		RepoOwner:       repo[0],
+		RepoName:        repo[1],
+		PullRequestLink: lock.Pull.URL,
+		LockedBy:        lock.Pull.Author,
+		Workspace:       lock.Workspace,
+		AtlantisVersion: l.AtlantisVersion,
+	}
+	l.LockDetailTemplate.Execute(w, viewData) // nolint: errcheck
+}
+
+// DeleteLockRoute handles deleting the lock at id.
+func (l *LocksController) DeleteLockRoute(w http.ResponseWriter, r *http.Request) {
+	id, ok := mux.Vars(r)["id"]
+	if !ok || id == "" {
+		l.respond(w, logging.Warn, http.StatusBadRequest, "No lock id in request")
+		return
+	}
+	l.DeleteLock(w, id)
+}
+
+// DeleteLock deletes the lock and comments back on the pull request that the
+// lock has been deleted.
+// DeleteLockRoute should be called first. This method is split out to make this route testable.
+func (l *LocksController) DeleteLock(w http.ResponseWriter, id string) {
+	idUnencoded, err := url.PathUnescape(id)
+	if err != nil {
+		l.respond(w, logging.Warn, http.StatusBadRequest, "Invalid lock id %q. Failed with error: %s", id, err)
+		return
+	}
+	lock, err := l.Locker.Unlock(idUnencoded)
+	if err != nil {
+		l.respond(w, logging.Error, http.StatusInternalServerError, "deleting lock failed with: %s", err)
+		return
+	}
+	if lock == nil {
+		l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id %q", idUnencoded)
+		return
+	}
+
+	// Once the lock has been deleted, comment back on the pull request.
+	comment := fmt.Sprintf("**Warning**: The plan for path: `%s` workspace: `%s` was **discarded** via the Atlantis UI.\n\n"+
+		"To `apply` you must run `plan` again.", lock.Project.Path, lock.Workspace)
+	// NOTE: Because BaseRepo was added to the PullRequest model later, previous
+	// installations of Atlantis will have locks in their DB that do not have
+	// this field on PullRequest. We skip commenting in this case.
+	if lock.Pull.BaseRepo != (models.Repo{}) {
+		err = l.VCSClient.CreateComment(lock.Pull.BaseRepo, lock.Pull.Num, comment)
+		if err != nil {
+			l.respond(w, logging.Error, http.StatusInternalServerError, "Failed commenting on pull request: %s", err)
+			return
+		}
+	} else {
+		l.Logger.Debug("skipping commenting on pull request that lock was deleted because BaseRepo field is empty")
+	}
+	l.respond(w, logging.Info, http.StatusOK, "Deleted lock id %q", id)
+}
+
+// respond is a helper function to respond and log the response. lvl is the log
+// level to log at, code is the HTTP response code.
+func (l *LocksController) respond(w http.ResponseWriter, lvl logging.LogLevel, responseCode int, format string, args ...interface{}) {
+	response := fmt.Sprintf(format, args...)
+	l.Logger.Log(lvl, response)
+	w.WriteHeader(responseCode)
+	fmt.Fprintln(w, response)
+}

server/locks_controller_test.go

@@ -0,0 +1,229 @@
+package server_test
+
+import (
+	"bytes"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"reflect"
+	"testing"
+
+	. "github.com/petergtz/pegomock"
+	"github.com/runatlantis/atlantis/server"
+	"github.com/runatlantis/atlantis/server/events/locking/mocks"
+	"github.com/runatlantis/atlantis/server/events/models"
+	vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks"
+	"github.com/runatlantis/atlantis/server/logging"
+	sMocks "github.com/runatlantis/atlantis/server/mocks"
+)
+
+func AnyRepo() models.Repo {
+	RegisterMatcher(NewAnyMatcher(reflect.TypeOf(models.Repo{})))
+	return models.Repo{}
+}
+
+func TestGetLockRoute_NoLockID(t *testing.T) {
+	t.Log("If there is no lock ID in the request then we should get a 400")
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc := server.LocksController{
+		Logger: logging.NewNoopLogger(),
+	}
+	lc.GetLockRoute(w, eventsReq)
+	responseContains(t, w, http.StatusBadRequest, "No lock id in request")
+}
+
+func TestGetLock_InvalidLockID(t *testing.T) {
+	t.Log("If the lock ID is invalid then we should get a 400")
+	lc := server.LocksController{
+		Logger: logging.NewNoopLogger(),
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.GetLock(w, "%A@")
+	responseContains(t, w, http.StatusBadRequest, "Invalid lock id")
+}
+
+func TestGetLock_LockerErr(t *testing.T) {
+	t.Log("If there is an error retrieving the lock, a 500 is returned")
+	RegisterMockTestingT(t)
+	l := mocks.NewMockLocker()
+	When(l.GetLock("id")).ThenReturn(nil, errors.New("err"))
+	lc := server.LocksController{
+		Logger: logging.NewNoopLogger(),
+		Locker: l,
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.GetLock(w, "id")
+	responseContains(t, w, http.StatusInternalServerError, "err")
+}
+
+func TestGetLock_None(t *testing.T) {
+	t.Log("If there is no lock at that ID we get a 404")
+	RegisterMockTestingT(t)
+	l := mocks.NewMockLocker()
+	When(l.GetLock("id")).ThenReturn(nil, nil)
+	lc := server.LocksController{
+		Logger: logging.NewNoopLogger(),
+		Locker: l,
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.GetLock(w, "id")
+	responseContains(t, w, http.StatusNotFound, "No lock found at id \"id\"")
+}
+
+func TestGetLock_Success(t *testing.T) {
+	t.Log("Should be able to render a lock successfully")
+	RegisterMockTestingT(t)
+	l := mocks.NewMockLocker()
+	When(l.GetLock("id")).ThenReturn(&models.ProjectLock{
+		Project:   models.Project{RepoFullName: "owner/repo", Path: "path"},
+		Pull:      models.PullRequest{URL: "url", Author: "lkysow"},
+		Workspace: "workspace",
+	}, nil)
+	tmpl := sMocks.NewMockTemplateWriter()
+	lc := server.LocksController{
+		Logger:             logging.NewNoopLogger(),
+		Locker:             l,
+		LockDetailTemplate: tmpl,
+		AtlantisVersion:    "1300135",
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.GetLock(w, "id")
+	t.Log(w.Code)
+	tmpl.VerifyWasCalledOnce().Execute(w, server.LockDetailData{
+		LockKeyEncoded:  "id",
+		LockKey:         "id",
+		RepoOwner:       "owner",
+		RepoName:        "repo",
+		PullRequestLink: "url",
+		LockedBy:        "lkysow",
+		Workspace:       "workspace",
+		AtlantisVersion: "1300135",
+	})
+	responseContains(t, w, http.StatusOK, "")
+}
+
+func TestDeleteLockRoute_NoLockID(t *testing.T) {
+	t.Log("If there is no lock ID in the request then we should get a 400")
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc := server.LocksController{Logger: logging.NewNoopLogger()}
+	lc.DeleteLockRoute(w, eventsReq)
+	responseContains(t, w, http.StatusBadRequest, "No lock id in request")
+}
+
+func TestDeleteLock_InvalidLockID(t *testing.T) {
+	t.Log("If the lock ID is invalid then we should get a 400")
+	lc := server.LocksController{Logger: logging.NewNoopLogger()}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.DeleteLock(w, "%A@")
+	responseContains(t, w, http.StatusBadRequest, "Invalid lock id \"%A@\"")
+}
+
+func TestDeleteLock_LockerErr(t *testing.T) {
+	t.Log("If there is an error retrieving the lock, a 500 is returned")
+	RegisterMockTestingT(t)
+	l := mocks.NewMockLocker()
+	When(l.Unlock("id")).ThenReturn(nil, errors.New("err"))
+	lc := server.LocksController{
+		Locker: l,
+		Logger: logging.NewNoopLogger(),
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.DeleteLock(w, "id")
+	responseContains(t, w, http.StatusInternalServerError, "err")
+}
+
+func TestDeleteLock_None(t *testing.T) {
+	t.Log("If there is no lock at that ID we get a 404")
+	RegisterMockTestingT(t)
+	l := mocks.NewMockLocker()
+	When(l.Unlock("id")).ThenReturn(nil, nil)
+	lc := server.LocksController{
+		Locker: l,
+		Logger: logging.NewNoopLogger(),
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.DeleteLock(w, "id")
+	responseContains(t, w, http.StatusNotFound, "No lock found at id \"id\"")
+}
+
+func TestDeleteLock_OldFormat(t *testing.T) {
+	t.Log("If the lock doesn't have BaseRepo set it is deleted successfully")
+	RegisterMockTestingT(t)
+
+	cp := vcsmocks.NewMockClientProxy()
+	l := mocks.NewMockLocker()
+	When(l.Unlock("id")).ThenReturn(&models.ProjectLock{}, nil)
+	lc := server.LocksController{
+		Locker:    l,
+		Logger:    logging.NewNoopLogger(),
+		VCSClient: cp,
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.DeleteLock(w, "id")
+	responseContains(t, w, http.StatusOK, "Deleted lock id \"id\"")
+	cp.VerifyWasCalled(Never()).CreateComment(AnyRepo(), AnyInt(), AnyString())
+}
+
+func TestDeleteLock_CommentFailed(t *testing.T) {
+	t.Log("If the commenting fails we return an error")
+	RegisterMockTestingT(t)
+
+	cp := vcsmocks.NewMockClientProxy()
+	When(cp.CreateComment(AnyRepo(), AnyInt(), AnyString())).ThenReturn(errors.New("err"))
+	l := mocks.NewMockLocker()
+	When(l.Unlock("id")).ThenReturn(&models.ProjectLock{
+		Pull: models.PullRequest{
+			BaseRepo: models.Repo{FullName: "owner/repo"},
+		},
+	}, nil)
+	lc := server.LocksController{
+		Locker:    l,
+		Logger:    logging.NewNoopLogger(),
+		VCSClient: cp,
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.DeleteLock(w, "id")
+	responseContains(t, w, http.StatusInternalServerError, "Failed commenting on pull request: err")
+}
+
+func TestDeleteLock_CommentSuccess(t *testing.T) {
+	t.Log("We should comment back on the pull request if the lock is deleted")
+	RegisterMockTestingT(t)
+
+	cp := vcsmocks.NewMockClientProxy()
+	l := mocks.NewMockLocker()
+	pull := models.PullRequest{
+		BaseRepo: models.Repo{FullName: "owner/repo"},
+	}
+	When(l.Unlock("id")).ThenReturn(&models.ProjectLock{
+		Pull:      pull,
+		Workspace: "workspace",
+		Project: models.Project{
+			Path:         "path",
+			RepoFullName: "owner/repo",
+		},
+	}, nil)
+	lc := server.LocksController{
+		Locker:    l,
+		Logger:    logging.NewNoopLogger(),
+		VCSClient: cp,
+	}
+	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
+	w := httptest.NewRecorder()
+	lc.DeleteLock(w, "id")
+	responseContains(t, w, http.StatusOK, "Deleted lock id \"id\"")
+	cp.VerifyWasCalled(Once()).CreateComment(pull.BaseRepo, pull.Num,
+		"**Warning**: The plan for path: `path` workspace: `workspace` was **discarded** via the Atlantis UI.\n\n"+
+			"To `apply` you must run `plan` again.")
+}

server/server.go

@@ -58,6 +58,7 @@ type Server struct {
 	Locker             locking.Locker
 	AtlantisURL        string
 	EventsController   *EventsController
+	LocksController    *LocksController
 	IndexTemplate      TemplateWriter
 	LockDetailTemplate TemplateWriter
 	SSLCertFile        string
@@ -243,6 +244,13 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 	repoWhitelist := &events.RepoWhitelist{
 		Whitelist: userConfig.RepoWhitelist,
 	}
+	locksController := &LocksController{
+		AtlantisVersion:    config.AtlantisVersion,
+		Locker:             lockingClient,
+		Logger:             logger,
+		VCSClient:          vcsClient,
+		LockDetailTemplate: lockTemplate,
+	}
 	eventsController := &EventsController{
 		CommandRunner:          commandHandler,
 		PullCleaner:            pullClosedExecutor,
@@ -267,6 +275,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		Locker:             lockingClient,
 		AtlantisURL:        userConfig.AtlantisURL,
 		EventsController:   eventsController,
+		LocksController:    locksController,
 		IndexTemplate:      indexTemplate,
 		LockDetailTemplate: lockTemplate,
 		SSLKeyFile:         userConfig.SSLKeyFile,
@@ -281,8 +290,8 @@ func (s *Server) Start() error {
 	})
 	s.Router.PathPrefix("/static/").Handler(http.FileServer(&assetfs.AssetFS{Asset: static.Asset, AssetDir: static.AssetDir, AssetInfo: static.AssetInfo}))
 	s.Router.HandleFunc("/events", s.postEvents).Methods("POST")
-	s.Router.HandleFunc("/locks", s.DeleteLockRoute).Methods("DELETE").Queries("id", "{id:.*}")
-	lockRoute := s.Router.HandleFunc("/lock", s.GetLockRoute).Methods("GET").Queries("id", "{id}").Name(LockRouteName)
+	s.Router.HandleFunc("/locks", s.LocksController.DeleteLockRoute).Methods("DELETE").Queries("id", "{id:.*}")
+	lockRoute := s.Router.HandleFunc("/lock", s.LocksController.GetLockRoute).Methods("GET").Queries("id", "{id}").Name(LockRouteName)
 	// function that planExecutor can use to construct detail view url
 	// injecting this here because this is the earliest routes are created
 	s.CommandHandler.SetLockURL(func(lockID string) string {
@@ -355,97 +364,8 @@ func (s *Server) Index(w http.ResponseWriter, _ *http.Request) {
 	})
 }
 
-// GetLockRoute is the GET /locks/{id} route. It renders the lock detail view.
-func (s *Server) GetLockRoute(w http.ResponseWriter, r *http.Request) {
-	id, ok := mux.Vars(r)["id"]
-	if !ok {
-		w.WriteHeader(http.StatusBadRequest)
-		fmt.Fprint(w, "No lock id in request")
-		return
-	}
-	s.GetLock(w, r, id)
-}
-
-// GetLock handles a lock detail page view. getLockRoute is expected to
-// be called before. This function was extracted to make it testable.
-func (s *Server) GetLock(w http.ResponseWriter, _ *http.Request, id string) {
-	idUnencoded, err := url.QueryUnescape(id)
-	if err != nil {
-		w.WriteHeader(http.StatusBadRequest)
-		fmt.Fprint(w, "Invalid lock id")
-		return
-	}
-
-	lock, err := s.Locker.GetLock(idUnencoded)
-	if err != nil {
-		w.WriteHeader(http.StatusInternalServerError)
-		fmt.Fprint(w, err.Error())
-		return
-	}
-	if lock == nil {
-		w.WriteHeader(http.StatusNotFound)
-		fmt.Fprint(w, "No lock found at that id")
-		return
-	}
-
-	// Extract the repo owner and repo name.
-	repo := strings.Split(lock.Project.RepoFullName, "/")
-
-	l := LockDetailData{
-		LockKeyEncoded:  id,
-		LockKey:         idUnencoded,
-		RepoOwner:       repo[0],
-		RepoName:        repo[1],
-		PullRequestLink: lock.Pull.URL,
-		LockedBy:        lock.Pull.Author,
-		Workspace:       lock.Workspace,
-		AtlantisVersion: s.AtlantisVersion,
-	}
-
-	s.LockDetailTemplate.Execute(w, l) // nolint: errcheck
-}
-
-// DeleteLockRoute handles deleting the lock at id.
-func (s *Server) DeleteLockRoute(w http.ResponseWriter, r *http.Request) {
-	id, ok := mux.Vars(r)["id"]
-	if !ok || id == "" {
-		s.respond(w, logging.Warn, http.StatusBadRequest, "No lock id in request")
-		return
-	}
-	s.DeleteLock(w, r, id)
-}
-
-// DeleteLock deletes the lock. DeleteLockRoute should be called first.
-// This method is split out to make this route testable.
-func (s *Server) DeleteLock(w http.ResponseWriter, _ *http.Request, id string) {
-	idUnencoded, err := url.PathUnescape(id)
-	if err != nil {
-		s.respond(w, logging.Warn, http.StatusBadRequest, "Invalid lock id: %s", err)
-		return
-	}
-	lock, err := s.Locker.Unlock(idUnencoded)
-	if err != nil {
-		s.respond(w, logging.Error, http.StatusInternalServerError, "Failed to delete lock %s: %s", idUnencoded, err)
-		return
-	}
-	if lock == nil {
-		s.respond(w, logging.Warn, http.StatusNotFound, "No lock found at that id", idUnencoded)
-		return
-	}
-	s.respond(w, logging.Info, http.StatusOK, "Deleted lock id %s", idUnencoded)
-}
-
 // postEvents handles POST requests to our /events endpoint. These should be
 // VCS webhook requests.
 func (s *Server) postEvents(w http.ResponseWriter, r *http.Request) {
 	s.EventsController.Post(w, r)
 }
-
-// respond is a helper function to respond and log the response. lvl is the log
-// level to log at, code is the HTTP response code.
-func (s *Server) respond(w http.ResponseWriter, lvl logging.LogLevel, code int, format string, args ...interface{}) {
-	response := fmt.Sprintf(format, args...)
-	s.Logger.Log(lvl, response)
-	w.WriteHeader(code)
-	fmt.Fprintln(w, response)
-}

server/server_test.go

@@ -28,7 +28,6 @@ import (
 	"github.com/runatlantis/atlantis/server"
 	"github.com/runatlantis/atlantis/server/events/locking/mocks"
 	"github.com/runatlantis/atlantis/server/events/models"
-	"github.com/runatlantis/atlantis/server/logging"
 	sMocks "github.com/runatlantis/atlantis/server/mocks"
 	. "github.com/runatlantis/atlantis/testing"
 )
@@ -103,144 +102,6 @@ func TestIndex_Success(t *testing.T) {
 	responseContains(t, w, http.StatusOK, "")
 }
 
-func TestGetLockRoute_NoLockID(t *testing.T) {
-	t.Log("If there is no lock ID in the request then we should get a 400")
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s := server.Server{}
-	s.GetLockRoute(w, eventsReq)
-	responseContains(t, w, http.StatusBadRequest, "No lock id in request")
-}
-
-func TestGetLock_InvalidLockID(t *testing.T) {
-	t.Log("If the lock ID is invalid then we should get a 400")
-	s := server.Server{}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.GetLock(w, eventsReq, "%A@")
-	responseContains(t, w, http.StatusBadRequest, "Invalid lock id")
-}
-
-func TestGetLock_LockerErr(t *testing.T) {
-	t.Log("If there is an error retrieving the lock, a 500 is returned")
-	RegisterMockTestingT(t)
-	l := mocks.NewMockLocker()
-	When(l.GetLock("id")).ThenReturn(nil, errors.New("err"))
-	s := server.Server{
-		Locker: l,
-	}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.GetLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusInternalServerError, "err")
-}
-
-func TestGetLock_None(t *testing.T) {
-	t.Log("If there is no lock at that ID we get a 404")
-	RegisterMockTestingT(t)
-	l := mocks.NewMockLocker()
-	When(l.GetLock("id")).ThenReturn(nil, nil)
-	s := server.Server{
-		Locker: l,
-	}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.GetLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusNotFound, "No lock found at that id")
-}
-
-func TestGetLock_Success(t *testing.T) {
-	t.Log("Should be able to render a lock successfully")
-	RegisterMockTestingT(t)
-	l := mocks.NewMockLocker()
-	When(l.GetLock("id")).ThenReturn(&models.ProjectLock{
-		Project:   models.Project{RepoFullName: "owner/repo", Path: "path"},
-		Pull:      models.PullRequest{URL: "url", Author: "lkysow"},
-		Workspace: "workspace",
-	}, nil)
-	tmpl := sMocks.NewMockTemplateWriter()
-	s := server.Server{
-		Locker:             l,
-		LockDetailTemplate: tmpl,
-	}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.GetLock(w, eventsReq, "id")
-	tmpl.VerifyWasCalledOnce().Execute(w, server.LockDetailData{
-		LockKeyEncoded:  "id",
-		LockKey:         "id",
-		RepoOwner:       "owner",
-		RepoName:        "repo",
-		PullRequestLink: "url",
-		LockedBy:        "lkysow",
-		Workspace:       "workspace",
-	})
-	responseContains(t, w, http.StatusOK, "")
-}
-
-func TestDeleteLockRoute_NoLockID(t *testing.T) {
-	t.Log("If there is no lock ID in the request then we should get a 400")
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s := server.Server{Logger: logging.NewNoopLogger()}
-	s.DeleteLockRoute(w, eventsReq)
-	responseContains(t, w, http.StatusBadRequest, "No lock id in request")
-}
-
-func TestDeleteLock_InvalidLockID(t *testing.T) {
-	t.Log("If the lock ID is invalid then we should get a 400")
-	s := server.Server{Logger: logging.NewNoopLogger()}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.DeleteLock(w, eventsReq, "%A@")
-	responseContains(t, w, http.StatusBadRequest, "Invalid lock id")
-}
-
-func TestDeleteLock_LockerErr(t *testing.T) {
-	t.Log("If there is an error retrieving the lock, a 500 is returned")
-	RegisterMockTestingT(t)
-	l := mocks.NewMockLocker()
-	When(l.Unlock("id")).ThenReturn(nil, errors.New("err"))
-	s := server.Server{
-		Locker: l,
-		Logger: logging.NewNoopLogger(),
-	}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.DeleteLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusInternalServerError, "err")
-}
-
-func TestDeleteLock_None(t *testing.T) {
-	t.Log("If there is no lock at that ID we get a 404")
-	RegisterMockTestingT(t)
-	l := mocks.NewMockLocker()
-	When(l.Unlock("id")).ThenReturn(nil, nil)
-	s := server.Server{
-		Locker: l,
-		Logger: logging.NewNoopLogger(),
-	}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.DeleteLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusNotFound, "No lock found at that id")
-}
-
-func TestDeleteLock_Success(t *testing.T) {
-	t.Log("If the lock is deleted successfully we get a 200")
-	RegisterMockTestingT(t)
-	l := mocks.NewMockLocker()
-	When(l.Unlock("id")).ThenReturn(&models.ProjectLock{}, nil)
-	s := server.Server{
-		Locker: l,
-		Logger: logging.NewNoopLogger(),
-	}
-	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
-	w := httptest.NewRecorder()
-	s.DeleteLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusOK, "Deleted lock id id")
-}
-
 func responseContains(t *testing.T, r *httptest.ResponseRecorder, status int, bodySubstr string) {
 	Equals(t, status, r.Result().StatusCode)
 	body, _ := ioutil.ReadAll(r.Result().Body)