Comment on pull request when lock discarded.

When the lock is discarded from the Atlantis UI, comment back on pull
request so users know the lock was discarded.

- Adds the BaseRepo field to the PullRequest model.
- This change is backwards compatible with previous installations where
the DB will have a Project model with a PullRequest without the BaseRepo
field.

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,7 +125,7 @@ func (e *EventParser) ParseGithubPull(pull *github.PullRequest) (models.PullRequ
 		URL:        url,
 		Num:        num,
 		State:      pullState,
-		HeadRepo:   headRepoModel,
+		BaseRepo:   baseRepoModel,
 	}, headRepoModel, nil
 }
 
@@ -137,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,
@@ -144,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
 }
 
@@ -170,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
@@ -185,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

@@ -137,7 +137,7 @@ func TestParseGithubPull(t *testing.T) {
 		HeadCommit: Pull.Head.GetSHA(),
 		Num:        Pull.GetNumber(),
 		State:      models.Open,
-		HeadRepo: models.Repo{
+		BaseRepo: models.Repo{
 			Owner:             "owner",
 			FullName:          "owner/repo",
 			CloneURL:          "https://github-user:github-token@github.com/owner/repo.git",
@@ -158,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",
@@ -177,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"
@@ -191,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",
@@ -199,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

@@ -113,11 +113,6 @@ type PullRequest struct {
 	State PullRequestState
 	// BaseRepo is the repository that the pull request will be merged into.
 	BaseRepo Repo
-	// HeadRepo is the repository that is getting merged into the BaseRepo.
-	// If the pull request branch is from the same repository then HeadRepo will
-	// be the same as BaseRepo.
-	// See https://help.github.com/articles/about-pull-request-merges/.
-	HeadRepo 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

@@ -1,23 +1,19 @@
 package server
 
 import (
-	"bytes"
 	"fmt"
-	"html/template"
 	"net/http"
 	"net/url"
 	"strings"
 
 	"github.com/gorilla/mux"
-	"github.com/pkg/errors"
 	"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 webhook requests which signify 'events' in the
-// VCS host, ex. GitHub. It's split out from Server to make testing easier.
+// LocksController handles all requests relating to Atlantis locks.
 type LocksController struct {
 	AtlantisVersion    string
 	Locker             locking.Locker
@@ -26,46 +22,38 @@ type LocksController struct {
 	LockDetailTemplate TemplateWriter
 }
 
-var lockDeletedTemplate = template.Must(template.New("").Parse(
-	"**Warning**: The plan for path: `{{ .Path }}` workspace: `{{ .Workspace }}` were deleted via the Atlantis UI.\n\n" +
-		"To `apply` you must run `plan` again."))
-
 // 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, http.StatusBadRequest, "No lock id in request")
+		l.respond(w, logging.Warn, http.StatusBadRequest, "No lock id in request")
 		return
 	}
 
-	l.GetLock(w, r, id)
+	l.GetLock(w, id)
 }
 
-// GetLock handles a lock detail page view. getLockRoute is expected to
+// 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, _ *http.Request, id string) {
+func (l *LocksController) GetLock(w http.ResponseWriter, id string) {
 	idUnencoded, err := url.QueryUnescape(id)
 	if err != nil {
-		l.respond(w, http.StatusBadRequest, "Invalid lock id", err)
+		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, http.StatusInternalServerError, "failed getting lock", err)
+		l.respond(w, logging.Error, http.StatusInternalServerError, "Failed getting lock: %s", err)
 		return
 	}
 	if lock == nil {
-		l.respond(w, http.StatusNotFound, "failed getting lock:", errors.New("no corresponding lock for given id"))
+		l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id %q", idUnencoded)
 		return
 	}
-	t := l.GetLockTemplate(lock, id, idUnencoded) // nolint: errcheck
-	l.LockDetailTemplate.Execute(w, t)
-}
 
-func (l *LocksController) GetLockTemplate(lock *models.ProjectLock, id string, idUnencoded string) LockDetailData {
 	// Extract the repo owner and repo name.
 	repo := strings.Split(lock.Project.RepoFullName, "/")
-	return LockDetailData{
+	viewData := LockDetailData{
 		LockKeyEncoded:  id,
 		LockKey:         idUnencoded,
 		RepoOwner:       repo[0],
@@ -75,68 +63,61 @@ func (l *LocksController) GetLockTemplate(lock *models.ProjectLock, id string, i
 		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, http.StatusBadRequest, "No lock id in request")
+		l.respond(w, logging.Warn, http.StatusBadRequest, "No lock id in request")
 		return
 	}
-
-	lock := l.DeleteLock(w, r, id)
-
-	err := l.CommentOnPullRequest(lock)
-	if err != nil {
-		l.respond(w, http.StatusInternalServerError, "Failed commenting on pull request: %s", err)
-		return
-	}
-	l.respond(w, http.StatusOK, "Deleted lock id %s", id)
+	l.DeleteLock(w, id)
 }
 
-// DeleteLock deletes the lock
+// 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, _ *http.Request, id string) *models.ProjectLock {
+func (l *LocksController) DeleteLock(w http.ResponseWriter, id string) {
 	idUnencoded, err := url.PathUnescape(id)
 	if err != nil {
-		l.respond(w, http.StatusBadRequest, "Invalid lock id: %s. Failed with error: %s", err)
-		return 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, http.StatusInternalServerError, "deleting lock failed with: %s", err)
-		return nil
+		l.respond(w, logging.Error, http.StatusInternalServerError, "deleting lock failed with: %s", err)
+		return
 	}
 	if lock == nil {
-		l.respond(w, http.StatusNotFound, "Error deleting lock: %s", errors.New("no corresponding lock for given id"))
-		return nil
+		l.respond(w, logging.Info, http.StatusNotFound, "No lock found at id %q", idUnencoded)
+		return
 	}
-	return lock
+
+	// 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)
 }
 
-// Writes a commment on pull request
-// Exported for testing
-func (l *LocksController) CommentOnPullRequest(lock *models.ProjectLock) error {
-	// templateData := buildTemplateData(locks)
-	templateData := struct {
-		Path      string
-		Workspace string
-	}{
-		lock.Project.Path,
-		lock.Workspace,
-	}
-	var buf bytes.Buffer
-	if err := lockDeletedTemplate.Execute(&buf, templateData); err != nil {
-		return errors.Wrap(err, "rendering template for comment")
-	}
-	l.Logger.Debug("%v", lock.Pull.HeadRepo)
-	return l.VCSClient.CreateComment(lock.Pull.HeadRepo, lock.Pull.Num, buf.String())
-}
-
-func (l *LocksController) respond(w http.ResponseWriter, responseCode int, format string, args ...interface{}) {
+// 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(logging.Warn, response)
+	l.Logger.Log(lvl, response)
 	w.WriteHeader(responseCode)
 	fmt.Fprintln(w, response)
 }

server/locks_controller_test.go

@@ -15,7 +15,6 @@ import (
 	vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks"
 	"github.com/runatlantis/atlantis/server/logging"
 	sMocks "github.com/runatlantis/atlantis/server/mocks"
-	. "github.com/runatlantis/atlantis/testing"
 )
 
 func AnyRepo() models.Repo {
@@ -41,7 +40,7 @@ func TestGetLock_InvalidLockID(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.GetLock(w, eventsReq, "%A@")
+	lc.GetLock(w, "%A@")
 	responseContains(t, w, http.StatusBadRequest, "Invalid lock id")
 }
 
@@ -56,7 +55,7 @@ func TestGetLock_LockerErr(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.GetLock(w, eventsReq, "id")
+	lc.GetLock(w, "id")
 	responseContains(t, w, http.StatusInternalServerError, "err")
 }
 
@@ -71,8 +70,8 @@ func TestGetLock_None(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.GetLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusNotFound, "no corresponding lock for given id")
+	lc.GetLock(w, "id")
+	responseContains(t, w, http.StatusNotFound, "No lock found at id \"id\"")
 }
 
 func TestGetLock_Success(t *testing.T) {
@@ -93,7 +92,7 @@ func TestGetLock_Success(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.GetLock(w, eventsReq, "id")
+	lc.GetLock(w, "id")
 	t.Log(w.Code)
 	tmpl.VerifyWasCalledOnce().Execute(w, server.LockDetailData{
 		LockKeyEncoded:  "id",
@@ -122,8 +121,8 @@ func TestDeleteLock_InvalidLockID(t *testing.T) {
 	lc := server.LocksController{Logger: logging.NewNoopLogger()}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.DeleteLock(w, eventsReq, "%A@")
-	responseContains(t, w, http.StatusBadRequest, "Invalid lock id")
+	lc.DeleteLock(w, "%A@")
+	responseContains(t, w, http.StatusBadRequest, "Invalid lock id \"%A@\"")
 }
 
 func TestDeleteLock_LockerErr(t *testing.T) {
@@ -137,7 +136,7 @@ func TestDeleteLock_LockerErr(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.DeleteLock(w, eventsReq, "id")
+	lc.DeleteLock(w, "id")
 	responseContains(t, w, http.StatusInternalServerError, "err")
 }
 
@@ -152,14 +151,15 @@ func TestDeleteLock_None(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lc.DeleteLock(w, eventsReq, "id")
-	responseContains(t, w, http.StatusNotFound, "no corresponding lock for given id")
+	lc.DeleteLock(w, "id")
+	responseContains(t, w, http.StatusNotFound, "No lock found at id \"id\"")
 }
 
-func TestDeleteLock_Success(t *testing.T) {
-	t.Log("If the lock is deleted successfully we get a 200")
-	cp := vcsmocks.NewMockClientProxy()
+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{
@@ -169,20 +169,61 @@ func TestDeleteLock_Success(t *testing.T) {
 	}
 	eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil))
 	w := httptest.NewRecorder()
-	lock := lc.DeleteLock(w, eventsReq, "id")
-	Equals(t, &models.ProjectLock{}, lock)
+	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 lock is deleted successfully we get a 200")
-	cp := vcsmocks.NewMockClientProxy()
+	t.Log("If the commenting fails we return an error")
 	RegisterMockTestingT(t)
-	When(cp.CreateComment(AnyRepo(), AnyInt(), AnyString())).ThenReturn(nil)
+
+	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,
 	}
-	lock := &models.ProjectLock{}
-	err := lc.CommentOnPullRequest(lock)
-	Equals(t, err, nil)
+	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

@@ -72,7 +72,6 @@ type UserConfig struct {
 	AllowForkPRs        bool   `mapstructure:"allow-fork-prs"`
 	AtlantisURL         string `mapstructure:"atlantis-url"`
 	DataDir             string `mapstructure:"data-dir"`
-	DisableLocking      string `mapstructure:"disable-locking"`
 	GithubHostname      string `mapstructure:"gh-hostname"`
 	GithubToken         string `mapstructure:"gh-token"`
 	GithubUser          string `mapstructure:"gh-user"`