From 75e90c1917e0ea8c596e07eefcd8d8fec3b914ad Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Tue, 27 Feb 2018 12:55:26 -0800 Subject: [PATCH 1/6] Quote extra comment args. To avoid an attacker prepending something like atlantis plan -- ; cat /etc/passwd --- server/events/event_parser.go | 7 ++++- server/events/event_parser_test.go | 10 +++--- server/events/plan_executor_test.go | 48 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 3225e527b7..36b2737d1d 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -117,7 +117,12 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma // todo: keep track of the args we're discarding and include that with // comment as a warning. if flagSet.ArgsLenAtDash() != -1 { - extraArgs = flagSet.Args()[flagSet.ArgsLenAtDash():] + extraArgsUnsafe := flagSet.Args()[flagSet.ArgsLenAtDash():] + // Quote all extra args so there isn't a security issue when we append + // them to the terraform commands, ex. "; cat /etc/passwd" + for _, arg := range extraArgsUnsafe { + extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, arg)) + } } // If dir is specified, must ensure it's a valid path. diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index c8c5bc8aea..2186416826 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -157,14 +157,14 @@ func TestDetermineCommand_Parsing(t *testing.T) { "workspace", "dir", false, - "--verbose", + "\"--verbose\"", }, { "-w workspace -- -d dir --verbose", "workspace", "", false, - "-d dir --verbose", + "\"-d\" \"dir\" \"--verbose\"", }, // Test missing arguments. { @@ -194,7 +194,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { "workspace", "dir", true, - "arg one -two --three &&", + "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", }, // Test whitespace. { @@ -202,14 +202,14 @@ func TestDetermineCommand_Parsing(t *testing.T) { "workspace", "dir", true, - "arg one -two --three &&", + "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", }, { " -w workspace -d dir --verbose -- arg one -two --three &&", "workspace", "dir", true, - "arg one -two --three &&", + "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", }, // Test that the dir string is normalized. { diff --git a/server/events/plan_executor_test.go b/server/events/plan_executor_test.go index a26440eed6..168114367f 100644 --- a/server/events/plan_executor_test.go +++ b/server/events/plan_executor_test.go @@ -97,6 +97,54 @@ func TestExecute_DirectoryAndWorkspaceSet(t *testing.T) { Equals(t, "lockurl-key", result.PlanSuccess.LockURL) } +func TestExecute_AddedArgs(t *testing.T) { + t.Log("Test that we include extra-args added to the comment in the plan command") + p, runner, _ := setupPlanExecutorTest(t) + ctx := deepcopy.Copy(planCtx).(events.CommandContext) + ctx.Log = logging.NewNoopLogger() + ctx.Command.Flags = []string{"\"-target=resource\"", "\"-var\"", "\"a=b\"", "\";\"", "\"echo\"", "\"hi\""} + + When(p.VCSClient.GetModifiedFiles(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), matchers.AnyVcsHost())).ThenReturn([]string{"file.tf"}, nil) + When(p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, "workspace")). + ThenReturn("/tmp/clone-repo", nil) + When(p.ProjectPreExecute.Execute(&ctx, "/tmp/clone-repo", models.Project{RepoFullName: "", Path: "."})). + ThenReturn(events.PreExecuteResult{ + LockResponse: locking.TryLockResponse{ + LockKey: "key", + }, + }) + r := p.Execute(&ctx) + + runner.VerifyWasCalledOnce().RunCommandWithVersion( + ctx.Log, + "/tmp/clone-repo", + []string{ + "plan", + "-refresh", + "-no-color", + "-out", + "/tmp/clone-repo/workspace.tfplan", + "-var", + "atlantis_user=anubhavmishra", + // NOTE: extra args should be quoted to prevent an attacker from + // appending malicious commands. + "\"-target=resource\"", + "\"-var\"", + "\"a=b\"", + "\";\"", + "\"echo\"", + "\"hi\"", + }, + nil, + "workspace", + ) + Assert(t, len(r.ProjectResults) == 1, "exp one project result") + result := r.ProjectResults[0] + Assert(t, result.PlanSuccess != nil, "exp plan success to not be nil") + Equals(t, "", result.PlanSuccess.TerraformOutput) + Equals(t, "lockurl-key", result.PlanSuccess.LockURL) +} + func TestExecute_Success(t *testing.T) { t.Log("If there are no errors, the plan should be returned") p, runner, _ := setupPlanExecutorTest(t) From 9586c87246cd06323501cb7aa37170702fb66023 Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Tue, 27 Feb 2018 14:49:26 -0800 Subject: [PATCH 2/6] Use pullNum instead of pull model in CreateComment This will enable us to use the CreateComment function without having the full pull request model. --- server/events/command_handler.go | 6 +++--- server/events/command_handler_test.go | 8 ++++---- server/events/pull_closed_executor.go | 2 +- server/events/pull_closed_executor_test.go | 4 ++-- server/events/vcs/client.go | 2 +- server/events/vcs/github_client.go | 4 ++-- server/events/vcs/gitlab_client.go | 4 ++-- server/events/vcs/mocks/mock_proxy.go | 20 +++++++++---------- .../events/vcs/not_configured_vcs_client.go | 2 +- server/events/vcs/proxy.go | 8 ++++---- 10 files changed, 30 insertions(+), 30 deletions(-) diff --git a/server/events/command_handler.go b/server/events/command_handler.go index b34f4b8e85..1ff9477c15 100644 --- a/server/events/command_handler.go +++ b/server/events/command_handler.go @@ -125,7 +125,7 @@ func (c *CommandHandler) run(ctx *CommandContext) { if ctx.Pull.State != models.Open { ctx.Log.Info("command was run on closed pull request") - c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull, "Atlantis commands can't be run on closed pull requests", ctx.VCSHost) // nolint: errcheck + c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, "Atlantis commands can't be run on closed pull requests", ctx.VCSHost) // nolint: errcheck return } @@ -167,14 +167,14 @@ func (c *CommandHandler) updatePull(ctx *CommandContext, res CommandResponse) { // Update the pull request's status icon and comment back. c.CommitStatusUpdater.UpdateProjectResult(ctx, res) // nolint: errcheck comment := c.MarkdownRenderer.Render(res, ctx.Command.Name, ctx.Log.History.String(), ctx.Command.Verbose) - c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull, comment, ctx.VCSHost) // nolint: errcheck + c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, comment, ctx.VCSHost) // nolint: errcheck } // logPanics logs and creates a comment on the pull request for panics. func (c *CommandHandler) logPanics(ctx *CommandContext) { if err := recover(); err != nil { stack := recovery.Stack(3) - c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull, // nolint: errcheck + c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, // nolint: errcheck fmt.Sprintf("**Error: goroutine panic. This is a bug.**\n```\n%s\n%s```", err, stack), ctx.VCSHost) ctx.Log.Err("PANIC: %s\n%s", err, stack) } diff --git a/server/events/command_handler_test.go b/server/events/command_handler_test.go index 7bdd416d11..1a8ee83b52 100644 --- a/server/events/command_handler_test.go +++ b/server/events/command_handler_test.go @@ -66,7 +66,7 @@ func TestExecuteCommand_LogPanics(t *testing.T) { setup(t) When(ghStatus.Update(fixtures.Repo, fixtures.Pull, vcs.Pending, nil, vcs.Github)).ThenPanic("panic") ch.ExecuteCommand(fixtures.Repo, fixtures.Repo, fixtures.User, 1, nil, vcs.Github) - _, _, comment, _ := vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments() + _, _, comment, _ := vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments() Assert(t, strings.Contains(comment, "Error: goroutine panic"), "comment should be about a goroutine panic") } @@ -125,7 +125,7 @@ func TestExecuteCommand_ClosedPull(t *testing.T) { When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, fixtures.Repo, nil) ch.ExecuteCommand(fixtures.Repo, fixtures.Repo, fixtures.User, fixtures.Pull.Num, nil, vcs.Github) - vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, modelPull, "Atlantis commands can't be run on closed pull requests", vcs.Github) + vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, modelPull.Num, "Atlantis commands can't be run on closed pull requests", vcs.Github) } func TestExecuteCommand_WorkspaceLocked(t *testing.T) { @@ -150,7 +150,7 @@ func TestExecuteCommand_WorkspaceLocked(t *testing.T) { ghStatus.VerifyWasCalledOnce().Update(fixtures.Repo, fixtures.Pull, vcs.Pending, &cmd, vcs.Github) _, response := ghStatus.VerifyWasCalledOnce().UpdateProjectResult(matchers.AnyPtrToEventsCommandContext(), matchers.AnyEventsCommandResponse()).GetCapturedArguments() Equals(t, msg, response.Failure) - vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, fixtures.Pull, + vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.Repo, fixtures.Pull.Num, "**Plan Failed**: "+msg+"\n\n", vcs.Github) } @@ -183,7 +183,7 @@ func TestExecuteCommand_FullRun(t *testing.T) { ghStatus.VerifyWasCalledOnce().Update(fixtures.Repo, fixtures.Pull, vcs.Pending, &cmd, vcs.Github) _, response := ghStatus.VerifyWasCalledOnce().UpdateProjectResult(matchers.AnyPtrToEventsCommandContext(), matchers.AnyEventsCommandResponse()).GetCapturedArguments() Equals(t, cmdResponse, response) - vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString(), matchers.AnyVcsHost()) + vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()) workspaceLocker.VerifyWasCalledOnce().Unlock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num) } } diff --git a/server/events/pull_closed_executor.go b/server/events/pull_closed_executor.go index 658e087073..7f3cd3d12d 100644 --- a/server/events/pull_closed_executor.go +++ b/server/events/pull_closed_executor.go @@ -64,7 +64,7 @@ func (p *PullClosedExecutor) CleanUpPull(repo models.Repo, pull models.PullReque if err = pullClosedTemplate.Execute(&buf, templateData); err != nil { return errors.Wrap(err, "rendering template for comment") } - return p.VCSClient.CreateComment(repo, pull, buf.String(), host) + return p.VCSClient.CreateComment(repo, pull.Num, buf.String(), host) } // buildTemplateData formats the lock data into a slice that can easily be diff --git a/server/events/pull_closed_executor_test.go b/server/events/pull_closed_executor_test.go index d89be41687..2e21c2eb1e 100644 --- a/server/events/pull_closed_executor_test.go +++ b/server/events/pull_closed_executor_test.go @@ -58,7 +58,7 @@ func TestCleanUpPullNoLocks(t *testing.T) { When(l.UnlockByPull(fixtures.Repo.FullName, fixtures.Pull.Num)).ThenReturn(nil, nil) err := pce.CleanUpPull(fixtures.Repo, fixtures.Pull, vcs.Github) Ok(t, err) - cp.VerifyWasCalled(Never()).CreateComment(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString(), matchers.AnyVcsHost()) + cp.VerifyWasCalled(Never()).CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()) } func TestCleanUpPullComments(t *testing.T) { @@ -139,7 +139,7 @@ func TestCleanUpPullComments(t *testing.T) { When(l.UnlockByPull(fixtures.Repo.FullName, fixtures.Pull.Num)).ThenReturn(c.Locks, nil) err := pce.CleanUpPull(fixtures.Repo, fixtures.Pull, vcs.Github) Ok(t, err) - _, _, comment, _ := cp.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments() + _, _, comment, _ := cp.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), matchers.AnyVcsHost()).GetCapturedArguments() expected := "Locks and plans deleted for the projects and workspaces modified in this pull request:\n\n" + c.Exp Equals(t, expected, comment) diff --git a/server/events/vcs/client.go b/server/events/vcs/client.go index 2e2db4de1d..d4e30eb7da 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -9,7 +9,7 @@ import ( // Client is used to make API calls to a VCS host like GitHub or GitLab. type Client interface { GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) - CreateComment(repo models.Repo, pull models.PullRequest, comment string) error + CreateComment(repo models.Repo, pullNum int, comment string) error PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) UpdateStatus(repo models.Repo, pull models.PullRequest, state CommitStatus, description string) error } diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index fbf4127789..6b344a091d 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -70,8 +70,8 @@ func (g *GithubClient) GetModifiedFiles(repo models.Repo, pull models.PullReques } // CreateComment creates a comment on the pull request. -func (g *GithubClient) CreateComment(repo models.Repo, pull models.PullRequest, comment string) error { - _, _, err := g.client.Issues.CreateComment(g.ctx, repo.Owner, repo.Name, pull.Num, &github.IssueComment{Body: &comment}) +func (g *GithubClient) CreateComment(repo models.Repo, pullNum int, comment string) error { + _, _, err := g.client.Issues.CreateComment(g.ctx, repo.Owner, repo.Name, pullNum, &github.IssueComment{Body: &comment}) return err } diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 50ad36becb..7e036d469b 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -48,8 +48,8 @@ func (g *GitlabClient) GetModifiedFiles(repo models.Repo, pull models.PullReques } // CreateComment creates a comment on the merge request. -func (g *GitlabClient) CreateComment(repo models.Repo, pull models.PullRequest, comment string) error { - _, _, err := g.Client.Notes.CreateMergeRequestNote(repo.FullName, pull.Num, &gitlab.CreateMergeRequestNoteOptions{Body: gitlab.String(comment)}) +func (g *GitlabClient) CreateComment(repo models.Repo, pullNum int, comment string) error { + _, _, err := g.Client.Notes.CreateMergeRequestNote(repo.FullName, pullNum, &gitlab.CreateMergeRequestNoteOptions{Body: gitlab.String(comment)}) return err } diff --git a/server/events/vcs/mocks/mock_proxy.go b/server/events/vcs/mocks/mock_proxy.go index efb20dff92..287757193e 100644 --- a/server/events/vcs/mocks/mock_proxy.go +++ b/server/events/vcs/mocks/mock_proxy.go @@ -35,8 +35,8 @@ func (mock *MockClientProxy) GetModifiedFiles(repo models.Repo, pull models.Pull return ret0, ret1 } -func (mock *MockClientProxy) CreateComment(repo models.Repo, pull models.PullRequest, comment string, host vcs.Host) error { - params := []pegomock.Param{repo, pull, comment, host} +func (mock *MockClientProxy) CreateComment(repo models.Repo, pullNum int, comment string, host vcs.Host) error { + params := []pegomock.Param{repo, pullNum, comment, host} result := pegomock.GetGenericMockFrom(mock).Invoke("CreateComment", params, []reflect.Type{reflect.TypeOf((*error)(nil)).Elem()}) var ret0 error if len(result) != 0 { @@ -128,8 +128,8 @@ func (c *ClientProxy_GetModifiedFiles_OngoingVerification) GetAllCapturedArgumen return } -func (verifier *VerifierClientProxy) CreateComment(repo models.Repo, pull models.PullRequest, comment string, host vcs.Host) *ClientProxy_CreateComment_OngoingVerification { - params := []pegomock.Param{repo, pull, comment, host} +func (verifier *VerifierClientProxy) CreateComment(repo models.Repo, pullNum int, comment string, host vcs.Host) *ClientProxy_CreateComment_OngoingVerification { + params := []pegomock.Param{repo, pullNum, comment, host} methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "CreateComment", params) return &ClientProxy_CreateComment_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } @@ -139,21 +139,21 @@ type ClientProxy_CreateComment_OngoingVerification struct { methodInvocations []pegomock.MethodInvocation } -func (c *ClientProxy_CreateComment_OngoingVerification) GetCapturedArguments() (models.Repo, models.PullRequest, string, vcs.Host) { - repo, pull, comment, host := c.GetAllCapturedArguments() - return repo[len(repo)-1], pull[len(pull)-1], comment[len(comment)-1], host[len(host)-1] +func (c *ClientProxy_CreateComment_OngoingVerification) GetCapturedArguments() (models.Repo, int, string, vcs.Host) { + repo, pullNum, comment, host := c.GetAllCapturedArguments() + return repo[len(repo)-1], pullNum[len(pullNum)-1], comment[len(comment)-1], host[len(host)-1] } -func (c *ClientProxy_CreateComment_OngoingVerification) GetAllCapturedArguments() (_param0 []models.Repo, _param1 []models.PullRequest, _param2 []string, _param3 []vcs.Host) { +func (c *ClientProxy_CreateComment_OngoingVerification) GetAllCapturedArguments() (_param0 []models.Repo, _param1 []int, _param2 []string, _param3 []vcs.Host) { params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) if len(params) > 0 { _param0 = make([]models.Repo, len(params[0])) for u, param := range params[0] { _param0[u] = param.(models.Repo) } - _param1 = make([]models.PullRequest, len(params[1])) + _param1 = make([]int, len(params[1])) for u, param := range params[1] { - _param1[u] = param.(models.PullRequest) + _param1[u] = param.(int) } _param2 = make([]string, len(params[2])) for u, param := range params[2] { diff --git a/server/events/vcs/not_configured_vcs_client.go b/server/events/vcs/not_configured_vcs_client.go index 7e02cba665..dd240f4b68 100644 --- a/server/events/vcs/not_configured_vcs_client.go +++ b/server/events/vcs/not_configured_vcs_client.go @@ -16,7 +16,7 @@ type NotConfiguredVCSClient struct { func (a *NotConfiguredVCSClient) GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) { return nil, a.err() } -func (a *NotConfiguredVCSClient) CreateComment(repo models.Repo, pull models.PullRequest, comment string) error { +func (a *NotConfiguredVCSClient) CreateComment(repo models.Repo, pullNum int, comment string) error { return a.err() } func (a *NotConfiguredVCSClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) { diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index 0ca3e2ec76..ee778b9093 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -11,7 +11,7 @@ import ( // VCS host is required. type ClientProxy interface { GetModifiedFiles(repo models.Repo, pull models.PullRequest, host Host) ([]string, error) - CreateComment(repo models.Repo, pull models.PullRequest, comment string, host Host) error + CreateComment(repo models.Repo, pullNum int, comment string, host Host) error PullIsApproved(repo models.Repo, pull models.PullRequest, host Host) (bool, error) UpdateStatus(repo models.Repo, pull models.PullRequest, state CommitStatus, description string, host Host) error } @@ -48,12 +48,12 @@ func (d *DefaultClientProxy) GetModifiedFiles(repo models.Repo, pull models.Pull return nil, invalidVCSErr } -func (d *DefaultClientProxy) CreateComment(repo models.Repo, pull models.PullRequest, comment string, host Host) error { +func (d *DefaultClientProxy) CreateComment(repo models.Repo, pullNum int, comment string, host Host) error { switch host { case Github: - return d.GithubClient.CreateComment(repo, pull, comment) + return d.GithubClient.CreateComment(repo, pullNum, comment) case Gitlab: - return d.GitlabClient.CreateComment(repo, pull, comment) + return d.GitlabClient.CreateComment(repo, pullNum, comment) } return invalidVCSErr } From ab1d36d2c8585de12109c707229d18e55ee4fbee Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Tue, 27 Feb 2018 17:34:08 -0800 Subject: [PATCH 3/6] Comment back on pull request sooner on error. - Refactor EventParser so it returns a comment we can send to the pull request when a bad command or help command is commented. - Remove now unneeded HelpExecutor because we comment right from the EventsController now - Use pflag package to parse commands instead of doing it manually - Comment back when user types terraform instead of atlantis --- server/events/command_handler.go | 3 - server/events/command_handler_test.go | 9 +- server/events/command_name.go | 3 - server/events/event_parser.go | 150 ++++++++++--- server/events/event_parser_test.go | 212 +++++++++++++----- server/events/help_executor.go | 11 - server/events/help_executor_test.go | 15 -- server/events/markdown_renderer.go | 28 --- .../matchers/events_commandparseresult.go | 20 ++ server/events/mocks/mock_event_parsing.go | 14 +- server/events_controller.go | 45 ++-- server/events_controller_test.go | 88 +++++--- server/server.go | 3 +- 13 files changed, 384 insertions(+), 217 deletions(-) delete mode 100644 server/events/help_executor.go delete mode 100644 server/events/help_executor_test.go create mode 100644 server/events/mocks/matchers/events_commandparseresult.go diff --git a/server/events/command_handler.go b/server/events/command_handler.go index 1ff9477c15..8658cc9a1d 100644 --- a/server/events/command_handler.go +++ b/server/events/command_handler.go @@ -42,7 +42,6 @@ type GitlabMergeRequestGetter interface { type CommandHandler struct { PlanExecutor Executor ApplyExecutor Executor - HelpExecutor Executor LockURLGenerator LockURLGenerator VCSClient vcs.ClientProxy GithubPullGetter GithubPullGetter @@ -148,8 +147,6 @@ func (c *CommandHandler) run(ctx *CommandContext) { cr = c.PlanExecutor.Execute(ctx) case Apply: cr = c.ApplyExecutor.Execute(ctx) - case Help: - cr = c.HelpExecutor.Execute(ctx) default: ctx.Log.Err("failed to determine desired command, neither plan nor apply") } diff --git a/server/events/command_handler_test.go b/server/events/command_handler_test.go index 1a8ee83b52..a583c28021 100644 --- a/server/events/command_handler_test.go +++ b/server/events/command_handler_test.go @@ -21,7 +21,6 @@ import ( ) var applier *mocks.MockExecutor -var helper *mocks.MockExecutor var planner *mocks.MockExecutor var eventParsing *mocks.MockEventParsing var vcsClient *vcsmocks.MockClientProxy @@ -35,7 +34,6 @@ var logBytes *bytes.Buffer func setup(t *testing.T) { RegisterMockTestingT(t) applier = mocks.NewMockExecutor() - helper = mocks.NewMockExecutor() planner = mocks.NewMockExecutor() eventParsing = mocks.NewMockEventParsing() ghStatus = mocks.NewMockCommitStatusUpdater() @@ -49,7 +47,6 @@ func setup(t *testing.T) { ch = events.CommandHandler{ PlanExecutor: planner, ApplyExecutor: applier, - HelpExecutor: helper, VCSClient: vcsClient, CommitStatusUpdater: ghStatus, EventParser: eventParsing, @@ -155,12 +152,12 @@ func TestExecuteCommand_WorkspaceLocked(t *testing.T) { } func TestExecuteCommand_FullRun(t *testing.T) { - t.Log("when running a plan, apply or help should comment") + t.Log("when running a plan, apply should comment") pull := &github.PullRequest{ State: github.String("closed"), } cmdResponse := events.CommandResponse{} - for _, c := range []events.CommandName{events.Help, events.Plan, events.Apply} { + for _, c := range []events.CommandName{events.Plan, events.Apply} { setup(t) cmd := events.Command{ Name: c, @@ -170,8 +167,6 @@ func TestExecuteCommand_FullRun(t *testing.T) { When(eventParsing.ParseGithubPull(pull)).ThenReturn(fixtures.Pull, fixtures.Repo, nil) When(workspaceLocker.TryLock(fixtures.Repo.FullName, cmd.Workspace, fixtures.Pull.Num)).ThenReturn(true) switch c { - case events.Help: - When(helper.Execute(matchers.AnyPtrToEventsCommandContext())).ThenReturn(cmdResponse) case events.Plan: When(planner.Execute(matchers.AnyPtrToEventsCommandContext())).ThenReturn(cmdResponse) case events.Apply: diff --git a/server/events/command_name.go b/server/events/command_name.go index 63d75a6ceb..529811f6c3 100644 --- a/server/events/command_name.go +++ b/server/events/command_name.go @@ -6,7 +6,6 @@ type CommandName int const ( Apply CommandName = iota Plan - Help // Adding more? Don't forget to update String() below ) @@ -17,8 +16,6 @@ func (c CommandName) String() string { return "apply" case Plan: return "plan" - case Help: - return "help" } return "" } diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 36b2737d1d..cb424b73d7 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -3,7 +3,9 @@ package events import ( "errors" "fmt" + "io/ioutil" "path/filepath" + "regexp" "strings" "github.com/google/go-github/github" @@ -14,6 +16,11 @@ import ( ) const gitlabPullOpened = "opened" +const usagesCols = 90 + +// multiLineRegex is used to ignore multi-line comments since those aren't valid +// Atlantis commands. +var multiLineRegex = regexp.MustCompile(`.*\r?\n.+`) //go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_event_parsing.go EventParsing @@ -29,7 +36,7 @@ type Command struct { } type EventParsing interface { - DetermineCommand(comment string, vcsHost vcs.Host) (*Command, error) + DetermineCommand(comment string, vcsHost vcs.Host) CommandParseResult ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pullNum int, err error) ParseGithubPull(pull *github.PullRequest) (models.PullRequest, models.Repo, error) ParseGithubRepo(ghRepo *github.Repository) (models.Repo, error) @@ -45,41 +52,81 @@ type EventParser struct { GitlabToken string } -// DetermineCommand parses the comment as an atlantis command. If it succeeds, -// it returns the command. Otherwise it returns error. +// CommandParseResult describes the result of parsing a comment as a command. +type CommandParseResult struct { + // Command is the successfully parsed command. Will be nil if + // CommentResponse or Ignore is set. + Command *Command + // CommentResponse is set when we should respond immediately to the command + // for example for atlantis help. + CommentResponse string + // Ignore is set to true when we should just ignore this comment. + Ignore bool +} + +// DetermineCommand parses the comment as an Atlantis command. +// +// Valid commands contain: +// - The initial "executable" name, 'run' or 'atlantis' or '@GithubUser' +// where GithubUser is the API user Atlantis is running as. +// - Then a command, either 'plan', 'apply', or 'help'. +// - Then optional flags, then an optional separator '--' followed by optional +// extra flags to be appended to the terraform plan/apply command. +// +// Examples: +// - atlantis help +// - run plan +// - @GithubUser plan -w staging +// - atlantis plan -w staging -d dir --verbose +// - atlantis plan --verbose -- -key=value -key2 value2 +// // nolint: gocyclo -func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Command, error) { - // valid commands contain: - // the initial "executable" name, 'run' or 'atlantis' or '@GithubUser' where GithubUser is the api user atlantis is running as - // then a command, either 'plan', 'apply', or 'help' - // then an optional workspace argument, an optional --verbose flag and any other flags - // - // examples: - // atlantis help - // run plan - // @GithubUser plan staging - // atlantis plan staging --verbose - // atlantis plan staging --verbose -key=value -key2 value2 - err := errors.New("not an Atlantis command") +func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) CommandParseResult { + if multiLineRegex.MatchString(comment) { + return CommandParseResult{Ignore: true} + } + + // strings.Fields strips out newlines but that's okay since we've removed + // multiline strings above. args := strings.Fields(comment) - if len(args) < 2 { - return nil, err + if len(args) < 1 { + return CommandParseResult{Ignore: true} } + // Helpfully warn the user if they're using "terraform" instead of "atlantis" + if args[0] == "terraform" { + return CommandParseResult{CommentResponse: DidYouMeanAtlantisComment} + } + + // Atlantis can be invoked using the name of the VCS host user we're + // running under. Need to be able to match against that user. vcsUser := e.GithubUser if vcsHost == vcs.Gitlab { vcsUser = e.GitlabUser } - if !e.stringInSlice(args[0], []string{"run", "atlantis", "@" + vcsUser}) { - return nil, err - } - if !e.stringInSlice(args[1], []string{"plan", "apply", "help", "-help", "--help"}) { - return nil, err + executableNames := []string{"run", "atlantis", "@" + vcsUser} + + // If the comment doesn't start with the name of our 'executable' then + // ignore it. + if !e.stringInSlice(args[0], executableNames) { + return CommandParseResult{Ignore: true} } + // If they've just typed the name of the executable then give them the help + // output. + if len(args) == 1 { + return CommandParseResult{CommentResponse: HelpComment} + } command := args[1] - if command == "help" || command == "-help" || command == "--help" { - return &Command{Name: Help}, nil + + // Help output. + if e.stringInSlice(command, []string{"help", "-h", "--help"}) { + return CommandParseResult{CommentResponse: HelpComment} + } + + // Need to have a plan or apply at this point. + if !e.stringInSlice(command, []string{"plan", "apply"}) { + return CommandParseResult{CommentResponse: fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", command)} } var workspace string @@ -91,25 +138,33 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma // Set up the flag parsing depending on the command. const defaultWorkspace = "default" - if command == "plan" { + switch command { + case "plan": name = Plan flagSet = pflag.NewFlagSet("plan", pflag.ContinueOnError) - flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, fmt.Sprintf("Switch to this Terraform workspace before planning. Defaults to '%s'", defaultWorkspace)) + flagSet.SetOutput(ioutil.Discard) + flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Switch to this Terraform workspace before planning.") flagSet.StringVarP(&dir, "dir", "d", "", "Which directory to run plan in relative to root of repo. Use '.' for root. If not specified, will attempt to run plan for all Terraform projects we think were modified in this changeset.") flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") - } else if command == "apply" { + case "apply": name = Apply flagSet = pflag.NewFlagSet("apply", pflag.ContinueOnError) - flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, fmt.Sprintf("Apply the plan for this Terraform workspace. Defaults to '%s'", defaultWorkspace)) + flagSet.SetOutput(ioutil.Discard) + flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Apply the plan for this Terraform workspace.") flagSet.StringVarP(&dir, "dir", "d", "", "Apply the plan for this directory, relative to root of repo. Use '.' for root. If not specified, will run apply against all plans created for this workspace.") flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") - } else { - return nil, fmt.Errorf("unknown command %q – this is a bug", command) + default: + return CommandParseResult{CommentResponse: fmt.Sprintf("Error: unknown command %q – this is a bug", command)} } // Now parse the flags. - if err := flagSet.Parse(args[2:]); err != nil { - return nil, err + // It's safe to use [2:] because we know there's at least 2 elements in args. + err := flagSet.Parse(args[2:]) + if err == pflag.ErrHelp { + return CommandParseResult{CommentResponse: fmt.Sprintf("```\nUsage of %s:\n%s\n```", command, flagSet.FlagUsagesWrapped(usagesCols))} + } + if err != nil { + return CommandParseResult{CommentResponse: fmt.Sprintf("```\nError: %s.\nUsage of %s:\n%s\n```", err.Error(), command, flagSet.FlagUsagesWrapped(usagesCols))} } // We only use the extra args after the --. For example given a comment: // "atlantis plan -bad-option -- -target=hi" @@ -135,7 +190,7 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma validatedDir = filepath.Clean(validatedDir) // Detect relative dirs since they're not allowed. if strings.HasPrefix(validatedDir, "..") { - return nil, fmt.Errorf("relative path %q not allowed", dir) + return CommandParseResult{CommentResponse: fmt.Sprintf("Error: Using a relative path %q with -d/--dir is not allowed", dir)} } dir = validatedDir @@ -143,11 +198,12 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma // Because we use the workspace name as a file, need to make sure it's // not doing something weird like being a relative dir. if strings.Contains(workspace, "..") { - return nil, errors.New("workspace can't contain '..'") + return CommandParseResult{CommentResponse: "Error: Value for -w/--workspace can't contain '..'"} } - c := &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs} - return c, nil + return CommandParseResult{ + Command: &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs}, + } } func (e *EventParser) ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pullNum int, err error) { @@ -349,3 +405,23 @@ func (e *EventParser) stringInSlice(a string, list []string) bool { } return false } + +var HelpComment = "```cmake\n" + + `atlantis +Terraform automation and collaboration for your team + +Usage: + atlantis [options] + +Commands: + plan Runs 'terraform plan' for the changes in this pull request. + apply Runs 'terraform apply' on the plans generated by 'atlantis plan'. + help View help. + +Flags: + -h, --help help for atlantis + +Use "atlantis [command] --help" for more information about a command. +` + +var DidYouMeanAtlantisComment = "Did you mean to use `atlantis` instead of `terraform`?" diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index 2186416826..aa45f01f89 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -24,70 +24,164 @@ var parser = events.EventParser{ GitlabToken: "gitlab-token", } -func TestDetermineCommandInvalid(t *testing.T) { - t.Log("given an invalid comment, should return an error") - comments := []string{ - // just the executable, no command +func TestDetermineCommand_Ignored(t *testing.T) { + t.Log("given a comment that should be ignored we should set " + + "CommandParseResult.Ignore to true") + ignoreComments := []string{ + "", + "a", + "abc", + "atlantis plan\nbut with newlines", + "terraform plan\nbut with newlines", + } + for _, c := range ignoreComments { + r := parser.DetermineCommand(c, vcs.Github) + Assert(t, r.Ignore, "expected Ignore to be true for comment %q", c) + } +} + +func TestDetermineCommand_HelpResponse(t *testing.T) { + t.Log("given a comment that should result in help output we " + + "should set CommandParseResult.CommentResult") + helpComments := []string{ "run", "atlantis", "@github-user", - // invalid command - "run slkjd", - "atlantis slkjd", - "@github-user slkjd", - "atlantis plans", - // relative dirs - "atlantis plan -d ..", - "atlantis plan -d ../", - "atlantis plan -d a/../../", - // using .. in workspace - "atlantis plan -w a..", - "atlantis plan -w ../", - "atlantis plan -w ..", - "atlantis plan -w a/../b", - // misc - "related comment mentioning atlantis", + "atlantis help", + "atlantis --help", + "atlantis -h", + "atlantis help something else", + "atlantis help plan", + } + for _, c := range helpComments { + r := parser.DetermineCommand(c, vcs.Github) + Equals(t, events.HelpComment, r.CommentResponse) + } +} + +func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { + t.Log("given a comment that should result in a 'did you mean atlantis'" + + "response, should set CommandParseResult.CommentResult") + comments := []string{ + "terraform", + "terraform help", + "terraform --help", + "terraform -h", + "terraform plan", + "terraform apply", + "terraform plan -w workspace -d . -- test", } for _, c := range comments { - _, e := parser.DetermineCommand(c, vcs.Github) - Assert(t, e != nil, "expected error for comment: "+c) + r := parser.DetermineCommand(c, vcs.Github) + Assert(t, r.CommentResponse == events.DidYouMeanAtlantisComment, + "For comment %q expected CommentResponse==%q but got %q", c, events.DidYouMeanAtlantisComment, r.CommentResponse) } } -func TestDetermineCommand_ExecutableNames(t *testing.T) { - t.Log("should be allowed to use different executable names in the comments") - parsed, err := parser.DetermineCommand("atlantis plan", vcs.Github) - Ok(t, err) - Equals(t, events.Plan, parsed.Name) +func TestDetermineCommand_InvalidCommand(t *testing.T) { + t.Log("given a comment with an invalid atlantis command, should return " + + "a warning.") + comments := []string{ + "atlantis paln", + "atlantis Plan", + "atlantis appely apply", + } + for _, c := range comments { + r := parser.DetermineCommand(c, vcs.Github) + exp := fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", strings.Fields(c)[1]) + Assert(t, r.CommentResponse == exp, + "For comment %q expected CommentResponse==%q but got %q", c, exp, r.CommentResponse) + } +} - parsed, err = parser.DetermineCommand("run plan", vcs.Github) - Ok(t, err) - Equals(t, events.Plan, parsed.Name) +func TestDetermineCommand_SubcommandUsage(t *testing.T) { + t.Log("given a comment asking for the usage of a subcommand should " + + "return help") + comments := []string{ + "atlantis plan -h", + "atlantis plan --help", + "atlantis apply -h", + "atlantis apply --help", + } + for _, c := range comments { + r := parser.DetermineCommand(c, vcs.Github) + exp := "Usage of " + strings.Fields(c)[1] + Assert(t, strings.Contains(r.CommentResponse, exp), + "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) + Assert(t, !strings.Contains(r.CommentResponse, "Error:"), + "For comment %q expected CommentResponse %q to not contain %q", c, r.CommentResponse, "Error: ") + } +} - parsed, err = parser.DetermineCommand("@github-user plan", vcs.Github) - Ok(t, err) - Equals(t, events.Plan, parsed.Name) +func TestDetermineCommand_InvalidFlags(t *testing.T) { + t.Log("given a comment with a valid atlantis command but invalid" + + " flags, should return a warning and the proper usage") + cases := []struct { + comment string + exp string + }{ + { + "atlantis plan -e", + "Error: unknown shorthand flag: 'e' in -e", + }, + { + "atlantis plan --abc", + "Error: unknown flag: --abc", + }, + { + "atlantis apply -e", + "Error: unknown shorthand flag: 'e' in -e", + }, + { + "atlantis apply --abc", + "Error: unknown flag: --abc", + }, + } + for _, c := range cases { + r := parser.DetermineCommand(c.comment, vcs.Github) + Assert(t, strings.Contains(r.CommentResponse, c.exp), + "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, c.exp) + Assert(t, strings.Contains(r.CommentResponse, "Usage of "), + "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, "Usage of ") + } +} - parsed, err = parser.DetermineCommand("@gitlab-user plan", vcs.Gitlab) - Ok(t, err) - Equals(t, events.Plan, parsed.Name) +func TestDetermineCommand_RelativeDirPath(t *testing.T) { + t.Log("if -d is used with a relative path, should return an error") + comments := []string{ + "atlantis plan -d ..", + "atlantis apply -d ..", + // These won't return an error because we prepend with . when parsing. + //"atlantis plan -d /..", + //"atlantis apply -d /..", + "atlantis plan -d ./..", + "atlantis apply -d ./..", + "atlantis plan -d a/b/../../..", + "atlantis apply -d a/../..", + } + for _, c := range comments { + r := parser.DetermineCommand(c, vcs.Github) + exp := "Error: Using a relative path" + Assert(t, strings.Contains(r.CommentResponse, exp), + "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) + } } -func TestDetermineCommand_Help(t *testing.T) { - t.Log("given a help comment, should match") - helpArgs := []string{ - "help", - "-help", - "--help", - "help -verbose", - "help --hi", - "help somethingelse", +func TestDetermineCommand_InvalidWorkspace(t *testing.T) { + t.Log("if -w is used with '..', should return an error") + comments := []string{ + "atlantis plan -w ..", + "atlantis apply -w ..", + "atlantis plan -w ..abc", + "atlantis apply -w abc..", + "atlantis plan -w abc..abc", + "atlantis apply -w ../../../etc/passwd", } - for _, arg := range helpArgs { - comment := fmt.Sprintf("atlantis %s", arg) - command, err := parser.DetermineCommand(comment, vcs.Github) - Assert(t, err == nil, "did not parse comment %q as help command, got err: %s", comment, err) - Assert(t, command.Name == events.Help, "did not parse comment %q as help command", comment) + for _, c := range comments { + r := parser.DetermineCommand(c, vcs.Github) + exp := "Error: Value for -w/--workspace can't contain '..'" + Assert(t, r.CommentResponse == exp, + "For comment %q expected CommentResponse %q to be %q", c, r.CommentResponse, exp) } } @@ -251,18 +345,18 @@ func TestDetermineCommand_Parsing(t *testing.T) { for _, test := range cases { for _, cmdName := range []string{"plan", "apply"} { comment := fmt.Sprintf("atlantis %s %s", cmdName, test.flags) - t.Logf("testing comment: %s", comment) - cmd, err := parser.DetermineCommand(comment, vcs.Github) - Assert(t, err == nil, "unexpected err parsing %q: %s", comment, err) - Equals(t, test.expDir, cmd.Dir) - Equals(t, test.expWorkspace, cmd.Workspace) - Equals(t, test.expVerbose, cmd.Verbose) - Equals(t, test.expExtraArgs, strings.Join(cmd.Flags, " ")) + r := parser.DetermineCommand(comment, vcs.Github) + Assert(t, r.CommentResponse == "", "CommentResponse should have been empty but was %q for comment %q", r.CommentResponse, comment) + Assert(t, test.expDir == r.Command.Dir, "exp dir to equal %q but was %q for comment %q", test.expDir, r.Command.Dir, comment) + Assert(t, test.expWorkspace == r.Command.Workspace, "exp workspace to equal %q but was %q for comment %q", test.expWorkspace, r.Command.Workspace, comment) + Assert(t, test.expVerbose == r.Command.Verbose, "exp verbose to equal %v but was %v for comment %q", test.expVerbose, r.Command.Verbose, comment) + actExtraArgs := strings.Join(r.Command.Flags, " ") + Assert(t, test.expExtraArgs == actExtraArgs, "exp extra args to equal %v but got %v for comment %q", test.expExtraArgs, actExtraArgs, comment) if cmdName == "plan" { - Assert(t, cmd.Name == events.Plan, "did not parse comment %q as plan command", comment) + Assert(t, r.Command.Name == events.Plan, "did not parse comment %q as plan command", comment) } if cmdName == "apply" { - Assert(t, cmd.Name == events.Apply, "did not parse comment %q as apply command", comment) + Assert(t, r.Command.Name == events.Apply, "did not parse comment %q as apply command", comment) } } } diff --git a/server/events/help_executor.go b/server/events/help_executor.go deleted file mode 100644 index 74cbd2e4e2..0000000000 --- a/server/events/help_executor.go +++ /dev/null @@ -1,11 +0,0 @@ -package events - -// HelpExecutor executes the help command. -type HelpExecutor struct{} - -// Execute executes the help command. -func (h *HelpExecutor) Execute(ctx *CommandContext) CommandResponse { - // We don't actually need to do anything since the comment renderer - // will see that it is a help command and render the help comment. - return CommandResponse{} -} diff --git a/server/events/help_executor_test.go b/server/events/help_executor_test.go deleted file mode 100644 index a1c6243116..0000000000 --- a/server/events/help_executor_test.go +++ /dev/null @@ -1,15 +0,0 @@ -package events_test - -import ( - "testing" - - "github.com/runatlantis/atlantis/server/events" - . "github.com/runatlantis/atlantis/testing" -) - -func TestExecute(t *testing.T) { - h := events.HelpExecutor{} - ctx := events.CommandContext{} - r := h.Execute(&ctx) - Equals(t, events.CommandResponse{}, r) -} diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index 1841068606..704ad58713 100644 --- a/server/events/markdown_renderer.go +++ b/server/events/markdown_renderer.go @@ -38,9 +38,6 @@ type ResultData struct { // Render formats the data into a markdown string. // nolint: interfacer func (g *MarkdownRenderer) Render(res CommandResponse, cmdName CommandName, log string, verbose bool) string { - if cmdName == Help { - return g.renderTemplate(helpTmpl, nil) - } commandStr := strings.Title(cmdName.String()) common := CommonData{commandStr, verbose, log} if res.Error != nil { @@ -97,31 +94,6 @@ func (g *MarkdownRenderer) renderTemplate(tmpl *template.Template, data interfac return buf.String() } -var helpTmpl = template.Must(template.New("").Parse("```cmake\n" + - `atlantis - Terraform collaboration tool that enables you to collaborate on infrastructure -safely and securely. - -Usage: atlantis [workspace] [--verbose] - -Commands: -plan Runs 'terraform plan' on the files changed in the pull request -apply Runs 'terraform apply' using the plans generated by 'atlantis plan' -help Get help - -Examples: - -# Generates a plan for staging workspace -atlantis plan staging - -# Generates a plan for a standalone terraform project -atlantis plan - -# Applies a plan for staging workspace -atlantis apply staging - -# Applies a plan for a standalone terraform project -atlantis apply -`)) var singleProjectTmpl = template.Must(template.New("").Parse("{{ range $result := .Results }}{{$result}}{{end}}\n" + logTmpl)) var multiProjectTmpl = template.Must(template.New("").Parse( "Ran {{.Command}} in {{ len .Results }} directories:\n" + diff --git a/server/events/mocks/matchers/events_commandparseresult.go b/server/events/mocks/matchers/events_commandparseresult.go new file mode 100644 index 0000000000..4b3607195a --- /dev/null +++ b/server/events/mocks/matchers/events_commandparseresult.go @@ -0,0 +1,20 @@ +package matchers + +import ( + "reflect" + + "github.com/petergtz/pegomock" + events "github.com/runatlantis/atlantis/server/events" +) + +func AnyEventsCommandParseResult() events.CommandParseResult { + pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(events.CommandParseResult))(nil)).Elem())) + var nullValue events.CommandParseResult + return nullValue +} + +func EqEventsCommandParseResult(value events.CommandParseResult) events.CommandParseResult { + pegomock.RegisterMatcher(&pegomock.EqMatcher{Value: value}) + var nullValue events.CommandParseResult + return nullValue +} diff --git a/server/events/mocks/mock_event_parsing.go b/server/events/mocks/mock_event_parsing.go index 64f6b11ab7..b4c7898de1 100644 --- a/server/events/mocks/mock_event_parsing.go +++ b/server/events/mocks/mock_event_parsing.go @@ -22,20 +22,16 @@ func NewMockEventParsing() *MockEventParsing { return &MockEventParsing{fail: pegomock.GlobalFailHandler} } -func (mock *MockEventParsing) DetermineCommand(comment string, vcsHost vcs.Host) (*events.Command, error) { +func (mock *MockEventParsing) DetermineCommand(comment string, vcsHost vcs.Host) events.CommandParseResult { params := []pegomock.Param{comment, vcsHost} - result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineCommand", params, []reflect.Type{reflect.TypeOf((**events.Command)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) - var ret0 *events.Command - var ret1 error + result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineCommand", params, []reflect.Type{reflect.TypeOf((*events.CommandParseResult)(nil)).Elem()}) + var ret0 events.CommandParseResult if len(result) != 0 { if result[0] != nil { - ret0 = result[0].(*events.Command) - } - if result[1] != nil { - ret1 = result[1].(error) + ret0 = result[0].(events.CommandParseResult) } } - return ret0, ret1 + return ret0 } func (mock *MockEventParsing) ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) (models.Repo, models.User, int, error) { diff --git a/server/events_controller.go b/server/events_controller.go index 1bfda73721..9cb75e2854 100644 --- a/server/events_controller.go +++ b/server/events_controller.go @@ -35,6 +35,7 @@ type EventsController struct { // SupportedVCSHosts is which VCS hosts Atlantis was configured upon // startup to support. SupportedVCSHosts []vcs.Host + VCSClient vcs.ClientProxy } // Post handles POST webhook requests. @@ -91,17 +92,11 @@ func (e *EventsController) HandleGithubCommentEvent(w http.ResponseWriter, event return } - command, err := e.Parser.DetermineCommand(event.Comment.GetBody(), vcs.Github) - if err != nil { - e.respond(w, logging.Debug, http.StatusOK, "Ignoring: %s %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. - fmt.Fprintln(w, "Processing...") - go e.CommandRunner.ExecuteCommand(baseRepo, models.Repo{}, user, pullNum, command, vcs.Github) + // We pass in an empty models.Repo for headRepo because we need to do additional + // calls to get that information but we need this code path to be generic. + // Later on in CommandHandler we detect that this is a GitHub event and + // make the necessary calls to get the headRepo. + e.handleCommentEvent(w, baseRepo, models.Repo{}, user, pullNum, event.Comment.GetBody(), vcs.Github) } // HandleGithubPullRequestEvent will delete any locks associated with the pull @@ -152,9 +147,29 @@ func (e *EventsController) handleGitlabPost(w http.ResponseWriter, r *http.Reque // commands can come from. It's exported to make testing easier. func (e *EventsController) HandleGitlabCommentEvent(w http.ResponseWriter, event gitlab.MergeCommentEvent) { baseRepo, headRepo, user := e.Parser.ParseGitlabMergeCommentEvent(event) - command, err := e.Parser.DetermineCommand(event.ObjectAttributes.Note, vcs.Gitlab) - if err != nil { - e.respond(w, logging.Debug, http.StatusOK, "Ignoring: %s", err) + e.handleCommentEvent(w, baseRepo, headRepo, user, event.MergeRequest.IID, event.ObjectAttributes.Note, vcs.Gitlab) +} + +func (e *EventsController) handleCommentEvent(w http.ResponseWriter, baseRepo models.Repo, headRepo models.Repo, user models.User, pullNum int, comment string, vcsHost vcs.Host) { + parseResult := e.Parser.DetermineCommand(comment, vcsHost) + if parseResult.Ignore { + truncated := comment + if len(truncated) > 40 { + truncated = comment[:40] + "..." + } + e.respond(w, logging.Debug, http.StatusOK, "Ignoring non-command comment: %q", truncated) + return + } + + // If the command isn't valid or doesn't require processing, ex. + // "atlantis help" then we just comment back immediately. + // We do this here rather than earlier because we need access to the pull + // variable to comment back on the pull request. + if parseResult.CommentResponse != "" { + if err := e.VCSClient.CreateComment(baseRepo, pullNum, parseResult.CommentResponse, vcsHost); err != nil { + e.Logger.Err("unable to comment on pull request: %s", err) + } + e.respond(w, logging.Info, http.StatusOK, "Commenting back on pull request") return } @@ -162,7 +177,7 @@ func (e *EventsController) HandleGitlabCommentEvent(w http.ResponseWriter, event // We use a goroutine so that this function returns and the connection is // closed. fmt.Fprintln(w, "Processing...") - go e.CommandRunner.ExecuteCommand(baseRepo, headRepo, user, event.MergeRequest.IID, command, vcs.Gitlab) + go e.CommandRunner.ExecuteCommand(baseRepo, headRepo, user, pullNum, parseResult.Command, vcsHost) } // HandleGitlabMergeRequestEvent will delete any locks associated with the pull diff --git a/server/events_controller_test.go b/server/events_controller_test.go index e16a1e7f11..97d95fbf28 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -16,6 +16,7 @@ import ( "github.com/runatlantis/atlantis/server/events/mocks/matchers" "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/events/vcs" + vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks" "github.com/runatlantis/atlantis/server/logging" "github.com/runatlantis/atlantis/server/mocks" ) @@ -28,7 +29,7 @@ var eventsReq *http.Request func TestPost_NotGithubOrGitlab(t *testing.T) { t.Log("when the request is not for gitlab or github a 400 is returned") - e, _, _, _, _, _ := setup(t) + e, _, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusBadRequest, "Ignoring request") @@ -36,7 +37,7 @@ func TestPost_NotGithubOrGitlab(t *testing.T) { func TestPost_UnsupportedVCSGithub(t *testing.T) { t.Log("when the request is for an unsupported vcs a 400 is returned") - e, _, _, _, _, _ := setup(t) + e, _, _, _, _, _, _ := setup(t) e.SupportedVCSHosts = nil eventsReq.Header.Set(githubHeader, "value") w := httptest.NewRecorder() @@ -46,7 +47,7 @@ func TestPost_UnsupportedVCSGithub(t *testing.T) { func TestPost_UnsupportedVCSGitlab(t *testing.T) { t.Log("when the request is for an unsupported vcs a 400 is returned") - e, _, _, _, _, _ := setup(t) + e, _, _, _, _, _, _ := setup(t) e.SupportedVCSHosts = nil eventsReq.Header.Set(gitlabHeader, "value") w := httptest.NewRecorder() @@ -56,7 +57,7 @@ func TestPost_UnsupportedVCSGitlab(t *testing.T) { func TestPost_InvalidGithubSecret(t *testing.T) { t.Log("when the github payload can't be validated a 400 is returned") - e, v, _, _, _, _ := setup(t) + e, v, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(githubHeader, "value") When(v.Validate(eventsReq, secret)).ThenReturn(nil, errors.New("err")) @@ -66,7 +67,7 @@ func TestPost_InvalidGithubSecret(t *testing.T) { func TestPost_InvalidGitlabSecret(t *testing.T) { t.Log("when the gitlab payload can't be validated a 400 is returned") - e, _, gl, _, _, _ := setup(t) + e, _, gl, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(nil, errors.New("err")) @@ -76,7 +77,7 @@ func TestPost_InvalidGitlabSecret(t *testing.T) { func TestPost_UnsupportedGithubEvent(t *testing.T) { t.Log("when the event type is an unsupported github event we ignore it") - e, v, _, _, _, _ := setup(t) + e, v, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(githubHeader, "value") When(v.Validate(eventsReq, nil)).ThenReturn([]byte(`{"not an event": ""}`), nil) @@ -86,7 +87,7 @@ func TestPost_UnsupportedGithubEvent(t *testing.T) { func TestPost_UnsupportedGitlabEvent(t *testing.T) { t.Log("when the event type is an unsupported gitlab event we ignore it") - e, _, gl, _, _, _ := setup(t) + e, _, gl, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn([]byte(`{"not an event": ""}`), nil) @@ -96,7 +97,7 @@ func TestPost_UnsupportedGitlabEvent(t *testing.T) { func TestPost_GithubCommentNotCreated(t *testing.T) { t.Log("when the event is a github comment but it's not a created event we ignore it") - e, v, _, _, _, _ := setup(t) + e, v, _, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") // comment action is deleted, not created event := `{"action": "deleted"}` @@ -108,7 +109,7 @@ func TestPost_GithubCommentNotCreated(t *testing.T) { func TestPost_GithubInvalidComment(t *testing.T) { t.Log("when the event is a github comment without all expected data we return a 400") - e, v, _, p, _, _ := setup(t) + e, v, _, p, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) @@ -120,31 +121,60 @@ func TestPost_GithubInvalidComment(t *testing.T) { func TestPost_GitlabCommentInvalidCommand(t *testing.T) { t.Log("when the event is a gitlab comment with an invalid command we ignore it") - e, _, gl, p, _, _ := setup(t) + e, _, gl, p, _, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(p.DetermineCommand("", vcs.Gitlab)).ThenReturn(nil, errors.New("err")) + When(p.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommandParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) - responseContains(t, w, http.StatusOK, "Ignoring: err") + responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") } func TestPost_GithubCommentInvalidCommand(t *testing.T) { t.Log("when the event is a github comment with an invalid command we ignore it") - e, v, _, p, _, _ := setup(t) + e, v, _, p, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(models.Repo{}, models.User{}, 1, nil) - When(p.DetermineCommand("", vcs.Github)).ThenReturn(nil, errors.New("err")) + When(p.DetermineCommand("", vcs.Github)).ThenReturn(events.CommandParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) - responseContains(t, w, http.StatusOK, "Ignoring: err") + responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") +} + +func TestPost_GitlabCommentResponse(t *testing.T) { + t.Log("when the event is a gitlab comment that warrants a comment response we comment back") + e, _, gl, p, _, _, vcsClient := setup(t) + eventsReq.Header.Set(gitlabHeader, "value") + When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) + When(p.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommandParseResult{CommentResponse: "a comment"}) + w := httptest.NewRecorder() + e.Post(w, eventsReq) + vcsClient.VerifyWasCalledOnce().CreateComment(models.Repo{}, 0, "a comment", vcs.Gitlab) + responseContains(t, w, http.StatusOK, "Commenting back on pull request") +} + +func TestPost_GithubCommentResponse(t *testing.T) { + t.Log("when the event is a github comment that warrants a comment response we comment back") + e, v, _, p, _, _, vcsClient := setup(t) + eventsReq.Header.Set(githubHeader, "issue_comment") + event := `{"action": "created"}` + When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) + baseRepo := models.Repo{} + user := models.User{} + When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) + When(p.DetermineCommand("", vcs.Github)).ThenReturn(events.CommandParseResult{CommentResponse: "a comment"}) + w := httptest.NewRecorder() + + e.Post(w, eventsReq) + vcsClient.VerifyWasCalledOnce().CreateComment(baseRepo, 1, "a comment", vcs.Github) + responseContains(t, w, http.StatusOK, "Commenting back on pull request") } func TestPost_GitlabCommentSuccess(t *testing.T) { t.Log("when the event is a gitlab comment with a valid command we call the command handler") - e, _, gl, _, cr, _ := setup(t) + e, _, gl, _, cr, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) w := httptest.NewRecorder() @@ -158,7 +188,7 @@ func TestPost_GitlabCommentSuccess(t *testing.T) { func TestPost_GithubCommentSuccess(t *testing.T) { t.Log("when the event is a github comment with a valid command we call the command handler") - e, v, _, p, cr, _ := setup(t) + e, v, _, p, cr, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) @@ -166,7 +196,7 @@ func TestPost_GithubCommentSuccess(t *testing.T) { user := models.User{} cmd := events.Command{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(p.DetermineCommand("", vcs.Github)).ThenReturn(&cmd, nil) + When(p.DetermineCommand("", vcs.Github)).ThenReturn(events.CommandParseResult{Command: &cmd}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Processing...") @@ -178,7 +208,7 @@ func TestPost_GithubCommentSuccess(t *testing.T) { func TestPost_GithubPullRequestNotClosed(t *testing.T) { t.Log("when the event is a github pull reuqest but it's not a closed event we ignore it") - e, v, _, _, _, _ := setup(t) + e, v, _, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "opened"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) @@ -189,7 +219,7 @@ func TestPost_GithubPullRequestNotClosed(t *testing.T) { func TestPost_GitlabMergeRequestNotClosed(t *testing.T) { t.Log("when the event is a gitlab merge request but it's not a closed event we ignore it") - e, _, gl, p, _, _ := setup(t) + e, _, gl, p, _, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") event := gitlab.MergeEvent{} When(gl.Validate(eventsReq, secret)).ThenReturn(event, nil) @@ -201,7 +231,7 @@ func TestPost_GitlabMergeRequestNotClosed(t *testing.T) { func TestPost_GithubPullRequestInvalid(t *testing.T) { t.Log("when the event is a github pull request with invalid data we return a 400") - e, v, _, p, _, _ := setup(t) + e, v, _, p, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -214,7 +244,7 @@ func TestPost_GithubPullRequestInvalid(t *testing.T) { func TestPost_GithubPullRequestInvalidRepo(t *testing.T) { t.Log("when the event is a github pull request with invalid repo data we return a 400") - e, v, _, p, _, _ := setup(t) + e, v, _, p, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -229,7 +259,7 @@ func TestPost_GithubPullRequestInvalidRepo(t *testing.T) { func TestPost_GithubPullRequestErrCleaningPull(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) - e, v, _, p, _, c := setup(t) + e, v, _, p, _, c, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -246,7 +276,7 @@ func TestPost_GithubPullRequestErrCleaningPull(t *testing.T) { func TestPost_GitlabMergeRequestErrCleaningPull(t *testing.T) { t.Log("when the event is a gitlab merge request and an error occurs calling CleanUpPull we return a 503") - e, _, gl, p, _, c := setup(t) + e, _, gl, p, _, c, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") event := gitlab.MergeEvent{} When(gl.Validate(eventsReq, secret)).ThenReturn(event, nil) @@ -261,7 +291,7 @@ func TestPost_GitlabMergeRequestErrCleaningPull(t *testing.T) { func TestPost_GithubPullRequestSuccess(t *testing.T) { t.Log("when the event is a pull request and everything works we return a 200") - e, v, _, p, _, c := setup(t) + e, v, _, p, _, c, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -278,7 +308,7 @@ func TestPost_GithubPullRequestSuccess(t *testing.T) { func TestPost_GitlabMergeRequestSuccess(t *testing.T) { t.Log("when the event is a gitlab merge request and the cleanup works we return a 200") - e, _, gl, p, _, _ := setup(t) + e, _, gl, p, _, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") event := gitlab.MergeEvent{} When(gl.Validate(eventsReq, secret)).ThenReturn(event, nil) @@ -290,7 +320,7 @@ func TestPost_GitlabMergeRequestSuccess(t *testing.T) { responseContains(t, w, http.StatusOK, "Merge request cleaned successfully") } -func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParser, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner) { +func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParser, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClientProxy) { RegisterMockTestingT(t) eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil)) v := mocks.NewMockGithubRequestValidator() @@ -298,6 +328,7 @@ func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValid p := emocks.NewMockEventParsing() cr := emocks.NewMockCommandRunner() c := emocks.NewMockPullCleaner() + vcsmock := vcsmocks.NewMockClientProxy() e := server.EventsController{ Logger: logging.NewNoopLogger(), GithubRequestValidator: v, @@ -308,6 +339,7 @@ func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValid SupportedVCSHosts: []vcs.Host{vcs.Github, vcs.Gitlab}, GitlabWebHookSecret: secret, GitlabRequestParser: gl, + VCSClient: vcsmock, } - return e, v, gl, p, cr, c + return e, v, gl, p, cr, c, vcsmock } diff --git a/server/server.go b/server/server.go index 9302438792..e0ddf3359f 100644 --- a/server/server.go +++ b/server/server.go @@ -184,7 +184,6 @@ func NewServer(config Config) (*Server, error) { Locker: lockingClient, ProjectFinder: &events.DefaultProjectFinder{}, } - helpExecutor := &events.HelpExecutor{} pullClosedExecutor := &events.PullClosedExecutor{ VCSClient: vcsClient, Locker: lockingClient, @@ -200,7 +199,6 @@ func NewServer(config Config) (*Server, error) { commandHandler := &events.CommandHandler{ ApplyExecutor: applyExecutor, PlanExecutor: planExecutor, - HelpExecutor: helpExecutor, LockURLGenerator: planExecutor, EventParser: eventParser, VCSClient: vcsClient, @@ -221,6 +219,7 @@ func NewServer(config Config) (*Server, error) { GitlabRequestParser: &DefaultGitlabRequestParser{}, GitlabWebHookSecret: []byte(config.GitlabWebHookSecret), SupportedVCSHosts: supportedVCSHosts, + VCSClient: vcsClient, } router := mux.NewRouter() return &Server{ From a09ad794059a8f15003c12d4910fc023a41461ec Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Wed, 28 Feb 2018 10:42:21 -0800 Subject: [PATCH 4/6] Refactor comment parsing into own class. --- server/events/comment_parser.go | 207 ++++++++++ server/events/comment_parser_test.go | 356 ++++++++++++++++++ server/events/event_parser.go | 188 --------- server/events/event_parser_test.go | 341 ----------------- .../matchers/events_commandparseresult.go | 10 +- .../matchers/events_commentparseresult.go | 20 + server/events/mocks/mock_comment_parsing.go | 81 ++++ server/events/mocks/mock_event_parsing.go | 45 --- server/events_controller.go | 3 +- server/events_controller_test.go | 62 +-- server/server.go | 7 + 11 files changed, 710 insertions(+), 610 deletions(-) create mode 100644 server/events/comment_parser.go create mode 100644 server/events/comment_parser_test.go create mode 100644 server/events/mocks/matchers/events_commentparseresult.go create mode 100644 server/events/mocks/mock_comment_parsing.go diff --git a/server/events/comment_parser.go b/server/events/comment_parser.go new file mode 100644 index 0000000000..5efac203ca --- /dev/null +++ b/server/events/comment_parser.go @@ -0,0 +1,207 @@ +package events + +import ( + "fmt" + "io/ioutil" + "path/filepath" + "strings" + + "github.com/runatlantis/atlantis/server/events/vcs" + "github.com/spf13/pflag" +) + +//go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_comment_parsing.go CommentParsing + +type CommentParsing interface { + DetermineCommand(comment string, vcsHost vcs.Host) CommentParseResult +} + +type CommentParser struct { + GithubUser string + GithubToken string + GitlabUser string + GitlabToken string +} + +// CommentParseResult describes the result of parsing a comment as a command. +type CommentParseResult struct { + // Command is the successfully parsed command. Will be nil if + // CommentResponse or Ignore is set. + Command *Command + // CommentResponse is set when we should respond immediately to the command + // for example for atlantis help. + CommentResponse string + // Ignore is set to true when we should just ignore this comment. + Ignore bool +} + +// DetermineCommand parses the comment as an Atlantis command. +// +// Valid commands contain: +// - The initial "executable" name, 'run' or 'atlantis' or '@GithubUser' +// where GithubUser is the API user Atlantis is running as. +// - Then a command, either 'plan', 'apply', or 'help'. +// - Then optional flags, then an optional separator '--' followed by optional +// extra flags to be appended to the terraform plan/apply command. +// +// Examples: +// - atlantis help +// - run plan +// - @GithubUser plan -w staging +// - atlantis plan -w staging -d dir --verbose +// - atlantis plan --verbose -- -key=value -key2 value2 +// +// nolint: gocyclo +func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) CommentParseResult { + if multiLineRegex.MatchString(comment) { + return CommentParseResult{Ignore: true} + } + + // strings.Fields strips out newlines but that's okay since we've removed + // multiline strings above. + args := strings.Fields(comment) + if len(args) < 1 { + return CommentParseResult{Ignore: true} + } + + // Helpfully warn the user if they're using "terraform" instead of "atlantis" + if args[0] == "terraform" { + return CommentParseResult{CommentResponse: DidYouMeanAtlantisComment} + } + + // Atlantis can be invoked using the name of the VCS host user we're + // running under. Need to be able to match against that user. + vcsUser := e.GithubUser + if vcsHost == vcs.Gitlab { + vcsUser = e.GitlabUser + } + executableNames := []string{"run", "atlantis", "@" + vcsUser} + + // If the comment doesn't start with the name of our 'executable' then + // ignore it. + if !e.stringInSlice(args[0], executableNames) { + return CommentParseResult{Ignore: true} + } + + // If they've just typed the name of the executable then give them the help + // output. + if len(args) == 1 { + return CommentParseResult{CommentResponse: HelpComment} + } + command := args[1] + + // Help output. + if e.stringInSlice(command, []string{"help", "-h", "--help"}) { + return CommentParseResult{CommentResponse: HelpComment} + } + + // Need to have a plan or apply at this point. + if !e.stringInSlice(command, []string{"plan", "apply"}) { + return CommentParseResult{CommentResponse: fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", command)} + } + + var workspace string + var dir string + var verbose bool + var extraArgs []string + var flagSet *pflag.FlagSet + var name CommandName + + // Set up the flag parsing depending on the command. + const defaultWorkspace = "default" + switch command { + case "plan": + name = Plan + flagSet = pflag.NewFlagSet("plan", pflag.ContinueOnError) + flagSet.SetOutput(ioutil.Discard) + flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Switch to this Terraform workspace before planning.") + flagSet.StringVarP(&dir, "dir", "d", "", "Which directory to run plan in relative to root of repo. Use '.' for root. If not specified, will attempt to run plan for all Terraform projects we think were modified in this changeset.") + flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") + case "apply": + name = Apply + flagSet = pflag.NewFlagSet("apply", pflag.ContinueOnError) + flagSet.SetOutput(ioutil.Discard) + flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Apply the plan for this Terraform workspace.") + flagSet.StringVarP(&dir, "dir", "d", "", "Apply the plan for this directory, relative to root of repo. Use '.' for root. If not specified, will run apply against all plans created for this workspace.") + flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") + default: + return CommentParseResult{CommentResponse: fmt.Sprintf("Error: unknown command %q – this is a bug", command)} + } + + // Now parse the flags. + // It's safe to use [2:] because we know there's at least 2 elements in args. + err := flagSet.Parse(args[2:]) + if err == pflag.ErrHelp { + return CommentParseResult{CommentResponse: fmt.Sprintf("```\nUsage of %s:\n%s\n```", command, flagSet.FlagUsagesWrapped(usagesCols))} + } + if err != nil { + return CommentParseResult{CommentResponse: fmt.Sprintf("```\nError: %s.\nUsage of %s:\n%s\n```", err.Error(), command, flagSet.FlagUsagesWrapped(usagesCols))} + } + // We only use the extra args after the --. For example given a comment: + // "atlantis plan -bad-option -- -target=hi" + // we only append "-target=hi" to the eventual command. + // todo: keep track of the args we're discarding and include that with + // comment as a warning. + if flagSet.ArgsLenAtDash() != -1 { + extraArgsUnsafe := flagSet.Args()[flagSet.ArgsLenAtDash():] + // Quote all extra args so there isn't a security issue when we append + // them to the terraform commands, ex. "; cat /etc/passwd" + for _, arg := range extraArgsUnsafe { + extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, arg)) + } + } + + // If dir is specified, must ensure it's a valid path. + if dir != "" { + validatedDir := filepath.Clean(dir) + // Join with . so the path is relative. This helps us if they use '/', + // and is safe to do if their path is relative since it's a no-op. + validatedDir = filepath.Join(".", validatedDir) + // Need to clean again to resolve relative validatedDirs. + validatedDir = filepath.Clean(validatedDir) + // Detect relative dirs since they're not allowed. + if strings.HasPrefix(validatedDir, "..") { + return CommentParseResult{CommentResponse: fmt.Sprintf("Error: Using a relative path %q with -d/--dir is not allowed", dir)} + } + + dir = validatedDir + } + // Because we use the workspace name as a file, need to make sure it's + // not doing something weird like being a relative dir. + if strings.Contains(workspace, "..") { + return CommentParseResult{CommentResponse: "Error: Value for -w/--workspace can't contain '..'"} + } + + return CommentParseResult{ + Command: &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs}, + } +} + +func (e *CommentParser) stringInSlice(a string, list []string) bool { + for _, b := range list { + if b == a { + return true + } + } + return false +} + +var HelpComment = "```cmake\n" + + `atlantis +Terraform automation and collaboration for your team + +Usage: + atlantis [options] + +Commands: + plan Runs 'terraform plan' for the changes in this pull request. + apply Runs 'terraform apply' on the plans generated by 'atlantis plan'. + help View help. + +Flags: + -h, --help help for atlantis + +Use "atlantis [command] --help" for more information about a command. +` + +var DidYouMeanAtlantisComment = "Did you mean to use `atlantis` instead of `terraform`?" diff --git a/server/events/comment_parser_test.go b/server/events/comment_parser_test.go new file mode 100644 index 0000000000..0d5a3f0f46 --- /dev/null +++ b/server/events/comment_parser_test.go @@ -0,0 +1,356 @@ +package events_test + +import ( + "fmt" + "strings" + "testing" + + "github.com/runatlantis/atlantis/server/events" + "github.com/runatlantis/atlantis/server/events/vcs" + . "github.com/runatlantis/atlantis/testing" +) + +var commentParser = events.CommentParser{ + GithubUser: "github-user", + GithubToken: "github-token", + GitlabUser: "gitlab-user", + GitlabToken: "gitlab-token", +} + +func TestDetermineCommand_Ignored(t *testing.T) { + t.Log("given a comment that should be ignored we should set " + + "CommentParseResult.Ignore to true") + ignoreComments := []string{ + "", + "a", + "abc", + "atlantis plan\nbut with newlines", + "terraform plan\nbut with newlines", + } + for _, c := range ignoreComments { + r := commentParser.DetermineCommand(c, vcs.Github) + Assert(t, r.Ignore, "expected Ignore to be true for comment %q", c) + } +} + +func TestDetermineCommand_HelpResponse(t *testing.T) { + t.Log("given a comment that should result in help output we " + + "should set CommentParseResult.CommentResult") + helpComments := []string{ + "run", + "atlantis", + "@github-user", + "atlantis help", + "atlantis --help", + "atlantis -h", + "atlantis help something else", + "atlantis help plan", + } + for _, c := range helpComments { + r := commentParser.DetermineCommand(c, vcs.Github) + Equals(t, events.HelpComment, r.CommentResponse) + } +} + +func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { + t.Log("given a comment that should result in a 'did you mean atlantis'" + + "response, should set CommentParseResult.CommentResult") + comments := []string{ + "terraform", + "terraform help", + "terraform --help", + "terraform -h", + "terraform plan", + "terraform apply", + "terraform plan -w workspace -d . -- test", + } + for _, c := range comments { + r := commentParser.DetermineCommand(c, vcs.Github) + Assert(t, r.CommentResponse == events.DidYouMeanAtlantisComment, + "For comment %q expected CommentResponse==%q but got %q", c, events.DidYouMeanAtlantisComment, r.CommentResponse) + } +} + +func TestDetermineCommand_InvalidCommand(t *testing.T) { + t.Log("given a comment with an invalid atlantis command, should return " + + "a warning.") + comments := []string{ + "atlantis paln", + "atlantis Plan", + "atlantis appely apply", + } + for _, c := range comments { + r := commentParser.DetermineCommand(c, vcs.Github) + exp := fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", strings.Fields(c)[1]) + Assert(t, r.CommentResponse == exp, + "For comment %q expected CommentResponse==%q but got %q", c, exp, r.CommentResponse) + } +} + +func TestDetermineCommand_SubcommandUsage(t *testing.T) { + t.Log("given a comment asking for the usage of a subcommand should " + + "return help") + comments := []string{ + "atlantis plan -h", + "atlantis plan --help", + "atlantis apply -h", + "atlantis apply --help", + } + for _, c := range comments { + r := commentParser.DetermineCommand(c, vcs.Github) + exp := "Usage of " + strings.Fields(c)[1] + Assert(t, strings.Contains(r.CommentResponse, exp), + "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) + Assert(t, !strings.Contains(r.CommentResponse, "Error:"), + "For comment %q expected CommentResponse %q to not contain %q", c, r.CommentResponse, "Error: ") + } +} + +func TestDetermineCommand_InvalidFlags(t *testing.T) { + t.Log("given a comment with a valid atlantis command but invalid" + + " flags, should return a warning and the proper usage") + cases := []struct { + comment string + exp string + }{ + { + "atlantis plan -e", + "Error: unknown shorthand flag: 'e' in -e", + }, + { + "atlantis plan --abc", + "Error: unknown flag: --abc", + }, + { + "atlantis apply -e", + "Error: unknown shorthand flag: 'e' in -e", + }, + { + "atlantis apply --abc", + "Error: unknown flag: --abc", + }, + } + for _, c := range cases { + r := commentParser.DetermineCommand(c.comment, vcs.Github) + Assert(t, strings.Contains(r.CommentResponse, c.exp), + "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, c.exp) + Assert(t, strings.Contains(r.CommentResponse, "Usage of "), + "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, "Usage of ") + } +} + +func TestDetermineCommand_RelativeDirPath(t *testing.T) { + t.Log("if -d is used with a relative path, should return an error") + comments := []string{ + "atlantis plan -d ..", + "atlantis apply -d ..", + // These won't return an error because we prepend with . when parsing. + //"atlantis plan -d /..", + //"atlantis apply -d /..", + "atlantis plan -d ./..", + "atlantis apply -d ./..", + "atlantis plan -d a/b/../../..", + "atlantis apply -d a/../..", + } + for _, c := range comments { + r := commentParser.DetermineCommand(c, vcs.Github) + exp := "Error: Using a relative path" + Assert(t, strings.Contains(r.CommentResponse, exp), + "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) + } +} + +func TestDetermineCommand_InvalidWorkspace(t *testing.T) { + t.Log("if -w is used with '..', should return an error") + comments := []string{ + "atlantis plan -w ..", + "atlantis apply -w ..", + "atlantis plan -w ..abc", + "atlantis apply -w abc..", + "atlantis plan -w abc..abc", + "atlantis apply -w ../../../etc/passwd", + } + for _, c := range comments { + r := commentParser.DetermineCommand(c, vcs.Github) + exp := "Error: Value for -w/--workspace can't contain '..'" + Assert(t, r.CommentResponse == exp, + "For comment %q expected CommentResponse %q to be %q", c, r.CommentResponse, exp) + } +} + +func TestDetermineCommand_Parsing(t *testing.T) { + cases := []struct { + flags string + expWorkspace string + expDir string + expVerbose bool + expExtraArgs string + }{ + // Test defaults. + { + "", + "default", + "", + false, + "", + }, + // Test each flag individually. + { + "-w workspace", + "workspace", + "", + false, + "", + }, + { + "-d dir", + "default", + "dir", + false, + "", + }, + { + "--verbose", + "default", + "", + true, + "", + }, + // Test all of them with different permutations. + { + "-w workspace -d dir --verbose", + "workspace", + "dir", + true, + "", + }, + { + "-d dir -w workspace --verbose", + "workspace", + "dir", + true, + "", + }, + { + "--verbose -w workspace -d dir", + "workspace", + "dir", + true, + "", + }, + // Test that flags after -- are ignored + { + "-w workspace -d dir -- --verbose", + "workspace", + "dir", + false, + "\"--verbose\"", + }, + { + "-w workspace -- -d dir --verbose", + "workspace", + "", + false, + "\"-d\" \"dir\" \"--verbose\"", + }, + // Test missing arguments. + { + "-w -d dir --verbose", + "-d", + "", + true, + "", + }, + // Test the extra args parsing. + { + "--", + "default", + "", + false, + "", + }, + { + "abc --", + "default", + "", + false, + "", + }, + { + "-w workspace -d dir --verbose -- arg one -two --three &&", + "workspace", + "dir", + true, + "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", + }, + // Test whitespace. + { + "\t-w\tworkspace\t-d\tdir\t--verbose\t--\targ\tone\t-two\t--three\t&&", + "workspace", + "dir", + true, + "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", + }, + { + " -w workspace -d dir --verbose -- arg one -two --three &&", + "workspace", + "dir", + true, + "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", + }, + // Test that the dir string is normalized. + { + "-d /", + "default", + ".", + false, + "", + }, + { + "-d /adir", + "default", + "adir", + false, + "", + }, + { + "-d .", + "default", + ".", + false, + "", + }, + { + "-d ./", + "default", + ".", + false, + "", + }, + { + "-d ./adir", + "default", + "adir", + false, + "", + }, + } + for _, test := range cases { + for _, cmdName := range []string{"plan", "apply"} { + comment := fmt.Sprintf("atlantis %s %s", cmdName, test.flags) + r := commentParser.DetermineCommand(comment, vcs.Github) + Assert(t, r.CommentResponse == "", "CommentResponse should have been empty but was %q for comment %q", r.CommentResponse, comment) + Assert(t, test.expDir == r.Command.Dir, "exp dir to equal %q but was %q for comment %q", test.expDir, r.Command.Dir, comment) + Assert(t, test.expWorkspace == r.Command.Workspace, "exp workspace to equal %q but was %q for comment %q", test.expWorkspace, r.Command.Workspace, comment) + Assert(t, test.expVerbose == r.Command.Verbose, "exp verbose to equal %v but was %v for comment %q", test.expVerbose, r.Command.Verbose, comment) + actExtraArgs := strings.Join(r.Command.Flags, " ") + Assert(t, test.expExtraArgs == actExtraArgs, "exp extra args to equal %v but got %v for comment %q", test.expExtraArgs, actExtraArgs, comment) + if cmdName == "plan" { + Assert(t, r.Command.Name == events.Plan, "did not parse comment %q as plan command", comment) + } + if cmdName == "apply" { + Assert(t, r.Command.Name == events.Apply, "did not parse comment %q as apply command", comment) + } + } + } +} diff --git a/server/events/event_parser.go b/server/events/event_parser.go index cb424b73d7..fd3db3ae77 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -3,16 +3,12 @@ package events import ( "errors" "fmt" - "io/ioutil" - "path/filepath" "regexp" "strings" "github.com/google/go-github/github" "github.com/lkysow/go-gitlab" "github.com/runatlantis/atlantis/server/events/models" - "github.com/runatlantis/atlantis/server/events/vcs" - "github.com/spf13/pflag" ) const gitlabPullOpened = "opened" @@ -36,7 +32,6 @@ type Command struct { } type EventParsing interface { - DetermineCommand(comment string, vcsHost vcs.Host) CommandParseResult ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pullNum int, err error) ParseGithubPull(pull *github.PullRequest) (models.PullRequest, models.Repo, error) ParseGithubRepo(ghRepo *github.Repository) (models.Repo, error) @@ -52,160 +47,6 @@ type EventParser struct { GitlabToken string } -// CommandParseResult describes the result of parsing a comment as a command. -type CommandParseResult struct { - // Command is the successfully parsed command. Will be nil if - // CommentResponse or Ignore is set. - Command *Command - // CommentResponse is set when we should respond immediately to the command - // for example for atlantis help. - CommentResponse string - // Ignore is set to true when we should just ignore this comment. - Ignore bool -} - -// DetermineCommand parses the comment as an Atlantis command. -// -// Valid commands contain: -// - The initial "executable" name, 'run' or 'atlantis' or '@GithubUser' -// where GithubUser is the API user Atlantis is running as. -// - Then a command, either 'plan', 'apply', or 'help'. -// - Then optional flags, then an optional separator '--' followed by optional -// extra flags to be appended to the terraform plan/apply command. -// -// Examples: -// - atlantis help -// - run plan -// - @GithubUser plan -w staging -// - atlantis plan -w staging -d dir --verbose -// - atlantis plan --verbose -- -key=value -key2 value2 -// -// nolint: gocyclo -func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) CommandParseResult { - if multiLineRegex.MatchString(comment) { - return CommandParseResult{Ignore: true} - } - - // strings.Fields strips out newlines but that's okay since we've removed - // multiline strings above. - args := strings.Fields(comment) - if len(args) < 1 { - return CommandParseResult{Ignore: true} - } - - // Helpfully warn the user if they're using "terraform" instead of "atlantis" - if args[0] == "terraform" { - return CommandParseResult{CommentResponse: DidYouMeanAtlantisComment} - } - - // Atlantis can be invoked using the name of the VCS host user we're - // running under. Need to be able to match against that user. - vcsUser := e.GithubUser - if vcsHost == vcs.Gitlab { - vcsUser = e.GitlabUser - } - executableNames := []string{"run", "atlantis", "@" + vcsUser} - - // If the comment doesn't start with the name of our 'executable' then - // ignore it. - if !e.stringInSlice(args[0], executableNames) { - return CommandParseResult{Ignore: true} - } - - // If they've just typed the name of the executable then give them the help - // output. - if len(args) == 1 { - return CommandParseResult{CommentResponse: HelpComment} - } - command := args[1] - - // Help output. - if e.stringInSlice(command, []string{"help", "-h", "--help"}) { - return CommandParseResult{CommentResponse: HelpComment} - } - - // Need to have a plan or apply at this point. - if !e.stringInSlice(command, []string{"plan", "apply"}) { - return CommandParseResult{CommentResponse: fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", command)} - } - - var workspace string - var dir string - var verbose bool - var extraArgs []string - var flagSet *pflag.FlagSet - var name CommandName - - // Set up the flag parsing depending on the command. - const defaultWorkspace = "default" - switch command { - case "plan": - name = Plan - flagSet = pflag.NewFlagSet("plan", pflag.ContinueOnError) - flagSet.SetOutput(ioutil.Discard) - flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Switch to this Terraform workspace before planning.") - flagSet.StringVarP(&dir, "dir", "d", "", "Which directory to run plan in relative to root of repo. Use '.' for root. If not specified, will attempt to run plan for all Terraform projects we think were modified in this changeset.") - flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") - case "apply": - name = Apply - flagSet = pflag.NewFlagSet("apply", pflag.ContinueOnError) - flagSet.SetOutput(ioutil.Discard) - flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, "Apply the plan for this Terraform workspace.") - flagSet.StringVarP(&dir, "dir", "d", "", "Apply the plan for this directory, relative to root of repo. Use '.' for root. If not specified, will run apply against all plans created for this workspace.") - flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") - default: - return CommandParseResult{CommentResponse: fmt.Sprintf("Error: unknown command %q – this is a bug", command)} - } - - // Now parse the flags. - // It's safe to use [2:] because we know there's at least 2 elements in args. - err := flagSet.Parse(args[2:]) - if err == pflag.ErrHelp { - return CommandParseResult{CommentResponse: fmt.Sprintf("```\nUsage of %s:\n%s\n```", command, flagSet.FlagUsagesWrapped(usagesCols))} - } - if err != nil { - return CommandParseResult{CommentResponse: fmt.Sprintf("```\nError: %s.\nUsage of %s:\n%s\n```", err.Error(), command, flagSet.FlagUsagesWrapped(usagesCols))} - } - // We only use the extra args after the --. For example given a comment: - // "atlantis plan -bad-option -- -target=hi" - // we only append "-target=hi" to the eventual command. - // todo: keep track of the args we're discarding and include that with - // comment as a warning. - if flagSet.ArgsLenAtDash() != -1 { - extraArgsUnsafe := flagSet.Args()[flagSet.ArgsLenAtDash():] - // Quote all extra args so there isn't a security issue when we append - // them to the terraform commands, ex. "; cat /etc/passwd" - for _, arg := range extraArgsUnsafe { - extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, arg)) - } - } - - // If dir is specified, must ensure it's a valid path. - if dir != "" { - validatedDir := filepath.Clean(dir) - // Join with . so the path is relative. This helps us if they use '/', - // and is safe to do if their path is relative since it's a no-op. - validatedDir = filepath.Join(".", validatedDir) - // Need to clean again to resolve relative validatedDirs. - validatedDir = filepath.Clean(validatedDir) - // Detect relative dirs since they're not allowed. - if strings.HasPrefix(validatedDir, "..") { - return CommandParseResult{CommentResponse: fmt.Sprintf("Error: Using a relative path %q with -d/--dir is not allowed", dir)} - } - - dir = validatedDir - } - // Because we use the workspace name as a file, need to make sure it's - // not doing something weird like being a relative dir. - if strings.Contains(workspace, "..") { - return CommandParseResult{CommentResponse: "Error: Value for -w/--workspace can't contain '..'"} - } - - return CommandParseResult{ - Command: &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs}, - } -} - func (e *EventParser) ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pullNum int, err error) { baseRepo, err = e.ParseGithubRepo(comment.Repo) if err != nil { @@ -396,32 +237,3 @@ func (e *EventParser) ParseGitlabMergeRequest(mr *gitlab.MergeRequest) models.Pu State: pullState, } } - -func (e *EventParser) stringInSlice(a string, list []string) bool { - for _, b := range list { - if b == a { - return true - } - } - return false -} - -var HelpComment = "```cmake\n" + - `atlantis -Terraform automation and collaboration for your team - -Usage: - atlantis [options] - -Commands: - plan Runs 'terraform plan' for the changes in this pull request. - apply Runs 'terraform apply' on the plans generated by 'atlantis plan'. - help View help. - -Flags: - -h, --help help for atlantis - -Use "atlantis [command] --help" for more information about a command. -` - -var DidYouMeanAtlantisComment = "Did you mean to use `atlantis` instead of `terraform`?" diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index aa45f01f89..a13b9f3e24 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -3,8 +3,6 @@ package events_test import ( "encoding/json" "errors" - "fmt" - "strings" "testing" "github.com/google/go-github/github" @@ -12,7 +10,6 @@ import ( "github.com/mohae/deepcopy" "github.com/runatlantis/atlantis/server/events" "github.com/runatlantis/atlantis/server/events/models" - "github.com/runatlantis/atlantis/server/events/vcs" . "github.com/runatlantis/atlantis/server/events/vcs/fixtures" . "github.com/runatlantis/atlantis/testing" ) @@ -24,344 +21,6 @@ var parser = events.EventParser{ GitlabToken: "gitlab-token", } -func TestDetermineCommand_Ignored(t *testing.T) { - t.Log("given a comment that should be ignored we should set " + - "CommandParseResult.Ignore to true") - ignoreComments := []string{ - "", - "a", - "abc", - "atlantis plan\nbut with newlines", - "terraform plan\nbut with newlines", - } - for _, c := range ignoreComments { - r := parser.DetermineCommand(c, vcs.Github) - Assert(t, r.Ignore, "expected Ignore to be true for comment %q", c) - } -} - -func TestDetermineCommand_HelpResponse(t *testing.T) { - t.Log("given a comment that should result in help output we " + - "should set CommandParseResult.CommentResult") - helpComments := []string{ - "run", - "atlantis", - "@github-user", - "atlantis help", - "atlantis --help", - "atlantis -h", - "atlantis help something else", - "atlantis help plan", - } - for _, c := range helpComments { - r := parser.DetermineCommand(c, vcs.Github) - Equals(t, events.HelpComment, r.CommentResponse) - } -} - -func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { - t.Log("given a comment that should result in a 'did you mean atlantis'" + - "response, should set CommandParseResult.CommentResult") - comments := []string{ - "terraform", - "terraform help", - "terraform --help", - "terraform -h", - "terraform plan", - "terraform apply", - "terraform plan -w workspace -d . -- test", - } - for _, c := range comments { - r := parser.DetermineCommand(c, vcs.Github) - Assert(t, r.CommentResponse == events.DidYouMeanAtlantisComment, - "For comment %q expected CommentResponse==%q but got %q", c, events.DidYouMeanAtlantisComment, r.CommentResponse) - } -} - -func TestDetermineCommand_InvalidCommand(t *testing.T) { - t.Log("given a comment with an invalid atlantis command, should return " + - "a warning.") - comments := []string{ - "atlantis paln", - "atlantis Plan", - "atlantis appely apply", - } - for _, c := range comments { - r := parser.DetermineCommand(c, vcs.Github) - exp := fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", strings.Fields(c)[1]) - Assert(t, r.CommentResponse == exp, - "For comment %q expected CommentResponse==%q but got %q", c, exp, r.CommentResponse) - } -} - -func TestDetermineCommand_SubcommandUsage(t *testing.T) { - t.Log("given a comment asking for the usage of a subcommand should " + - "return help") - comments := []string{ - "atlantis plan -h", - "atlantis plan --help", - "atlantis apply -h", - "atlantis apply --help", - } - for _, c := range comments { - r := parser.DetermineCommand(c, vcs.Github) - exp := "Usage of " + strings.Fields(c)[1] - Assert(t, strings.Contains(r.CommentResponse, exp), - "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) - Assert(t, !strings.Contains(r.CommentResponse, "Error:"), - "For comment %q expected CommentResponse %q to not contain %q", c, r.CommentResponse, "Error: ") - } -} - -func TestDetermineCommand_InvalidFlags(t *testing.T) { - t.Log("given a comment with a valid atlantis command but invalid" + - " flags, should return a warning and the proper usage") - cases := []struct { - comment string - exp string - }{ - { - "atlantis plan -e", - "Error: unknown shorthand flag: 'e' in -e", - }, - { - "atlantis plan --abc", - "Error: unknown flag: --abc", - }, - { - "atlantis apply -e", - "Error: unknown shorthand flag: 'e' in -e", - }, - { - "atlantis apply --abc", - "Error: unknown flag: --abc", - }, - } - for _, c := range cases { - r := parser.DetermineCommand(c.comment, vcs.Github) - Assert(t, strings.Contains(r.CommentResponse, c.exp), - "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, c.exp) - Assert(t, strings.Contains(r.CommentResponse, "Usage of "), - "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, "Usage of ") - } -} - -func TestDetermineCommand_RelativeDirPath(t *testing.T) { - t.Log("if -d is used with a relative path, should return an error") - comments := []string{ - "atlantis plan -d ..", - "atlantis apply -d ..", - // These won't return an error because we prepend with . when parsing. - //"atlantis plan -d /..", - //"atlantis apply -d /..", - "atlantis plan -d ./..", - "atlantis apply -d ./..", - "atlantis plan -d a/b/../../..", - "atlantis apply -d a/../..", - } - for _, c := range comments { - r := parser.DetermineCommand(c, vcs.Github) - exp := "Error: Using a relative path" - Assert(t, strings.Contains(r.CommentResponse, exp), - "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) - } -} - -func TestDetermineCommand_InvalidWorkspace(t *testing.T) { - t.Log("if -w is used with '..', should return an error") - comments := []string{ - "atlantis plan -w ..", - "atlantis apply -w ..", - "atlantis plan -w ..abc", - "atlantis apply -w abc..", - "atlantis plan -w abc..abc", - "atlantis apply -w ../../../etc/passwd", - } - for _, c := range comments { - r := parser.DetermineCommand(c, vcs.Github) - exp := "Error: Value for -w/--workspace can't contain '..'" - Assert(t, r.CommentResponse == exp, - "For comment %q expected CommentResponse %q to be %q", c, r.CommentResponse, exp) - } -} - -func TestDetermineCommand_Parsing(t *testing.T) { - cases := []struct { - flags string - expWorkspace string - expDir string - expVerbose bool - expExtraArgs string - }{ - // Test defaults. - { - "", - "default", - "", - false, - "", - }, - // Test each flag individually. - { - "-w workspace", - "workspace", - "", - false, - "", - }, - { - "-d dir", - "default", - "dir", - false, - "", - }, - { - "--verbose", - "default", - "", - true, - "", - }, - // Test all of them with different permutations. - { - "-w workspace -d dir --verbose", - "workspace", - "dir", - true, - "", - }, - { - "-d dir -w workspace --verbose", - "workspace", - "dir", - true, - "", - }, - { - "--verbose -w workspace -d dir", - "workspace", - "dir", - true, - "", - }, - // Test that flags after -- are ignored - { - "-w workspace -d dir -- --verbose", - "workspace", - "dir", - false, - "\"--verbose\"", - }, - { - "-w workspace -- -d dir --verbose", - "workspace", - "", - false, - "\"-d\" \"dir\" \"--verbose\"", - }, - // Test missing arguments. - { - "-w -d dir --verbose", - "-d", - "", - true, - "", - }, - // Test the extra args parsing. - { - "--", - "default", - "", - false, - "", - }, - { - "abc --", - "default", - "", - false, - "", - }, - { - "-w workspace -d dir --verbose -- arg one -two --three &&", - "workspace", - "dir", - true, - "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", - }, - // Test whitespace. - { - "\t-w\tworkspace\t-d\tdir\t--verbose\t--\targ\tone\t-two\t--three\t&&", - "workspace", - "dir", - true, - "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", - }, - { - " -w workspace -d dir --verbose -- arg one -two --three &&", - "workspace", - "dir", - true, - "\"arg\" \"one\" \"-two\" \"--three\" \"&&\"", - }, - // Test that the dir string is normalized. - { - "-d /", - "default", - ".", - false, - "", - }, - { - "-d /adir", - "default", - "adir", - false, - "", - }, - { - "-d .", - "default", - ".", - false, - "", - }, - { - "-d ./", - "default", - ".", - false, - "", - }, - { - "-d ./adir", - "default", - "adir", - false, - "", - }, - } - for _, test := range cases { - for _, cmdName := range []string{"plan", "apply"} { - comment := fmt.Sprintf("atlantis %s %s", cmdName, test.flags) - r := parser.DetermineCommand(comment, vcs.Github) - Assert(t, r.CommentResponse == "", "CommentResponse should have been empty but was %q for comment %q", r.CommentResponse, comment) - Assert(t, test.expDir == r.Command.Dir, "exp dir to equal %q but was %q for comment %q", test.expDir, r.Command.Dir, comment) - Assert(t, test.expWorkspace == r.Command.Workspace, "exp workspace to equal %q but was %q for comment %q", test.expWorkspace, r.Command.Workspace, comment) - Assert(t, test.expVerbose == r.Command.Verbose, "exp verbose to equal %v but was %v for comment %q", test.expVerbose, r.Command.Verbose, comment) - actExtraArgs := strings.Join(r.Command.Flags, " ") - Assert(t, test.expExtraArgs == actExtraArgs, "exp extra args to equal %v but got %v for comment %q", test.expExtraArgs, actExtraArgs, comment) - if cmdName == "plan" { - Assert(t, r.Command.Name == events.Plan, "did not parse comment %q as plan command", comment) - } - if cmdName == "apply" { - Assert(t, r.Command.Name == events.Apply, "did not parse comment %q as apply command", comment) - } - } - } -} - func TestParseGithubRepo(t *testing.T) { testRepo := Repo testRepo.FullName = nil diff --git a/server/events/mocks/matchers/events_commandparseresult.go b/server/events/mocks/matchers/events_commandparseresult.go index 4b3607195a..12e5991a7e 100644 --- a/server/events/mocks/matchers/events_commandparseresult.go +++ b/server/events/mocks/matchers/events_commandparseresult.go @@ -7,14 +7,14 @@ import ( events "github.com/runatlantis/atlantis/server/events" ) -func AnyEventsCommandParseResult() events.CommandParseResult { - pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(events.CommandParseResult))(nil)).Elem())) - var nullValue events.CommandParseResult +func AnyEventsCommandParseResult() events.CommentParseResult { + pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(events.CommentParseResult))(nil)).Elem())) + var nullValue events.CommentParseResult return nullValue } -func EqEventsCommandParseResult(value events.CommandParseResult) events.CommandParseResult { +func EqEventsCommandParseResult(value events.CommentParseResult) events.CommentParseResult { pegomock.RegisterMatcher(&pegomock.EqMatcher{Value: value}) - var nullValue events.CommandParseResult + var nullValue events.CommentParseResult return nullValue } diff --git a/server/events/mocks/matchers/events_commentparseresult.go b/server/events/mocks/matchers/events_commentparseresult.go new file mode 100644 index 0000000000..5409329944 --- /dev/null +++ b/server/events/mocks/matchers/events_commentparseresult.go @@ -0,0 +1,20 @@ +package matchers + +import ( + "reflect" + + "github.com/petergtz/pegomock" + events "github.com/runatlantis/atlantis/server/events" +) + +func AnyEventsCommentParseResult() events.CommentParseResult { + pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(events.CommentParseResult))(nil)).Elem())) + var nullValue events.CommentParseResult + return nullValue +} + +func EqEventsCommentParseResult(value events.CommentParseResult) events.CommentParseResult { + pegomock.RegisterMatcher(&pegomock.EqMatcher{Value: value}) + var nullValue events.CommentParseResult + return nullValue +} diff --git a/server/events/mocks/mock_comment_parsing.go b/server/events/mocks/mock_comment_parsing.go new file mode 100644 index 0000000000..e61cda2267 --- /dev/null +++ b/server/events/mocks/mock_comment_parsing.go @@ -0,0 +1,81 @@ +// Automatically generated by pegomock. DO NOT EDIT! +// Source: github.com/runatlantis/atlantis/server/events (interfaces: CommentParsing) + +package mocks + +import ( + "reflect" + + pegomock "github.com/petergtz/pegomock" + events "github.com/runatlantis/atlantis/server/events" + vcs "github.com/runatlantis/atlantis/server/events/vcs" +) + +type MockCommentParsing struct { + fail func(message string, callerSkip ...int) +} + +func NewMockCommentParsing() *MockCommentParsing { + return &MockCommentParsing{fail: pegomock.GlobalFailHandler} +} + +func (mock *MockCommentParsing) DetermineCommand(comment string, vcsHost vcs.Host) events.CommentParseResult { + params := []pegomock.Param{comment, vcsHost} + result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineCommand", params, []reflect.Type{reflect.TypeOf((*events.CommentParseResult)(nil)).Elem()}) + var ret0 events.CommentParseResult + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].(events.CommentParseResult) + } + } + return ret0 +} + +func (mock *MockCommentParsing) VerifyWasCalledOnce() *VerifierCommentParsing { + return &VerifierCommentParsing{mock, pegomock.Times(1), nil} +} + +func (mock *MockCommentParsing) VerifyWasCalled(invocationCountMatcher pegomock.Matcher) *VerifierCommentParsing { + return &VerifierCommentParsing{mock, invocationCountMatcher, nil} +} + +func (mock *MockCommentParsing) VerifyWasCalledInOrder(invocationCountMatcher pegomock.Matcher, inOrderContext *pegomock.InOrderContext) *VerifierCommentParsing { + return &VerifierCommentParsing{mock, invocationCountMatcher, inOrderContext} +} + +type VerifierCommentParsing struct { + mock *MockCommentParsing + invocationCountMatcher pegomock.Matcher + inOrderContext *pegomock.InOrderContext +} + +func (verifier *VerifierCommentParsing) DetermineCommand(comment string, vcsHost vcs.Host) *CommentParsing_DetermineCommand_OngoingVerification { + params := []pegomock.Param{comment, vcsHost} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "DetermineCommand", params) + return &CommentParsing_DetermineCommand_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +} + +type CommentParsing_DetermineCommand_OngoingVerification struct { + mock *MockCommentParsing + methodInvocations []pegomock.MethodInvocation +} + +func (c *CommentParsing_DetermineCommand_OngoingVerification) GetCapturedArguments() (string, vcs.Host) { + comment, vcsHost := c.GetAllCapturedArguments() + return comment[len(comment)-1], vcsHost[len(vcsHost)-1] +} + +func (c *CommentParsing_DetermineCommand_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []vcs.Host) { + params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(params) > 0 { + _param0 = make([]string, len(params[0])) + for u, param := range params[0] { + _param0[u] = param.(string) + } + _param1 = make([]vcs.Host, len(params[1])) + for u, param := range params[1] { + _param1[u] = param.(vcs.Host) + } + } + return +} diff --git a/server/events/mocks/mock_event_parsing.go b/server/events/mocks/mock_event_parsing.go index b4c7898de1..ad401861df 100644 --- a/server/events/mocks/mock_event_parsing.go +++ b/server/events/mocks/mock_event_parsing.go @@ -9,9 +9,7 @@ import ( github "github.com/google/go-github/github" go_gitlab "github.com/lkysow/go-gitlab" pegomock "github.com/petergtz/pegomock" - events "github.com/runatlantis/atlantis/server/events" models "github.com/runatlantis/atlantis/server/events/models" - vcs "github.com/runatlantis/atlantis/server/events/vcs" ) type MockEventParsing struct { @@ -22,18 +20,6 @@ func NewMockEventParsing() *MockEventParsing { return &MockEventParsing{fail: pegomock.GlobalFailHandler} } -func (mock *MockEventParsing) DetermineCommand(comment string, vcsHost vcs.Host) events.CommandParseResult { - params := []pegomock.Param{comment, vcsHost} - result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineCommand", params, []reflect.Type{reflect.TypeOf((*events.CommandParseResult)(nil)).Elem()}) - var ret0 events.CommandParseResult - if len(result) != 0 { - if result[0] != nil { - ret0 = result[0].(events.CommandParseResult) - } - } - return ret0 -} - func (mock *MockEventParsing) ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) (models.Repo, models.User, int, error) { params := []pegomock.Param{comment} result := pegomock.GetGenericMockFrom(mock).Invoke("ParseGithubIssueCommentEvent", params, []reflect.Type{reflect.TypeOf((*models.Repo)(nil)).Elem(), reflect.TypeOf((*models.User)(nil)).Elem(), reflect.TypeOf((*int)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) @@ -160,37 +146,6 @@ type VerifierEventParsing struct { inOrderContext *pegomock.InOrderContext } -func (verifier *VerifierEventParsing) DetermineCommand(comment string, vcsHost vcs.Host) *EventParsing_DetermineCommand_OngoingVerification { - params := []pegomock.Param{comment, vcsHost} - methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "DetermineCommand", params) - return &EventParsing_DetermineCommand_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} -} - -type EventParsing_DetermineCommand_OngoingVerification struct { - mock *MockEventParsing - methodInvocations []pegomock.MethodInvocation -} - -func (c *EventParsing_DetermineCommand_OngoingVerification) GetCapturedArguments() (string, vcs.Host) { - comment, vcsHost := c.GetAllCapturedArguments() - return comment[len(comment)-1], vcsHost[len(vcsHost)-1] -} - -func (c *EventParsing_DetermineCommand_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []vcs.Host) { - params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) - if len(params) > 0 { - _param0 = make([]string, len(params[0])) - for u, param := range params[0] { - _param0[u] = param.(string) - } - _param1 = make([]vcs.Host, len(params[1])) - for u, param := range params[1] { - _param1[u] = param.(vcs.Host) - } - } - return -} - func (verifier *VerifierEventParsing) ParseGithubIssueCommentEvent(comment *github.IssueCommentEvent) *EventParsing_ParseGithubIssueCommentEvent_OngoingVerification { params := []pegomock.Param{comment} methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "ParseGithubIssueCommentEvent", params) diff --git a/server/events_controller.go b/server/events_controller.go index 9cb75e2854..084e1d172e 100644 --- a/server/events_controller.go +++ b/server/events_controller.go @@ -22,6 +22,7 @@ type EventsController struct { PullCleaner events.PullCleaner Logger *logging.SimpleLogger Parser events.EventParsing + CommentParser events.CommentParsing // GithubWebHookSecret is the secret added to this webhook via the GitHub // UI that identifies this call as coming from GitHub. If empty, no // request validation is done. @@ -151,7 +152,7 @@ func (e *EventsController) HandleGitlabCommentEvent(w http.ResponseWriter, event } func (e *EventsController) handleCommentEvent(w http.ResponseWriter, baseRepo models.Repo, headRepo models.Repo, user models.User, pullNum int, comment string, vcsHost vcs.Host) { - parseResult := e.Parser.DetermineCommand(comment, vcsHost) + parseResult := e.CommentParser.DetermineCommand(comment, vcsHost) if parseResult.Ignore { truncated := comment if len(truncated) > 40 { diff --git a/server/events_controller_test.go b/server/events_controller_test.go index 97d95fbf28..c9f46b907f 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -29,7 +29,7 @@ var eventsReq *http.Request func TestPost_NotGithubOrGitlab(t *testing.T) { t.Log("when the request is not for gitlab or github a 400 is returned") - e, _, _, _, _, _, _ := setup(t) + e, _, _, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusBadRequest, "Ignoring request") @@ -37,7 +37,7 @@ func TestPost_NotGithubOrGitlab(t *testing.T) { func TestPost_UnsupportedVCSGithub(t *testing.T) { t.Log("when the request is for an unsupported vcs a 400 is returned") - e, _, _, _, _, _, _ := setup(t) + e, _, _, _, _, _, _, _ := setup(t) e.SupportedVCSHosts = nil eventsReq.Header.Set(githubHeader, "value") w := httptest.NewRecorder() @@ -47,7 +47,7 @@ func TestPost_UnsupportedVCSGithub(t *testing.T) { func TestPost_UnsupportedVCSGitlab(t *testing.T) { t.Log("when the request is for an unsupported vcs a 400 is returned") - e, _, _, _, _, _, _ := setup(t) + e, _, _, _, _, _, _, _ := setup(t) e.SupportedVCSHosts = nil eventsReq.Header.Set(gitlabHeader, "value") w := httptest.NewRecorder() @@ -57,7 +57,7 @@ func TestPost_UnsupportedVCSGitlab(t *testing.T) { func TestPost_InvalidGithubSecret(t *testing.T) { t.Log("when the github payload can't be validated a 400 is returned") - e, v, _, _, _, _, _ := setup(t) + e, v, _, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(githubHeader, "value") When(v.Validate(eventsReq, secret)).ThenReturn(nil, errors.New("err")) @@ -67,7 +67,7 @@ func TestPost_InvalidGithubSecret(t *testing.T) { func TestPost_InvalidGitlabSecret(t *testing.T) { t.Log("when the gitlab payload can't be validated a 400 is returned") - e, _, gl, _, _, _, _ := setup(t) + e, _, gl, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(nil, errors.New("err")) @@ -77,7 +77,7 @@ func TestPost_InvalidGitlabSecret(t *testing.T) { func TestPost_UnsupportedGithubEvent(t *testing.T) { t.Log("when the event type is an unsupported github event we ignore it") - e, v, _, _, _, _, _ := setup(t) + e, v, _, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(githubHeader, "value") When(v.Validate(eventsReq, nil)).ThenReturn([]byte(`{"not an event": ""}`), nil) @@ -87,7 +87,7 @@ func TestPost_UnsupportedGithubEvent(t *testing.T) { func TestPost_UnsupportedGitlabEvent(t *testing.T) { t.Log("when the event type is an unsupported gitlab event we ignore it") - e, _, gl, _, _, _, _ := setup(t) + e, _, gl, _, _, _, _, _ := setup(t) w := httptest.NewRecorder() eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn([]byte(`{"not an event": ""}`), nil) @@ -97,7 +97,7 @@ func TestPost_UnsupportedGitlabEvent(t *testing.T) { func TestPost_GithubCommentNotCreated(t *testing.T) { t.Log("when the event is a github comment but it's not a created event we ignore it") - e, v, _, _, _, _, _ := setup(t) + e, v, _, _, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") // comment action is deleted, not created event := `{"action": "deleted"}` @@ -109,7 +109,7 @@ func TestPost_GithubCommentNotCreated(t *testing.T) { func TestPost_GithubInvalidComment(t *testing.T) { t.Log("when the event is a github comment without all expected data we return a 400") - e, v, _, p, _, _, _ := setup(t) + e, v, _, p, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) @@ -121,10 +121,10 @@ func TestPost_GithubInvalidComment(t *testing.T) { func TestPost_GitlabCommentInvalidCommand(t *testing.T) { t.Log("when the event is a gitlab comment with an invalid command we ignore it") - e, _, gl, p, _, _, _ := setup(t) + e, _, gl, _, _, _, _, cp := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(p.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommandParseResult{Ignore: true}) + When(cp.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") @@ -132,12 +132,12 @@ func TestPost_GitlabCommentInvalidCommand(t *testing.T) { func TestPost_GithubCommentInvalidCommand(t *testing.T) { t.Log("when the event is a github comment with an invalid command we ignore it") - e, v, _, p, _, _, _ := setup(t) + e, v, _, p, _, _, _, cp := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(models.Repo{}, models.User{}, 1, nil) - When(p.DetermineCommand("", vcs.Github)).ThenReturn(events.CommandParseResult{Ignore: true}) + When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") @@ -145,10 +145,10 @@ func TestPost_GithubCommentInvalidCommand(t *testing.T) { func TestPost_GitlabCommentResponse(t *testing.T) { t.Log("when the event is a gitlab comment that warrants a comment response we comment back") - e, _, gl, p, _, _, vcsClient := setup(t) + e, _, gl, _, _, _, vcsClient, cp := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(p.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommandParseResult{CommentResponse: "a comment"}) + When(cp.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) w := httptest.NewRecorder() e.Post(w, eventsReq) vcsClient.VerifyWasCalledOnce().CreateComment(models.Repo{}, 0, "a comment", vcs.Gitlab) @@ -157,14 +157,14 @@ func TestPost_GitlabCommentResponse(t *testing.T) { func TestPost_GithubCommentResponse(t *testing.T) { t.Log("when the event is a github comment that warrants a comment response we comment back") - e, v, _, p, _, _, vcsClient := setup(t) + e, v, _, p, _, _, vcsClient, cp := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) baseRepo := models.Repo{} user := models.User{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(p.DetermineCommand("", vcs.Github)).ThenReturn(events.CommandParseResult{CommentResponse: "a comment"}) + When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) w := httptest.NewRecorder() e.Post(w, eventsReq) @@ -174,7 +174,7 @@ func TestPost_GithubCommentResponse(t *testing.T) { func TestPost_GitlabCommentSuccess(t *testing.T) { t.Log("when the event is a gitlab comment with a valid command we call the command handler") - e, _, gl, _, cr, _, _ := setup(t) + e, _, gl, _, cr, _, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) w := httptest.NewRecorder() @@ -188,7 +188,7 @@ func TestPost_GitlabCommentSuccess(t *testing.T) { func TestPost_GithubCommentSuccess(t *testing.T) { t.Log("when the event is a github comment with a valid command we call the command handler") - e, v, _, p, cr, _, _ := setup(t) + e, v, _, p, cr, _, _, cp := setup(t) eventsReq.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) @@ -196,7 +196,7 @@ func TestPost_GithubCommentSuccess(t *testing.T) { user := models.User{} cmd := events.Command{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(p.DetermineCommand("", vcs.Github)).ThenReturn(events.CommandParseResult{Command: &cmd}) + When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{Command: &cmd}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Processing...") @@ -208,7 +208,7 @@ func TestPost_GithubCommentSuccess(t *testing.T) { func TestPost_GithubPullRequestNotClosed(t *testing.T) { t.Log("when the event is a github pull reuqest but it's not a closed event we ignore it") - e, v, _, _, _, _, _ := setup(t) + e, v, _, _, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "opened"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) @@ -219,7 +219,7 @@ func TestPost_GithubPullRequestNotClosed(t *testing.T) { func TestPost_GitlabMergeRequestNotClosed(t *testing.T) { t.Log("when the event is a gitlab merge request but it's not a closed event we ignore it") - e, _, gl, p, _, _, _ := setup(t) + e, _, gl, p, _, _, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") event := gitlab.MergeEvent{} When(gl.Validate(eventsReq, secret)).ThenReturn(event, nil) @@ -231,7 +231,7 @@ func TestPost_GitlabMergeRequestNotClosed(t *testing.T) { func TestPost_GithubPullRequestInvalid(t *testing.T) { t.Log("when the event is a github pull request with invalid data we return a 400") - e, v, _, p, _, _, _ := setup(t) + e, v, _, p, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -244,7 +244,7 @@ func TestPost_GithubPullRequestInvalid(t *testing.T) { func TestPost_GithubPullRequestInvalidRepo(t *testing.T) { t.Log("when the event is a github pull request with invalid repo data we return a 400") - e, v, _, p, _, _, _ := setup(t) + e, v, _, p, _, _, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -259,7 +259,7 @@ func TestPost_GithubPullRequestInvalidRepo(t *testing.T) { func TestPost_GithubPullRequestErrCleaningPull(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) - e, v, _, p, _, c, _ := setup(t) + e, v, _, p, _, c, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -276,7 +276,7 @@ func TestPost_GithubPullRequestErrCleaningPull(t *testing.T) { func TestPost_GitlabMergeRequestErrCleaningPull(t *testing.T) { t.Log("when the event is a gitlab merge request and an error occurs calling CleanUpPull we return a 503") - e, _, gl, p, _, c, _ := setup(t) + e, _, gl, p, _, c, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") event := gitlab.MergeEvent{} When(gl.Validate(eventsReq, secret)).ThenReturn(event, nil) @@ -291,7 +291,7 @@ func TestPost_GitlabMergeRequestErrCleaningPull(t *testing.T) { func TestPost_GithubPullRequestSuccess(t *testing.T) { t.Log("when the event is a pull request and everything works we return a 200") - e, v, _, p, _, c, _ := setup(t) + e, v, _, p, _, c, _, _ := setup(t) eventsReq.Header.Set(githubHeader, "pull_request") event := `{"action": "closed"}` @@ -308,7 +308,7 @@ func TestPost_GithubPullRequestSuccess(t *testing.T) { func TestPost_GitlabMergeRequestSuccess(t *testing.T) { t.Log("when the event is a gitlab merge request and the cleanup works we return a 200") - e, _, gl, p, _, _, _ := setup(t) + e, _, gl, p, _, _, _, _ := setup(t) eventsReq.Header.Set(gitlabHeader, "value") event := gitlab.MergeEvent{} When(gl.Validate(eventsReq, secret)).ThenReturn(event, nil) @@ -320,12 +320,13 @@ func TestPost_GitlabMergeRequestSuccess(t *testing.T) { responseContains(t, w, http.StatusOK, "Merge request cleaned successfully") } -func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParser, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClientProxy) { +func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParser, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClientProxy, *emocks.MockCommentParsing) { RegisterMockTestingT(t) eventsReq, _ = http.NewRequest("GET", "", bytes.NewBuffer(nil)) v := mocks.NewMockGithubRequestValidator() gl := mocks.NewMockGitlabRequestParser() p := emocks.NewMockEventParsing() + cp := emocks.NewMockCommentParsing() cr := emocks.NewMockCommandRunner() c := emocks.NewMockPullCleaner() vcsmock := vcsmocks.NewMockClientProxy() @@ -333,6 +334,7 @@ func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValid Logger: logging.NewNoopLogger(), GithubRequestValidator: v, Parser: p, + CommentParser: cp, CommandRunner: cr, PullCleaner: c, GithubWebHookSecret: secret, @@ -341,5 +343,5 @@ func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValid GitlabRequestParser: gl, VCSClient: vcsmock, } - return e, v, gl, p, cr, c, vcsmock + return e, v, gl, p, cr, c, vcsmock, cp } diff --git a/server/server.go b/server/server.go index e0ddf3359f..0ed76c360a 100644 --- a/server/server.go +++ b/server/server.go @@ -196,6 +196,12 @@ func NewServer(config Config) (*Server, error) { GitlabUser: config.GitlabUser, GitlabToken: config.GitlabToken, } + commentParser := &events.CommentParser{ + GithubUser: config.GithubUser, + GithubToken: config.GithubToken, + GitlabUser: config.GitlabUser, + GitlabToken: config.GitlabToken, + } commandHandler := &events.CommandHandler{ ApplyExecutor: applyExecutor, PlanExecutor: planExecutor, @@ -213,6 +219,7 @@ func NewServer(config Config) (*Server, error) { CommandRunner: commandHandler, PullCleaner: pullClosedExecutor, Parser: eventParser, + CommentParser: commentParser, Logger: logger, GithubWebHookSecret: []byte(config.GithubWebHookSecret), GithubRequestValidator: &DefaultGithubRequestValidator{}, From 2af2f10cbc64b527518f20a736b0ed41e0170d0d Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Wed, 28 Feb 2018 10:52:53 -0800 Subject: [PATCH 5/6] Ensure quotes are escaped for extra args. --- server/events/comment_parser.go | 3 ++- server/events/comment_parser_test.go | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/server/events/comment_parser.go b/server/events/comment_parser.go index 5efac203ca..afb2e22959 100644 --- a/server/events/comment_parser.go +++ b/server/events/comment_parser.go @@ -147,7 +147,8 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme // Quote all extra args so there isn't a security issue when we append // them to the terraform commands, ex. "; cat /etc/passwd" for _, arg := range extraArgsUnsafe { - extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, arg)) + quotesEscaped := strings.Replace(arg, `"`, `\"`, -1) + extraArgs = append(extraArgs, fmt.Sprintf(`"%s"`, quotesEscaped)) } } diff --git a/server/events/comment_parser_test.go b/server/events/comment_parser_test.go index 0d5a3f0f46..e19149fe0c 100644 --- a/server/events/comment_parser_test.go +++ b/server/events/comment_parser_test.go @@ -276,6 +276,14 @@ func TestDetermineCommand_Parsing(t *testing.T) { false, "", }, + // Test trying to escape quoting + { + "abc -- \";echo \"hi", + "default", + "", + false, + `"\";echo" "\"hi"`, + }, { "-w workspace -d dir --verbose -- arg one -two --three &&", "workspace", From a24d940f5d58acd3bf2d8d1b22152e7f8d08e8f4 Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Wed, 28 Feb 2018 10:56:36 -0800 Subject: [PATCH 6/6] Rename DetermineCommand to Parse --- server/events/comment_parser.go | 47 +++++++++++++-------- server/events/comment_parser_test.go | 36 ++++++++-------- server/events/mocks/mock_comment_parsing.go | 16 +++---- server/events_controller.go | 2 +- server/events_controller_test.go | 10 ++--- 5 files changed, 62 insertions(+), 49 deletions(-) diff --git a/server/events/comment_parser.go b/server/events/comment_parser.go index afb2e22959..31991e3569 100644 --- a/server/events/comment_parser.go +++ b/server/events/comment_parser.go @@ -12,10 +12,14 @@ import ( //go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_comment_parsing.go CommentParsing +// CommentParsing handles parsing pull request comments. type CommentParsing interface { - DetermineCommand(comment string, vcsHost vcs.Host) CommentParseResult + // Parse attempts to parse a pull request comment to see if it's an Atlantis + // commmand. + Parse(comment string, vcsHost vcs.Host) CommentParseResult } +// CommentParser implements CommentParsing type CommentParser struct { GithubUser string GithubToken string @@ -35,7 +39,7 @@ type CommentParseResult struct { Ignore bool } -// DetermineCommand parses the comment as an Atlantis command. +// Parse parses the comment as an Atlantis command. // // Valid commands contain: // - The initial "executable" name, 'run' or 'atlantis' or '@GithubUser' @@ -52,7 +56,7 @@ type CommentParseResult struct { // - atlantis plan --verbose -- -key=value -key2 value2 // // nolint: gocyclo -func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) CommentParseResult { +func (e *CommentParser) Parse(comment string, vcsHost vcs.Host) CommentParseResult { if multiLineRegex.MatchString(comment) { return CommentParseResult{Ignore: true} } @@ -137,6 +141,7 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme if err != nil { return CommentParseResult{CommentResponse: fmt.Sprintf("```\nError: %s.\nUsage of %s:\n%s\n```", err.Error(), command, flagSet.FlagUsagesWrapped(usagesCols))} } + // We only use the extra args after the --. For example given a comment: // "atlantis plan -bad-option -- -target=hi" // we only append "-target=hi" to the eventual command. @@ -152,21 +157,11 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme } } - // If dir is specified, must ensure it's a valid path. - if dir != "" { - validatedDir := filepath.Clean(dir) - // Join with . so the path is relative. This helps us if they use '/', - // and is safe to do if their path is relative since it's a no-op. - validatedDir = filepath.Join(".", validatedDir) - // Need to clean again to resolve relative validatedDirs. - validatedDir = filepath.Clean(validatedDir) - // Detect relative dirs since they're not allowed. - if strings.HasPrefix(validatedDir, "..") { - return CommentParseResult{CommentResponse: fmt.Sprintf("Error: Using a relative path %q with -d/--dir is not allowed", dir)} - } - - dir = validatedDir + dir, err = e.validateDir(dir) + if err != nil { + return CommentParseResult{CommentResponse: err.Error()} } + // Because we use the workspace name as a file, need to make sure it's // not doing something weird like being a relative dir. if strings.Contains(workspace, "..") { @@ -178,6 +173,24 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme } } +func (e *CommentParser) validateDir(dir string) (string, error) { + if dir == "" { + return dir, nil + } + validatedDir := filepath.Clean(dir) + // Join with . so the path is relative. This helps us if they use '/', + // and is safe to do if their path is relative since it's a no-op. + validatedDir = filepath.Join(".", validatedDir) + // Need to clean again to resolve relative validatedDirs. + validatedDir = filepath.Clean(validatedDir) + // Detect relative dirs since they're not allowed. + if strings.HasPrefix(validatedDir, "..") { + return "", fmt.Errorf("Error: Using a relative path %q with -d/--dir is not allowed", dir) + } + + return validatedDir, nil +} + func (e *CommentParser) stringInSlice(a string, list []string) bool { for _, b := range list { if b == a { diff --git a/server/events/comment_parser_test.go b/server/events/comment_parser_test.go index e19149fe0c..8fbd69e2ef 100644 --- a/server/events/comment_parser_test.go +++ b/server/events/comment_parser_test.go @@ -17,7 +17,7 @@ var commentParser = events.CommentParser{ GitlabToken: "gitlab-token", } -func TestDetermineCommand_Ignored(t *testing.T) { +func TestParse_Ignored(t *testing.T) { t.Log("given a comment that should be ignored we should set " + "CommentParseResult.Ignore to true") ignoreComments := []string{ @@ -28,12 +28,12 @@ func TestDetermineCommand_Ignored(t *testing.T) { "terraform plan\nbut with newlines", } for _, c := range ignoreComments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) Assert(t, r.Ignore, "expected Ignore to be true for comment %q", c) } } -func TestDetermineCommand_HelpResponse(t *testing.T) { +func TestParse_HelpResponse(t *testing.T) { t.Log("given a comment that should result in help output we " + "should set CommentParseResult.CommentResult") helpComments := []string{ @@ -47,12 +47,12 @@ func TestDetermineCommand_HelpResponse(t *testing.T) { "atlantis help plan", } for _, c := range helpComments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) Equals(t, events.HelpComment, r.CommentResponse) } } -func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { +func TestParse_DidYouMeanAtlantis(t *testing.T) { t.Log("given a comment that should result in a 'did you mean atlantis'" + "response, should set CommentParseResult.CommentResult") comments := []string{ @@ -65,13 +65,13 @@ func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { "terraform plan -w workspace -d . -- test", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) Assert(t, r.CommentResponse == events.DidYouMeanAtlantisComment, "For comment %q expected CommentResponse==%q but got %q", c, events.DidYouMeanAtlantisComment, r.CommentResponse) } } -func TestDetermineCommand_InvalidCommand(t *testing.T) { +func TestParse_InvalidCommand(t *testing.T) { t.Log("given a comment with an invalid atlantis command, should return " + "a warning.") comments := []string{ @@ -80,14 +80,14 @@ func TestDetermineCommand_InvalidCommand(t *testing.T) { "atlantis appely apply", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", strings.Fields(c)[1]) Assert(t, r.CommentResponse == exp, "For comment %q expected CommentResponse==%q but got %q", c, exp, r.CommentResponse) } } -func TestDetermineCommand_SubcommandUsage(t *testing.T) { +func TestParse_SubcommandUsage(t *testing.T) { t.Log("given a comment asking for the usage of a subcommand should " + "return help") comments := []string{ @@ -97,7 +97,7 @@ func TestDetermineCommand_SubcommandUsage(t *testing.T) { "atlantis apply --help", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := "Usage of " + strings.Fields(c)[1] Assert(t, strings.Contains(r.CommentResponse, exp), "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) @@ -106,7 +106,7 @@ func TestDetermineCommand_SubcommandUsage(t *testing.T) { } } -func TestDetermineCommand_InvalidFlags(t *testing.T) { +func TestParse_InvalidFlags(t *testing.T) { t.Log("given a comment with a valid atlantis command but invalid" + " flags, should return a warning and the proper usage") cases := []struct { @@ -131,7 +131,7 @@ func TestDetermineCommand_InvalidFlags(t *testing.T) { }, } for _, c := range cases { - r := commentParser.DetermineCommand(c.comment, vcs.Github) + r := commentParser.Parse(c.comment, vcs.Github) Assert(t, strings.Contains(r.CommentResponse, c.exp), "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, c.exp) Assert(t, strings.Contains(r.CommentResponse, "Usage of "), @@ -139,7 +139,7 @@ func TestDetermineCommand_InvalidFlags(t *testing.T) { } } -func TestDetermineCommand_RelativeDirPath(t *testing.T) { +func TestParse_RelativeDirPath(t *testing.T) { t.Log("if -d is used with a relative path, should return an error") comments := []string{ "atlantis plan -d ..", @@ -153,14 +153,14 @@ func TestDetermineCommand_RelativeDirPath(t *testing.T) { "atlantis apply -d a/../..", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := "Error: Using a relative path" Assert(t, strings.Contains(r.CommentResponse, exp), "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) } } -func TestDetermineCommand_InvalidWorkspace(t *testing.T) { +func TestParse_InvalidWorkspace(t *testing.T) { t.Log("if -w is used with '..', should return an error") comments := []string{ "atlantis plan -w ..", @@ -171,14 +171,14 @@ func TestDetermineCommand_InvalidWorkspace(t *testing.T) { "atlantis apply -w ../../../etc/passwd", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := "Error: Value for -w/--workspace can't contain '..'" Assert(t, r.CommentResponse == exp, "For comment %q expected CommentResponse %q to be %q", c, r.CommentResponse, exp) } } -func TestDetermineCommand_Parsing(t *testing.T) { +func TestParse_Parsing(t *testing.T) { cases := []struct { flags string expWorkspace string @@ -346,7 +346,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { for _, test := range cases { for _, cmdName := range []string{"plan", "apply"} { comment := fmt.Sprintf("atlantis %s %s", cmdName, test.flags) - r := commentParser.DetermineCommand(comment, vcs.Github) + r := commentParser.Parse(comment, vcs.Github) Assert(t, r.CommentResponse == "", "CommentResponse should have been empty but was %q for comment %q", r.CommentResponse, comment) Assert(t, test.expDir == r.Command.Dir, "exp dir to equal %q but was %q for comment %q", test.expDir, r.Command.Dir, comment) Assert(t, test.expWorkspace == r.Command.Workspace, "exp workspace to equal %q but was %q for comment %q", test.expWorkspace, r.Command.Workspace, comment) diff --git a/server/events/mocks/mock_comment_parsing.go b/server/events/mocks/mock_comment_parsing.go index e61cda2267..54dabbd7de 100644 --- a/server/events/mocks/mock_comment_parsing.go +++ b/server/events/mocks/mock_comment_parsing.go @@ -19,9 +19,9 @@ func NewMockCommentParsing() *MockCommentParsing { return &MockCommentParsing{fail: pegomock.GlobalFailHandler} } -func (mock *MockCommentParsing) DetermineCommand(comment string, vcsHost vcs.Host) events.CommentParseResult { +func (mock *MockCommentParsing) Parse(comment string, vcsHost vcs.Host) events.CommentParseResult { params := []pegomock.Param{comment, vcsHost} - result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineCommand", params, []reflect.Type{reflect.TypeOf((*events.CommentParseResult)(nil)).Elem()}) + result := pegomock.GetGenericMockFrom(mock).Invoke("Parse", params, []reflect.Type{reflect.TypeOf((*events.CommentParseResult)(nil)).Elem()}) var ret0 events.CommentParseResult if len(result) != 0 { if result[0] != nil { @@ -49,23 +49,23 @@ type VerifierCommentParsing struct { inOrderContext *pegomock.InOrderContext } -func (verifier *VerifierCommentParsing) DetermineCommand(comment string, vcsHost vcs.Host) *CommentParsing_DetermineCommand_OngoingVerification { +func (verifier *VerifierCommentParsing) Parse(comment string, vcsHost vcs.Host) *CommentParsing_Parse_OngoingVerification { params := []pegomock.Param{comment, vcsHost} - methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "DetermineCommand", params) - return &CommentParsing_DetermineCommand_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "Parse", params) + return &CommentParsing_Parse_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } -type CommentParsing_DetermineCommand_OngoingVerification struct { +type CommentParsing_Parse_OngoingVerification struct { mock *MockCommentParsing methodInvocations []pegomock.MethodInvocation } -func (c *CommentParsing_DetermineCommand_OngoingVerification) GetCapturedArguments() (string, vcs.Host) { +func (c *CommentParsing_Parse_OngoingVerification) GetCapturedArguments() (string, vcs.Host) { comment, vcsHost := c.GetAllCapturedArguments() return comment[len(comment)-1], vcsHost[len(vcsHost)-1] } -func (c *CommentParsing_DetermineCommand_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []vcs.Host) { +func (c *CommentParsing_Parse_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []vcs.Host) { params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) if len(params) > 0 { _param0 = make([]string, len(params[0])) diff --git a/server/events_controller.go b/server/events_controller.go index 084e1d172e..145c5f263b 100644 --- a/server/events_controller.go +++ b/server/events_controller.go @@ -152,7 +152,7 @@ func (e *EventsController) HandleGitlabCommentEvent(w http.ResponseWriter, event } func (e *EventsController) handleCommentEvent(w http.ResponseWriter, baseRepo models.Repo, headRepo models.Repo, user models.User, pullNum int, comment string, vcsHost vcs.Host) { - parseResult := e.CommentParser.DetermineCommand(comment, vcsHost) + parseResult := e.CommentParser.Parse(comment, vcsHost) if parseResult.Ignore { truncated := comment if len(truncated) > 40 { diff --git a/server/events_controller_test.go b/server/events_controller_test.go index c9f46b907f..5179ed59d0 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -124,7 +124,7 @@ func TestPost_GitlabCommentInvalidCommand(t *testing.T) { e, _, gl, _, _, _, _, cp := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(cp.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{Ignore: true}) + When(cp.Parse("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") @@ -137,7 +137,7 @@ func TestPost_GithubCommentInvalidCommand(t *testing.T) { event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(models.Repo{}, models.User{}, 1, nil) - When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{Ignore: true}) + When(cp.Parse("", vcs.Github)).ThenReturn(events.CommentParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") @@ -148,7 +148,7 @@ func TestPost_GitlabCommentResponse(t *testing.T) { e, _, gl, _, _, _, vcsClient, cp := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(cp.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) + When(cp.Parse("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) w := httptest.NewRecorder() e.Post(w, eventsReq) vcsClient.VerifyWasCalledOnce().CreateComment(models.Repo{}, 0, "a comment", vcs.Gitlab) @@ -164,7 +164,7 @@ func TestPost_GithubCommentResponse(t *testing.T) { baseRepo := models.Repo{} user := models.User{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) + When(cp.Parse("", vcs.Github)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) w := httptest.NewRecorder() e.Post(w, eventsReq) @@ -196,7 +196,7 @@ func TestPost_GithubCommentSuccess(t *testing.T) { user := models.User{} cmd := events.Command{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{Command: &cmd}) + When(cp.Parse("", vcs.Github)).ThenReturn(events.CommentParseResult{Command: &cmd}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Processing...")