From 9df3609e6ec8941430a2a15f433eb668fd4a3a22 Mon Sep 17 00:00:00 2001 From: mo-hit Date: Tue, 27 Mar 2018 23:36:17 -0700 Subject: [PATCH 1/2] WIP Implement commenting on pull when lock deleted. --- server/events/event_parser.go | 1 + server/events/event_parser_test.go | 25 ++-- server/events/models/models.go | 7 ++ server/locks_controller.go | 142 ++++++++++++++++++++++ server/locks_controller_test.go | 188 +++++++++++++++++++++++++++++ server/server.go | 103 ++-------------- server/server_test.go | 139 --------------------- 7 files changed, 362 insertions(+), 243 deletions(-) create mode 100644 server/locks_controller.go create mode 100644 server/locks_controller_test.go diff --git a/server/events/event_parser.go b/server/events/event_parser.go index eb372704f..1b393b023 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -121,6 +121,7 @@ func (e *EventParser) ParseGithubPull(pull *github.PullRequest) (models.PullRequ URL: url, Num: num, State: pullState, + HeadRepo: headRepoModel, }, headRepoModel, nil } diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index 3d4ab57e3..63b084677 100644 --- a/server/events/event_parser_test.go +++ b/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, + HeadRepo: 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) { diff --git a/server/events/models/models.go b/server/events/models/models.go index 3775a40a2..13d6815ba 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -111,6 +111,13 @@ 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 + // 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/locks_controller.go b/server/locks_controller.go new file mode 100644 index 000000000..62afa3557 --- /dev/null +++ b/server/locks_controller.go @@ -0,0 +1,142 @@ +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. +type LocksController struct { + AtlantisVersion string + Locker locking.Locker + Logger *logging.SimpleLogger + VCSClient vcs.ClientProxy + 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") + return + } + + l.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 (l *LocksController) GetLock(w http.ResponseWriter, _ *http.Request, id string) { + idUnencoded, err := url.QueryUnescape(id) + if err != nil { + l.respond(w, http.StatusBadRequest, "Invalid lock id", err) + return + } + lock, err := l.Locker.GetLock(idUnencoded) + if err != nil { + l.respond(w, http.StatusInternalServerError, "failed getting lock", err) + return + } + if lock == nil { + l.respond(w, http.StatusNotFound, "failed getting lock:", errors.New("no corresponding lock for given id")) + 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{ + LockKeyEncoded: id, + LockKey: idUnencoded, + RepoOwner: repo[0], + RepoName: repo[1], + PullRequestLink: lock.Pull.URL, + LockedBy: lock.Pull.Author, + Workspace: lock.Workspace, + AtlantisVersion: l.AtlantisVersion, + } +} + +// 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") + 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) +} + +// DeleteLock deletes the lock +// 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 { + idUnencoded, err := url.PathUnescape(id) + if err != nil { + l.respond(w, http.StatusBadRequest, "Invalid lock id: %s. Failed with error: %s", err) + return nil + } + lock, err := l.Locker.Unlock(idUnencoded) + if err != nil { + l.respond(w, http.StatusInternalServerError, "deleting lock failed with: %s", err) + return nil + } + if lock == nil { + l.respond(w, http.StatusNotFound, "Error deleting lock: %s", errors.New("no corresponding lock for given id")) + return nil + } + return lock +} + +// 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{}) { + response := fmt.Sprintf(format, args...) + l.Logger.Log(logging.Warn, response) + w.WriteHeader(responseCode) + fmt.Fprintln(w, response) +} diff --git a/server/locks_controller_test.go b/server/locks_controller_test.go new file mode 100644 index 000000000..5efaa412d --- /dev/null +++ b/server/locks_controller_test.go @@ -0,0 +1,188 @@ +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" + . "github.com/runatlantis/atlantis/testing" +) + +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, 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")) + lc := server.LocksController{ + Logger: logging.NewNoopLogger(), + Locker: l, + } + eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil)) + w := httptest.NewRecorder() + lc.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) + lc := server.LocksController{ + Logger: logging.NewNoopLogger(), + Locker: l, + } + 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") +} + +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, eventsReq, "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, 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")) + lc := server.LocksController{ + Locker: l, + Logger: logging.NewNoopLogger(), + } + eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil)) + w := httptest.NewRecorder() + lc.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) + lc := server.LocksController{ + Locker: l, + Logger: logging.NewNoopLogger(), + } + 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") +} + +func TestDeleteLock_Success(t *testing.T) { + t.Log("If the lock is deleted successfully we get a 200") + cp := vcsmocks.NewMockClientProxy() + RegisterMockTestingT(t) + 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() + lock := lc.DeleteLock(w, eventsReq, "id") + Equals(t, &models.ProjectLock{}, lock) +} + +func TestDeleteLock_CommentFailed(t *testing.T) { + t.Log("If the lock is deleted successfully we get a 200") + cp := vcsmocks.NewMockClientProxy() + RegisterMockTestingT(t) + When(cp.CreateComment(AnyRepo(), AnyInt(), AnyString())).ThenReturn(nil) + lc := server.LocksController{ + Logger: logging.NewNoopLogger(), + VCSClient: cp, + } + lock := &models.ProjectLock{} + err := lc.CommentOnPullRequest(lock) + Equals(t, err, nil) +} diff --git a/server/server.go b/server/server.go index 5be4122f6..5e2f0a58e 100644 --- a/server/server.go +++ b/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 @@ -71,6 +72,7 @@ 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"` @@ -243,6 +245,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 +276,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 +291,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 +365,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) -} diff --git a/server/server_test.go b/server/server_test.go index 8c6510f97..21134d906 100644 --- a/server/server_test.go +++ b/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) From a57d4c6618b6cebbedafe4900d24d24f0089178f Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Wed, 30 May 2018 16:29:43 +0200 Subject: [PATCH 2/2] 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"`