From a57d4c6618b6cebbedafe4900d24d24f0089178f Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Wed, 30 May 2018 16:29:43 +0200 Subject: [PATCH] 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 | 8 +- server/events/event_parser.go | 20 +++-- server/events/event_parser_test.go | 42 ++++++--- server/events/mocks/mock_event_parsing.go | 20 +++-- server/events/models/models.go | 5 -- server/events/vcs/fixtures/fixtures.go | 3 +- server/locks_controller.go | 103 +++++++++------------- server/locks_controller_test.go | 85 +++++++++++++----- server/server.go | 1 - 9 files changed, 167 insertions(+), 120 deletions(-) diff --git a/server/events/command_handler.go b/server/events/command_handler.go index fb70901fe..52dd2090b 100644 --- a/server/events/command_handler.go +++ b/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 } diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 1b393b023..d42e4beda 100644 --- a/server/events/event_parser.go +++ b/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, } } diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index 63b084677..9928dc69c 100644 --- a/server/events/event_parser_test.go +++ b/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) } diff --git a/server/events/mocks/mock_event_parsing.go b/server/events/mocks/mock_event_parsing.go index 5e2d1746f..e88b581dc 100644 --- a/server/events/mocks/mock_event_parsing.go +++ b/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 } diff --git a/server/events/models/models.go b/server/events/models/models.go index 13d6815ba..7344fd28e 100644 --- a/server/events/models/models.go +++ b/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 diff --git a/server/events/vcs/fixtures/fixtures.go b/server/events/vcs/fixtures/fixtures.go index 168ddc5b4..e3155f199 100644 --- a/server/events/vcs/fixtures/fixtures.go +++ b/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{ diff --git a/server/locks_controller.go b/server/locks_controller.go index 62afa3557..de290662f 100644 --- a/server/locks_controller.go +++ b/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) } diff --git a/server/locks_controller_test.go b/server/locks_controller_test.go index 5efaa412d..869ad2ab0 100644 --- a/server/locks_controller_test.go +++ b/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.") } diff --git a/server/server.go b/server/server.go index 5e2f0a58e..06c263b27 100644 --- a/server/server.go +++ b/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"`