Refactor EventParser to not take reference to ctx

server/events/event_parser.go

@@ -21,7 +21,7 @@ type Command struct {
 
 type EventParsing interface {
 	DetermineCommand(comment *github.IssueCommentEvent) (*Command, error)
-	ExtractCommentData(comment *github.IssueCommentEvent, ctx *CommandContext) error
+	ExtractCommentData(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pull models.PullRequest, err error)
 	ExtractPullData(pull *github.PullRequest) (models.PullRequest, models.Repo, error)
 	ExtractRepoData(ghRepo *github.Repository) (models.Repo, error)
 }
@@ -100,35 +100,38 @@ func (e *EventParser) DetermineCommand(comment *github.IssueCommentEvent) (*Comm
 	return c, nil
 }
 
-func (e *EventParser) ExtractCommentData(comment *github.IssueCommentEvent, ctx *CommandContext) error {
-	repo, err := e.ExtractRepoData(comment.Repo)
+func (e *EventParser) ExtractCommentData(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pull models.PullRequest, err error) {
+	baseRepo, err = e.ExtractRepoData(comment.Repo)
 	if err != nil {
-		return err
+		return
 	}
 	pullNum := comment.Issue.GetNumber()
 	if pullNum == 0 {
-		return errors.New("issue.number is null")
+		err = errors.New("issue.number is null")
+		return
 	}
 	pullCreator := comment.Issue.User.GetLogin()
 	if pullCreator == "" {
-		return errors.New("issue.user.login is null")
+		err = errors.New("issue.user.login is null")
+		return
 	}
 	htmlURL := comment.Issue.GetHTMLURL()
 	if htmlURL == "" {
-		return errors.New("issue.html_url is null")
+		err = errors.New("issue.html_url is null")
+		return
 	}
 	commentorUsername := comment.Comment.User.GetLogin()
 	if commentorUsername == "" {
-		return errors.New("comment.user.login is null")
+		err = errors.New("comment.user.login is null")
+		return
 	}
-	ctx.BaseRepo = repo
-	ctx.User = models.User{
+	user = models.User{
 		Username: commentorUsername,
 	}
-	ctx.Pull = models.PullRequest{
+	pull = models.PullRequest{
 		Num: pullNum,
 	}
-	return nil
+	return
 }
 
 func (e *EventParser) ExtractPullData(pull *github.PullRequest) (models.PullRequest, models.Repo, error) {

server/events/event_parser_test.go

@@ -158,35 +158,33 @@ func TestExtractCommentData(t *testing.T) {
 			User: &github.User{Login: github.String("comment_user")},
 		},
 	}
-	ctx := events.CommandContext{}
-
 	testComment := deepcopy.Copy(comment).(github.IssueCommentEvent)
 	testComment.Repo = nil
-	err := parser.ExtractCommentData(&testComment, &ctx)
+	_, _, _, err := parser.ExtractCommentData(&testComment)
 	Equals(t, errors.New("repository.full_name is null"), err)
 
 	testComment = deepcopy.Copy(comment).(github.IssueCommentEvent)
 	testComment.Issue = nil
-	err = parser.ExtractCommentData(&testComment, &ctx)
+	_, _, _, err = parser.ExtractCommentData(&testComment)
 	Equals(t, errors.New("issue.number is null"), err)
 
 	testComment = deepcopy.Copy(comment).(github.IssueCommentEvent)
 	testComment.Issue.User = nil
-	err = parser.ExtractCommentData(&testComment, &ctx)
+	_, _, _, err = parser.ExtractCommentData(&testComment)
 	Equals(t, errors.New("issue.user.login is null"), err)
 
 	testComment = deepcopy.Copy(comment).(github.IssueCommentEvent)
 	testComment.Issue.HTMLURL = nil
-	err = parser.ExtractCommentData(&testComment, &ctx)
+	_, _, _, err = parser.ExtractCommentData(&testComment)
 	Equals(t, errors.New("issue.html_url is null"), err)
 
 	testComment = deepcopy.Copy(comment).(github.IssueCommentEvent)
 	testComment.Comment.User.Login = nil
-	err = parser.ExtractCommentData(&testComment, &ctx)
+	_, _, _, err = parser.ExtractCommentData(&testComment)
 	Equals(t, errors.New("comment.user.login is null"), err)
 
 	// this should be successful
-	err = parser.ExtractCommentData(&comment, &ctx)
+	repo, user, pull, err := parser.ExtractCommentData(&comment)
 	Ok(t, err)
 	Equals(t, models.Repo{
 		Owner:             *comment.Repo.Owner.Login,
@@ -194,13 +192,13 @@ func TestExtractCommentData(t *testing.T) {
 		CloneURL:          "https://user:token@github.com/lkysow/atlantis-example.git",
 		SanitizedCloneURL: *comment.Repo.CloneURL,
 		Name:              "repo",
-	}, ctx.BaseRepo)
+	}, repo)
 	Equals(t, models.User{
 		Username: *comment.Comment.User.Login,
-	}, ctx.User)
+	}, user)
 	Equals(t, models.PullRequest{
 		Num: *comment.Issue.Number,
-	}, ctx.Pull)
+	}, pull)
 }
 
 func TestExtractPullData(t *testing.T) {

server/events/mocks/mock_event_parsing.go

@@ -35,16 +35,28 @@ func (mock *MockEventParsing) DetermineCommand(comment *github.IssueCommentEvent
 	return ret0, ret1
 }
 
-func (mock *MockEventParsing) ExtractCommentData(comment *github.IssueCommentEvent, ctx *events.CommandContext) error {
-	params := []pegomock.Param{comment, ctx}
-	result := pegomock.GetGenericMockFrom(mock).Invoke("ExtractCommentData", params, []reflect.Type{reflect.TypeOf((*error)(nil)).Elem()})
-	var ret0 error
+func (mock *MockEventParsing) ExtractCommentData(comment *github.IssueCommentEvent) (models.Repo, models.User, models.PullRequest, error) {
+	params := []pegomock.Param{comment}
+	result := pegomock.GetGenericMockFrom(mock).Invoke("ExtractCommentData", params, []reflect.Type{reflect.TypeOf((*models.Repo)(nil)).Elem(), reflect.TypeOf((*models.User)(nil)).Elem(), reflect.TypeOf((*models.PullRequest)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()})
+	var ret0 models.Repo
+	var ret1 models.User
+	var ret2 models.PullRequest
+	var ret3 error
 	if len(result) != 0 {
 		if result[0] != nil {
-			ret0 = result[0].(error)
+			ret0 = result[0].(models.Repo)
+		}
+		if result[1] != nil {
+			ret1 = result[1].(models.User)
+		}
+		if result[2] != nil {
+			ret2 = result[2].(models.PullRequest)
+		}
+		if result[3] != nil {
+			ret3 = result[3].(error)
 		}
 	}
-	return ret0
+	return ret0, ret1, ret2, ret3
 }
 
 func (mock *MockEventParsing) ExtractPullData(pull *github.PullRequest) (models.PullRequest, models.Repo, error) {
@@ -128,8 +140,8 @@ func (c *EventParsing_DetermineCommand_OngoingVerification) GetAllCapturedArgume
 	return
 }
 
-func (verifier *VerifierEventParsing) ExtractCommentData(comment *github.IssueCommentEvent, ctx *events.CommandContext) *EventParsing_ExtractCommentData_OngoingVerification {
-	params := []pegomock.Param{comment, ctx}
+func (verifier *VerifierEventParsing) ExtractCommentData(comment *github.IssueCommentEvent) *EventParsing_ExtractCommentData_OngoingVerification {
+	params := []pegomock.Param{comment}
 	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "ExtractCommentData", params)
 	return &EventParsing_ExtractCommentData_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
 }
@@ -139,22 +151,18 @@ type EventParsing_ExtractCommentData_OngoingVerification struct {
 	methodInvocations []pegomock.MethodInvocation
 }
 
-func (c *EventParsing_ExtractCommentData_OngoingVerification) GetCapturedArguments() (*github.IssueCommentEvent, *events.CommandContext) {
-	comment, ctx := c.GetAllCapturedArguments()
-	return comment[len(comment)-1], ctx[len(ctx)-1]
+func (c *EventParsing_ExtractCommentData_OngoingVerification) GetCapturedArguments() *github.IssueCommentEvent {
+	comment := c.GetAllCapturedArguments()
+	return comment[len(comment)-1]
 }
 
-func (c *EventParsing_ExtractCommentData_OngoingVerification) GetAllCapturedArguments() (_param0 []*github.IssueCommentEvent, _param1 []*events.CommandContext) {
+func (c *EventParsing_ExtractCommentData_OngoingVerification) GetAllCapturedArguments() (_param0 []*github.IssueCommentEvent) {
 	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
 	if len(params) > 0 {
 		_param0 = make([]*github.IssueCommentEvent, len(params[0]))
 		for u, param := range params[0] {
 			_param0[u] = param.(*github.IssueCommentEvent)
 		}
-		_param1 = make([]*events.CommandContext, len(params[1]))
-		for u, param := range params[1] {
-			_param1[u] = param.(*events.CommandContext)
-		}
 	}
 	return
 }

server/events_controller.go

@@ -44,7 +44,16 @@ func (e *EventsController) HandleCommentEvent(w http.ResponseWriter, event *gith
 		return
 	}
 
+	baseRepo, user, pull, err := e.Parser.ExtractCommentData(event)
+	if err != nil {
+		e.respond(w, logging.Error, http.StatusBadRequest, "Failed parsing event: %v %s", err, githubReqID)
+		return
+	}
 	ctx := &events.CommandContext{}
+	ctx.BaseRepo = baseRepo
+	ctx.User = user
+	ctx.Pull = pull
+
 	command, err := e.Parser.DetermineCommand(event)
 	if err != nil {
 		e.respond(w, logging.Debug, http.StatusOK, "Ignoring: %s %s", err, githubReqID)
@@ -52,10 +61,6 @@ func (e *EventsController) HandleCommentEvent(w http.ResponseWriter, event *gith
 	}
 	ctx.Command = command
 
-	if err = e.Parser.ExtractCommentData(event, ctx); err != nil {
-		e.respond(w, logging.Error, http.StatusInternalServerError, "Failed parsing event: %v %s", err, githubReqID)
-		return
-	}
 	// Respond with success and then actually execute the command asynchronously.
 	// We use a goroutine so that this function returns and the connection is
 	// closed.
@@ -81,7 +86,7 @@ func (e *EventsController) HandlePullRequestEvent(w http.ResponseWriter, pullEve
 	}
 
 	if err := e.PullClosedExecutor.CleanUpPull(repo, pull); err != nil {
-		e.respond(w, logging.Error, http.StatusInternalServerError, "Error cleaning pull request: %s", err)
+		e.respond(w, logging.Error, http.StatusServiceUnavailable, "Error cleaning pull request: %s", err)
 		return
 	}
 	e.Logger.Info("deleted locks and workspace for repo %s, pull %d", repo.FullName, pull.Num)

server/events_controller_test.go

@@ -2,21 +2,82 @@ package server_test
 
 import (
 	"testing"
-	//. "github.com/hootsuite/atlantis/testing_util"
-	//. "github.com/petergtz/pegomock"
-	//"github.com/hootsuite/atlantis/server"
-	//"github.com/hootsuite/atlantis/server/logging"
-	//"net/http"
-	//"bytes"
+	. "github.com/hootsuite/atlantis/testing_util"
+	. "github.com/petergtz/pegomock"
+	"github.com/hootsuite/atlantis/server"
+	"github.com/hootsuite/atlantis/server/logging"
+	"net/http"
+	"bytes"
+	"github.com/hootsuite/atlantis/server/mocks"
+	"net/http/httptest"
+	"errors"
 )
 
 func TestPost_InvalidSecret(t *testing.T) {
-	//t.Log("when the payload can't be validated against the github secret there is an error")
-	//RegisterMockTestingT(t)
-	//e := server.EventsController{
-	//	Logger: logging.NewNoopLogger(),
-	//	GithubWebHookSecret: []byte("secret"),
-	//}
-	//req, err := http.NewRequest("GET", "http://localhost/event", bytes.NewBuffer(nil))
-	//e.Post()
+	t.Log("when the payload can't be validated a 400 is returned")
+	RegisterMockTestingT(t)
+	v := mocks.NewMockGHRequestValidator()
+	secret := []byte("secret")
+	e := server.EventsController{
+		Logger:              logging.NewNoopLogger(),
+		GithubWebHookSecret: secret,
+		Validator:           v,
+	}
+	req, err := http.NewRequest("GET", "http://localhost/event", bytes.NewBuffer(nil))
+	Ok(t, err)
+	w := httptest.NewRecorder()
+	When(v.Validate(req, secret)).ThenReturn(nil, errors.New("err"))
+	e.Post(w, req)
+
+	Equals(t, http.StatusBadRequest, w.Result().StatusCode)
+}
+
+func TestPost_UnsupportedEvent(t *testing.T) {
+	t.Log("when the event type is unsupported we ignore it")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_CommentNotCreated(t *testing.T) {
+	t.Log("when the event is a comment but it's not a created event we ignore it")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_CommentInvalidComment(t *testing.T) {
+	t.Log("when the event is a comment without all expected data we return a 400")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_CommentInvalidCommand(t *testing.T) {
+	t.Log("when the event is a comment with an invalid command we ignore it")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_CommentSuccess(t *testing.T) {
+	t.Log("when the event is comment with a valid command we call the command handler")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_PullRequestNotClosed(t *testing.T) {
+	t.Log("when the event is pull reuqest but it's not a closed event we ignore it")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_PullRequestInvalid(t *testing.T) {
+	t.Log("when the event is pull reuqest with invalid data we return a 400")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_PullRequestInvalidRepo(t *testing.T) {
+	t.Log("when the event is pull reuqest with invalid repo data we return a 400")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_PullRequestErrCleaningPull(t *testing.T) {
+	t.Log("when the event is a pull request and we have an error calling CleanUpPull we return a 503")
+	RegisterMockTestingT(t)
+}
+
+func TestPost_PullRequestSuccess(t *testing.T) {
+	t.Log("when the event is a pull request and everything works we return a 200")
+	RegisterMockTestingT(t)
 }

server/gh_request_validation.go

@@ -9,6 +9,8 @@ import (
 	"github.com/google/go-github/github"
 )
 
+//go:generate pegomock generate --use-experimental-model-gen --package mocks -o mocks/mock_gh_request_validation.go GHRequestValidator
+
 // GHRequestValidator validates GitHub requests.
 type GHRequestValidator interface {
 	// Validate returns the JSON payload of the request.

server/mocks/mock_gh_request_validation.go

@@ -0,0 +1,83 @@
+// Automatically generated by pegomock. DO NOT EDIT!
+// Source: github.com/hootsuite/atlantis/server (interfaces: GHRequestValidator)
+
+package mocks
+
+import (
+	pegomock "github.com/petergtz/pegomock"
+	http "net/http"
+	"reflect"
+)
+
+type MockGHRequestValidator struct {
+	fail func(message string, callerSkip ...int)
+}
+
+func NewMockGHRequestValidator() *MockGHRequestValidator {
+	return &MockGHRequestValidator{fail: pegomock.GlobalFailHandler}
+}
+
+func (mock *MockGHRequestValidator) Validate(r *http.Request, secret []byte) ([]byte, error) {
+	params := []pegomock.Param{r, secret}
+	result := pegomock.GetGenericMockFrom(mock).Invoke("Validate", params, []reflect.Type{reflect.TypeOf((*[]byte)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()})
+	var ret0 []byte
+	var ret1 error
+	if len(result) != 0 {
+		if result[0] != nil {
+			ret0 = result[0].([]byte)
+		}
+		if result[1] != nil {
+			ret1 = result[1].(error)
+		}
+	}
+	return ret0, ret1
+}
+
+func (mock *MockGHRequestValidator) VerifyWasCalledOnce() *VerifierGHRequestValidator {
+	return &VerifierGHRequestValidator{mock, pegomock.Times(1), nil}
+}
+
+func (mock *MockGHRequestValidator) VerifyWasCalled(invocationCountMatcher pegomock.Matcher) *VerifierGHRequestValidator {
+	return &VerifierGHRequestValidator{mock, invocationCountMatcher, nil}
+}
+
+func (mock *MockGHRequestValidator) VerifyWasCalledInOrder(invocationCountMatcher pegomock.Matcher, inOrderContext *pegomock.InOrderContext) *VerifierGHRequestValidator {
+	return &VerifierGHRequestValidator{mock, invocationCountMatcher, inOrderContext}
+}
+
+type VerifierGHRequestValidator struct {
+	mock                   *MockGHRequestValidator
+	invocationCountMatcher pegomock.Matcher
+	inOrderContext         *pegomock.InOrderContext
+}
+
+func (verifier *VerifierGHRequestValidator) Validate(r *http.Request, secret []byte) *GHRequestValidator_Validate_OngoingVerification {
+	params := []pegomock.Param{r, secret}
+	methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "Validate", params)
+	return &GHRequestValidator_Validate_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations}
+}
+
+type GHRequestValidator_Validate_OngoingVerification struct {
+	mock              *MockGHRequestValidator
+	methodInvocations []pegomock.MethodInvocation
+}
+
+func (c *GHRequestValidator_Validate_OngoingVerification) GetCapturedArguments() (*http.Request, []byte) {
+	r, secret := c.GetAllCapturedArguments()
+	return r[len(r)-1], secret[len(secret)-1]
+}
+
+func (c *GHRequestValidator_Validate_OngoingVerification) GetAllCapturedArguments() (_param0 []*http.Request, _param1 [][]byte) {
+	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
+	if len(params) > 0 {
+		_param0 = make([]*http.Request, len(params[0]))
+		for u, param := range params[0] {
+			_param0[u] = param.(*http.Request)
+		}
+		_param1 = make([][]byte, len(params[1]))
+		for u, param := range params[1] {
+			_param1[u] = param.([]byte)
+		}
+	}
+	return
+}