diff --git a/server/events/command_runner.go b/server/events/command_runner.go index e7cf6bd2f3..5d78eebe3f 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -108,9 +108,10 @@ type DefaultCommandRunner struct { // SilenceForkPRErrorsFlag is the name of the flag that controls fork PR's. We use // this in our error message back to the user on a forked PR so they know // how to disable error comment - SilenceForkPRErrorsFlag string - CommentCommandRunnerByCmd map[models.CommandName]CommentCommandRunner - Drainer *Drainer + SilenceForkPRErrorsFlag string + CommentCommandRunnerByCmd map[models.CommandName]CommentCommandRunner + Drainer *Drainer + PreWorkflowHooksCommandRunner PreWorkflowHooksCommandRunner } // RunAutoplanCommand runs plan and policy_checks when a pull request is opened or updated. @@ -139,12 +140,14 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo return } - autoPlanRunner := buildCommentCommandRunner(c, models.PlanCommand) - if autoPlanRunner == nil { - ctx.Log.Err("invalid autoplan command") - return + err := c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx) + + if err != nil { + ctx.Log.Err("Error running pre-workflow hooks %s. Proceeding with %s command.", err, models.PlanCommand) } + autoPlanRunner := buildCommentCommandRunner(c, models.PlanCommand) + autoPlanRunner.Run(ctx, nil) } @@ -182,12 +185,14 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead return } - cmdRunner := buildCommentCommandRunner(c, cmd.CommandName()) - if cmdRunner == nil { - ctx.Log.Err("command %s is not supported", cmd.Name.String()) - return + err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx) + + if err != nil { + ctx.Log.Err("Error running pre-workflow hooks %s. Proceeding with %s command.", err, cmd.Name.String()) } + cmdRunner := buildCommentCommandRunner(c, cmd.CommandName()) + cmdRunner.Run(ctx, cmd) } diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index 47aba3c1d5..08e3566c40 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -61,6 +61,7 @@ var approvePoliciesCommandRunner *events.ApprovePoliciesCommandRunner var planCommandRunner *events.PlanCommandRunner var applyCommandRunner *events.ApplyCommandRunner var unlockCommandRunner *events.UnlockCommandRunner +var preWorkflowHooksCommandRunner events.PreWorkflowHooksCommandRunner func setup(t *testing.T) *vcsmocks.MockClient { RegisterMockTestingT(t) @@ -161,17 +162,22 @@ func setup(t *testing.T) *vcsmocks.MockClient { models.UnlockCommand: unlockCommandRunner, } + preWorkflowHooksCommandRunner = mocks.NewMockPreWorkflowHooksCommandRunner() + + When(preWorkflowHooksCommandRunner.RunPreHooks(matchers.AnyPtrToEventsCommandContext())).ThenReturn(nil) + ch = events.DefaultCommandRunner{ - VCSClient: vcsClient, - CommentCommandRunnerByCmd: commentCommandRunnerByCmd, - EventParser: eventParsing, - GithubPullGetter: githubGetter, - GitlabMergeRequestGetter: gitlabGetter, - AzureDevopsPullGetter: azuredevopsGetter, - Logger: logger, - AllowForkPRs: false, - AllowForkPRsFlag: "allow-fork-prs-flag", - Drainer: drainer, + VCSClient: vcsClient, + CommentCommandRunnerByCmd: commentCommandRunnerByCmd, + EventParser: eventParsing, + GithubPullGetter: githubGetter, + GitlabMergeRequestGetter: gitlabGetter, + AzureDevopsPullGetter: azuredevopsGetter, + Logger: logger, + AllowForkPRs: false, + AllowForkPRsFlag: "allow-fork-prs-flag", + Drainer: drainer, + PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, } return vcsClient } diff --git a/server/events/delete_lock_command.go b/server/events/delete_lock_command.go index 7033017db4..5ed05f0bc4 100644 --- a/server/events/delete_lock_command.go +++ b/server/events/delete_lock_command.go @@ -45,6 +45,7 @@ func (l *DefaultDeleteLockCommand) DeleteLocksByPull(repoFullName string, pullNu return err } if len(locks) == 0 { + l.Logger.Debug("No locks found for pull") return nil } @@ -61,6 +62,7 @@ func (l *DefaultDeleteLockCommand) deleteWorkingDir(lock models.ProjectLock) { // installations of Atlantis will have locks in their DB that do not have // this field on PullRequest. We skip deleting the working dir in this case. if lock.Pull.BaseRepo == (models.Repo{}) { + l.Logger.Debug("Not deleting the working dir.") return } unlock, err := l.WorkingDirLocker.TryLock(lock.Pull.BaseRepo.FullName, lock.Pull.Num, lock.Workspace) diff --git a/server/events/mocks/matchers/ptr_to_events_commandcontext.go b/server/events/mocks/matchers/ptr_to_events_commandcontext.go index 896b495636..60fe569249 100644 --- a/server/events/mocks/matchers/ptr_to_events_commandcontext.go +++ b/server/events/mocks/matchers/ptr_to_events_commandcontext.go @@ -3,8 +3,9 @@ package matchers import ( "github.com/petergtz/pegomock" - events "github.com/runatlantis/atlantis/server/events" "reflect" + + events "github.com/runatlantis/atlantis/server/events" ) func AnyPtrToEventsCommandContext() *events.CommandContext { @@ -18,3 +19,15 @@ func EqPtrToEventsCommandContext(value *events.CommandContext) *events.CommandCo var nullValue *events.CommandContext return nullValue } + +func NotEqPtrToEventsCommandContext(value *events.CommandContext) *events.CommandContext { + pegomock.RegisterMatcher(&pegomock.NotEqMatcher{Value: value}) + var nullValue *events.CommandContext + return nullValue +} + +func PtrToEventsCommandContextThat(matcher pegomock.ArgumentMatcher) *events.CommandContext { + pegomock.RegisterMatcher(matcher) + var nullValue *events.CommandContext + return nullValue +} diff --git a/server/events/mocks/mock_pre_workflows_hooks_command_runner.go b/server/events/mocks/mock_pre_workflows_hooks_command_runner.go index 3019cc1a79..ebbb9f8766 100644 --- a/server/events/mocks/mock_pre_workflows_hooks_command_runner.go +++ b/server/events/mocks/mock_pre_workflows_hooks_command_runner.go @@ -5,7 +5,7 @@ package mocks import ( pegomock "github.com/petergtz/pegomock" - models "github.com/runatlantis/atlantis/server/events/models" + events "github.com/runatlantis/atlantis/server/events" "reflect" "time" ) @@ -27,12 +27,19 @@ func (mock *MockPreWorkflowHooksCommandRunner) SetFailHandler(fh pegomock.FailHa } func (mock *MockPreWorkflowHooksCommandRunner) FailHandler() pegomock.FailHandler { return mock.fail } -func (mock *MockPreWorkflowHooksCommandRunner) RunPreHooks(baseRepo models.Repo, headRepo models.Repo, pull models.PullRequest, user models.User) { +func (mock *MockPreWorkflowHooksCommandRunner) RunPreHooks(ctx *events.CommandContext) error { if mock == nil { panic("mock must not be nil. Use myMock := NewMockPreWorkflowHooksCommandRunner().") } - params := []pegomock.Param{baseRepo, headRepo, pull, user} - pegomock.GetGenericMockFrom(mock).Invoke("RunPreHooks", params, []reflect.Type{}) + params := []pegomock.Param{ctx} + result := pegomock.GetGenericMockFrom(mock).Invoke("RunPreHooks", params, []reflect.Type{reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 error + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].(error) + } + } + return ret0 } func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledOnce() *VerifierMockPreWorkflowHooksCommandRunner { @@ -42,14 +49,14 @@ func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledOnce() *VerifierMo } } -func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalled(invocationCountMatcher pegomock.Matcher) *VerifierMockPreWorkflowHooksCommandRunner { +func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalled(invocationCountMatcher pegomock.InvocationCountMatcher) *VerifierMockPreWorkflowHooksCommandRunner { return &VerifierMockPreWorkflowHooksCommandRunner{ mock: mock, invocationCountMatcher: invocationCountMatcher, } } -func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledInOrder(invocationCountMatcher pegomock.Matcher, inOrderContext *pegomock.InOrderContext) *VerifierMockPreWorkflowHooksCommandRunner { +func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledInOrder(invocationCountMatcher pegomock.InvocationCountMatcher, inOrderContext *pegomock.InOrderContext) *VerifierMockPreWorkflowHooksCommandRunner { return &VerifierMockPreWorkflowHooksCommandRunner{ mock: mock, invocationCountMatcher: invocationCountMatcher, @@ -57,7 +64,7 @@ func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledInOrder(invocation } } -func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledEventually(invocationCountMatcher pegomock.Matcher, timeout time.Duration) *VerifierMockPreWorkflowHooksCommandRunner { +func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledEventually(invocationCountMatcher pegomock.InvocationCountMatcher, timeout time.Duration) *VerifierMockPreWorkflowHooksCommandRunner { return &VerifierMockPreWorkflowHooksCommandRunner{ mock: mock, invocationCountMatcher: invocationCountMatcher, @@ -67,13 +74,13 @@ func (mock *MockPreWorkflowHooksCommandRunner) VerifyWasCalledEventually(invocat type VerifierMockPreWorkflowHooksCommandRunner struct { mock *MockPreWorkflowHooksCommandRunner - invocationCountMatcher pegomock.Matcher + invocationCountMatcher pegomock.InvocationCountMatcher inOrderContext *pegomock.InOrderContext timeout time.Duration } -func (verifier *VerifierMockPreWorkflowHooksCommandRunner) RunPreHooks(baseRepo models.Repo, headRepo models.Repo, pull models.PullRequest, user models.User) *MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification { - params := []pegomock.Param{baseRepo, headRepo, pull, user} +func (verifier *VerifierMockPreWorkflowHooksCommandRunner) RunPreHooks(ctx *events.CommandContext) *MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification { + params := []pegomock.Param{ctx} methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "RunPreHooks", params, verifier.timeout) return &MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } @@ -83,29 +90,17 @@ type MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification struct { methodInvocations []pegomock.MethodInvocation } -func (c *MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification) GetCapturedArguments() (models.Repo, models.Repo, models.PullRequest, models.User) { - baseRepo, headRepo, pull, user := c.GetAllCapturedArguments() - return baseRepo[len(baseRepo)-1], headRepo[len(headRepo)-1], pull[len(pull)-1], user[len(user)-1] +func (c *MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification) GetCapturedArguments() *events.CommandContext { + ctx := c.GetAllCapturedArguments() + return ctx[len(ctx)-1] } -func (c *MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification) GetAllCapturedArguments() (_param0 []models.Repo, _param1 []models.Repo, _param2 []models.PullRequest, _param3 []models.User) { +func (c *MockPreWorkflowHooksCommandRunner_RunPreHooks_OngoingVerification) GetAllCapturedArguments() (_param0 []*events.CommandContext) { params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) if len(params) > 0 { - _param0 = make([]models.Repo, len(c.methodInvocations)) + _param0 = make([]*events.CommandContext, len(c.methodInvocations)) for u, param := range params[0] { - _param0[u] = param.(models.Repo) - } - _param1 = make([]models.Repo, len(c.methodInvocations)) - for u, param := range params[1] { - _param1[u] = param.(models.Repo) - } - _param2 = make([]models.PullRequest, len(c.methodInvocations)) - for u, param := range params[2] { - _param2[u] = param.(models.PullRequest) - } - _param3 = make([]models.User, len(c.methodInvocations)) - for u, param := range params[3] { - _param3[u] = param.(models.User) + _param0[u] = param.(*events.CommandContext) } } return diff --git a/server/events/pre_workflow_hooks_command_runner.go b/server/events/pre_workflow_hooks_command_runner.go index 739359b408..6c0b67f0d7 100644 --- a/server/events/pre_workflow_hooks_command_runner.go +++ b/server/events/pre_workflow_hooks_command_runner.go @@ -1,93 +1,79 @@ package events import ( - "fmt" - "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/events/runtime" "github.com/runatlantis/atlantis/server/events/vcs" "github.com/runatlantis/atlantis/server/events/yaml/valid" - "github.com/runatlantis/atlantis/server/logging" - "github.com/runatlantis/atlantis/server/recovery" ) //go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_pre_workflows_hooks_command_runner.go PreWorkflowHooksCommandRunner type PreWorkflowHooksCommandRunner interface { - RunPreHooks( - baseRepo models.Repo, - headRepo models.Repo, - pull models.PullRequest, - user models.User, - ) + RunPreHooks(ctx *CommandContext) error } // DefaultPreWorkflowHooksCommandRunner is the first step when processing a workflow hook commands. type DefaultPreWorkflowHooksCommandRunner struct { VCSClient vcs.Client - Logger logging.SimpleLogging WorkingDirLocker WorkingDirLocker WorkingDir WorkingDir GlobalCfg valid.GlobalCfg - Drainer *Drainer - PreWorkflowHookRunner *runtime.PreWorkflowHookRunner + PreWorkflowHookRunner runtime.PreWorkflowHookRunner } // RunPreHooks runs pre_workflow_hooks when PR is opened or updated. func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks( - baseRepo models.Repo, - headRepo models.Repo, - pull models.PullRequest, - user models.User, -) { - if opStarted := w.Drainer.StartOp(); !opStarted { - if commentErr := w.VCSClient.CreateComment(baseRepo, pull.Num, ShutdownComment, "pre_workflow_hooks"); commentErr != nil { - w.Logger.Log(logging.Error, "unable to comment that Atlantis is shutting down: %s", commentErr) + ctx *CommandContext, +) error { + pull := ctx.Pull + baseRepo := pull.BaseRepo + headRepo := ctx.HeadRepo + user := ctx.User + log := ctx.Log + + preWorkflowHooks := make([]*valid.PreWorkflowHook, 0) + for _, repo := range w.GlobalCfg.Repos { + if repo.IDMatches(baseRepo.ID()) && len(repo.PreWorkflowHooks) > 0 { + preWorkflowHooks = append(preWorkflowHooks, repo.PreWorkflowHooks...) } - return } - defer w.Drainer.OpDone() - log := w.buildLogger(baseRepo.FullName, pull.Num) - defer w.logPanics(baseRepo, pull.Num, log) + // short circuit any other calls if there are no pre-hooks configured + if len(preWorkflowHooks) == 0 { + return nil + } - log.Info("running pre hooks") + log.Debug("pre-hooks configured, running...") unlockFn, err := w.WorkingDirLocker.TryLock(baseRepo.FullName, pull.Num, DefaultWorkspace) if err != nil { - log.Warn("workspace is locked") - return + return err } log.Debug("got workspace lock") defer unlockFn() repoDir, _, err := w.WorkingDir.Clone(log, headRepo, pull, DefaultWorkspace) if err != nil { - log.Err("unable to run pre workflow hooks: %s", err) - return + return err } - preWorkflowHooks := make([]*valid.PreWorkflowHook, 0) - for _, repo := range w.GlobalCfg.Repos { - if repo.IDMatches(baseRepo.ID()) && len(repo.PreWorkflowHooks) > 0 { - preWorkflowHooks = append(preWorkflowHooks, repo.PreWorkflowHooks...) - } - } - - ctx := models.PreWorkflowHookCommandContext{ - BaseRepo: baseRepo, - HeadRepo: headRepo, - Log: log, - Pull: pull, - User: user, - Verbose: false, - } - - err = w.runHooks(ctx, preWorkflowHooks, repoDir) + err = w.runHooks( + models.PreWorkflowHookCommandContext{ + BaseRepo: baseRepo, + HeadRepo: headRepo, + Log: log, + Pull: pull, + User: user, + Verbose: false, + }, + preWorkflowHooks, repoDir) if err != nil { - log.Err("pre workflow hook run error results: %s", err) + return err } + + return nil } func (w *DefaultPreWorkflowHooksCommandRunner) runHooks( @@ -100,30 +86,9 @@ func (w *DefaultPreWorkflowHooksCommandRunner) runHooks( _, err := w.PreWorkflowHookRunner.Run(ctx, hook.RunCommand, repoDir) if err != nil { - return nil + return err } } return nil } - -func (w *DefaultPreWorkflowHooksCommandRunner) buildLogger(repoFullName string, pullNum int) *logging.SimpleLogger { - src := fmt.Sprintf("%s#%d", repoFullName, pullNum) - return w.Logger.NewLogger(src, true, w.Logger.GetLevel()) -} - -// logPanics logs and creates a comment on the pull request for panics. -func (w *DefaultPreWorkflowHooksCommandRunner) logPanics(baseRepo models.Repo, pullNum int, logger logging.SimpleLogging) { - if err := recover(); err != nil { - stack := recovery.Stack(3) - logger.Err("PANIC: %s\n%s", err, stack) - if commentErr := w.VCSClient.CreateComment( - baseRepo, - pullNum, - fmt.Sprintf("**Error: goroutine panic. This is a bug.**\n```\n%s\n%s```", err, stack), - "", - ); commentErr != nil { - logger.Err("unable to comment: %s", commentErr) - } - } -} diff --git a/server/events/pre_workflow_hooks_command_runner_test.go b/server/events/pre_workflow_hooks_command_runner_test.go index e8290359c9..b0c3a7dc98 100644 --- a/server/events/pre_workflow_hooks_command_runner_test.go +++ b/server/events/pre_workflow_hooks_command_runner_test.go @@ -1,87 +1,221 @@ package events_test import ( - "fmt" - "strings" + "errors" "testing" . "github.com/petergtz/pegomock" "github.com/runatlantis/atlantis/server/events" - "github.com/runatlantis/atlantis/server/events/matchers" "github.com/runatlantis/atlantis/server/events/mocks" + "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/events/models/fixtures" - "github.com/runatlantis/atlantis/server/events/runtime" + runtime_mocks "github.com/runatlantis/atlantis/server/events/runtime/mocks" vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks" + "github.com/runatlantis/atlantis/server/events/yaml/valid" "github.com/runatlantis/atlantis/server/logging" - logmocks "github.com/runatlantis/atlantis/server/logging/mocks" . "github.com/runatlantis/atlantis/testing" ) var wh events.DefaultPreWorkflowHooksCommandRunner var whWorkingDir *mocks.MockWorkingDir var whWorkingDirLocker *mocks.MockWorkingDirLocker -var whDrainer *events.Drainer +var whPreWorkflowHookRunner *runtime_mocks.MockPreWorkflowHookRunner -func preWorkflowHooksSetup(t *testing.T) *vcsmocks.MockClient { +func preWorkflowHooksSetup(t *testing.T) { RegisterMockTestingT(t) vcsClient := vcsmocks.NewMockClient() - logger := logmocks.NewMockSimpleLogging() whWorkingDir = mocks.NewMockWorkingDir() whWorkingDirLocker = mocks.NewMockWorkingDirLocker() - whDrainer = &events.Drainer{} + whPreWorkflowHookRunner = runtime_mocks.NewMockPreWorkflowHookRunner() wh = events.DefaultPreWorkflowHooksCommandRunner{ VCSClient: vcsClient, - Logger: logger, WorkingDirLocker: whWorkingDirLocker, WorkingDir: whWorkingDir, - Drainer: whDrainer, - PreWorkflowHookRunner: &runtime.PreWorkflowHookRunner{}, + PreWorkflowHookRunner: whPreWorkflowHookRunner, } - return vcsClient } -func TestPreWorkflowHooksCommand_LogPanics(t *testing.T) { - t.Log("if there is a panic it is commented back on the pull request") - vcsClient := preWorkflowHooksSetup(t) - logger := wh.Logger.NewLogger("log", false, logging.LogLevel(1)) - - When(whWorkingDir.Clone( - logger, - fixtures.GithubRepo, - fixtures.Pull, - events.DefaultWorkspace, - )).ThenPanic("panic test - if you're seeing this in a test failure this isn't the failing test") - - wh.RunPreHooks(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) - _, _, comment, _ := vcsClient.VerifyWasCalledOnce().CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), AnyString()).GetCapturedArguments() - Assert(t, strings.Contains(comment, "Error: goroutine panic"), fmt.Sprintf("comment should be about a goroutine panic but was %q", comment)) +func newBool(b bool) *bool { + return &b } -// Test that if one plan fails and we are using automerge, that -// we delete the plans. func TestRunPreHooks_Clone(t *testing.T) { - preWorkflowHooksSetup(t) - logger := wh.Logger.NewLogger("log", false, logging.LogLevel(1)) - When(whWorkingDir.Clone(logger, fixtures.GithubRepo, fixtures.Pull, events.DefaultWorkspace)). - ThenReturn("path/to/repo", false, nil) - wh.RunPreHooks(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) -} + log := logging.NewNoopLogger() -func TestRunPreHooks_DrainOngoing(t *testing.T) { - t.Log("if drain is ongoing then a message should be displayed") - vcsClient := preWorkflowHooksSetup(t) - whDrainer.ShutdownBlocking() - wh.RunPreHooks(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) - vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, fixtures.Pull.Num, "Atlantis server is shutting down, please try again later.", "pre_workflow_hooks") -} + var newPull = fixtures.Pull + newPull.BaseRepo = fixtures.GithubRepo + + ctx := &events.CommandContext{ + Pull: newPull, + HeadRepo: fixtures.GithubRepo, + User: fixtures.User, + Log: log, + } + + testHook := valid.PreWorkflowHook{ + StepName: "test", + RunCommand: "some command", + } + + pCtx := models.PreWorkflowHookCommandContext{ + BaseRepo: fixtures.GithubRepo, + HeadRepo: fixtures.GithubRepo, + Pull: newPull, + Log: log, + User: fixtures.User, + Verbose: false, + } + + repoDir := "path/to/repo" + result := "some result" + + t.Run("success hooks in cfg", func(t *testing.T) { + preWorkflowHooksSetup(t) + + var unlockCalled *bool = newBool(false) + unlockFn := func() { + unlockCalled = newBool(true) + } + + globalCfg := valid.GlobalCfg{ + Repos: []valid.Repo{ + { + ID: fixtures.GithubRepo.ID(), + PreWorkflowHooks: []*valid.PreWorkflowHook{ + &testHook, + }, + }, + }, + } + + wh.GlobalCfg = globalCfg + + When(whWorkingDirLocker.TryLock(fixtures.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace)).ThenReturn(unlockFn, nil) + When(whWorkingDir.Clone(log, fixtures.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil) + When(whPreWorkflowHookRunner.Run(pCtx, testHook.RunCommand, repoDir)).ThenReturn(result, nil) + + err := wh.RunPreHooks(ctx) + + Ok(t, err) + whPreWorkflowHookRunner.VerifyWasCalledOnce().Run(pCtx, testHook.RunCommand, repoDir) + Assert(t, *unlockCalled == true, "unlock function called") + }) + t.Run("success hooks not in cfg", func(t *testing.T) { + preWorkflowHooksSetup(t) + globalCfg := valid.GlobalCfg{ + Repos: []valid.Repo{ + // one with hooks but mismatched id + { + ID: "id1", + PreWorkflowHooks: []*valid.PreWorkflowHook{ + &testHook, + }, + }, + // one with the correct id but no hooks + { + ID: fixtures.GithubRepo.ID(), + PreWorkflowHooks: []*valid.PreWorkflowHook{}, + }, + }, + } + + wh.GlobalCfg = globalCfg + + err := wh.RunPreHooks(ctx) + + Ok(t, err) + + whPreWorkflowHookRunner.VerifyWasCalled(Never()).Run(pCtx, testHook.RunCommand, repoDir) + whWorkingDirLocker.VerifyWasCalled(Never()).TryLock(fixtures.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace) + whWorkingDir.VerifyWasCalled(Never()).Clone(log, fixtures.GithubRepo, newPull, events.DefaultWorkspace) + }) + t.Run("error locking work dir", func(t *testing.T) { + preWorkflowHooksSetup(t) + + globalCfg := valid.GlobalCfg{ + Repos: []valid.Repo{ + { + ID: fixtures.GithubRepo.ID(), + PreWorkflowHooks: []*valid.PreWorkflowHook{ + &testHook, + }, + }, + }, + } + + wh.GlobalCfg = globalCfg + + When(whWorkingDirLocker.TryLock(fixtures.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace)).ThenReturn(func() {}, errors.New("some error")) + + err := wh.RunPreHooks(ctx) + + Assert(t, err != nil, "error not nil") + whWorkingDir.VerifyWasCalled(Never()).Clone(log, fixtures.GithubRepo, newPull, events.DefaultWorkspace) + whPreWorkflowHookRunner.VerifyWasCalled(Never()).Run(pCtx, testHook.RunCommand, repoDir) + }) + + t.Run("error cloning", func(t *testing.T) { + preWorkflowHooksSetup(t) + + var unlockCalled *bool = newBool(false) + unlockFn := func() { + unlockCalled = newBool(true) + } + + globalCfg := valid.GlobalCfg{ + Repos: []valid.Repo{ + { + ID: fixtures.GithubRepo.ID(), + PreWorkflowHooks: []*valid.PreWorkflowHook{ + &testHook, + }, + }, + }, + } + + wh.GlobalCfg = globalCfg + + When(whWorkingDirLocker.TryLock(fixtures.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace)).ThenReturn(unlockFn, nil) + When(whWorkingDir.Clone(log, fixtures.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, errors.New("some error")) + + err := wh.RunPreHooks(ctx) + + Assert(t, err != nil, "error not nil") + + whPreWorkflowHookRunner.VerifyWasCalled(Never()).Run(pCtx, testHook.RunCommand, repoDir) + Assert(t, *unlockCalled == true, "unlock function called") + }) + + t.Run("error running pre hook", func(t *testing.T) { + preWorkflowHooksSetup(t) + + var unlockCalled *bool = newBool(false) + unlockFn := func() { + unlockCalled = newBool(true) + } + + globalCfg := valid.GlobalCfg{ + Repos: []valid.Repo{ + { + ID: fixtures.GithubRepo.ID(), + PreWorkflowHooks: []*valid.PreWorkflowHook{ + &testHook, + }, + }, + }, + } + + wh.GlobalCfg = globalCfg + + When(whWorkingDirLocker.TryLock(fixtures.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace)).ThenReturn(unlockFn, nil) + When(whWorkingDir.Clone(log, fixtures.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil) + When(whPreWorkflowHookRunner.Run(pCtx, testHook.RunCommand, repoDir)).ThenReturn(result, errors.New("some error")) + + err := wh.RunPreHooks(ctx) -func TestRunPreHooks_DrainNotOngoing(t *testing.T) { - t.Log("if drain is not ongoing then remove ongoing operation must be called even if panic occured") - preWorkflowHooksSetup(t) - When(whWorkingDir.Clone(logger, fixtures.GithubRepo, fixtures.Pull, events.DefaultWorkspace)).ThenPanic("panic test - if you're seeing this in a test failure this isn't the failing test") - wh.RunPreHooks(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) - whWorkingDir.VerifyWasCalledOnce().Clone(logger, fixtures.GithubRepo, fixtures.Pull, events.DefaultWorkspace) - Equals(t, 0, whDrainer.GetStatus().InProgressOps) + Assert(t, err != nil, "error not nil") + Assert(t, *unlockCalled == true, "unlock function called") + }) } diff --git a/server/events/runtime/mocks/matchers/models_preworkflowhookcommandcontext.go b/server/events/runtime/mocks/matchers/models_preworkflowhookcommandcontext.go new file mode 100644 index 0000000000..8a57120c7c --- /dev/null +++ b/server/events/runtime/mocks/matchers/models_preworkflowhookcommandcontext.go @@ -0,0 +1,33 @@ +// Code generated by pegomock. DO NOT EDIT. +package matchers + +import ( + "github.com/petergtz/pegomock" + "reflect" + + models "github.com/runatlantis/atlantis/server/events/models" +) + +func AnyModelsPreWorkflowHookCommandContext() models.PreWorkflowHookCommandContext { + pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(models.PreWorkflowHookCommandContext))(nil)).Elem())) + var nullValue models.PreWorkflowHookCommandContext + return nullValue +} + +func EqModelsPreWorkflowHookCommandContext(value models.PreWorkflowHookCommandContext) models.PreWorkflowHookCommandContext { + pegomock.RegisterMatcher(&pegomock.EqMatcher{Value: value}) + var nullValue models.PreWorkflowHookCommandContext + return nullValue +} + +func NotEqModelsPreWorkflowHookCommandContext(value models.PreWorkflowHookCommandContext) models.PreWorkflowHookCommandContext { + pegomock.RegisterMatcher(&pegomock.NotEqMatcher{Value: value}) + var nullValue models.PreWorkflowHookCommandContext + return nullValue +} + +func ModelsPreWorkflowHookCommandContextThat(matcher pegomock.ArgumentMatcher) models.PreWorkflowHookCommandContext { + pegomock.RegisterMatcher(matcher) + var nullValue models.PreWorkflowHookCommandContext + return nullValue +} diff --git a/server/events/runtime/mocks/mock_pre_workflows_hook_runner.go b/server/events/runtime/mocks/mock_pre_workflows_hook_runner.go new file mode 100644 index 0000000000..dcb5c32935 --- /dev/null +++ b/server/events/runtime/mocks/mock_pre_workflows_hook_runner.go @@ -0,0 +1,117 @@ +// Code generated by pegomock. DO NOT EDIT. +// Source: github.com/runatlantis/atlantis/server/events/runtime (interfaces: PreWorkflowHookRunner) + +package mocks + +import ( + pegomock "github.com/petergtz/pegomock" + models "github.com/runatlantis/atlantis/server/events/models" + "reflect" + "time" +) + +type MockPreWorkflowHookRunner struct { + fail func(message string, callerSkip ...int) +} + +func NewMockPreWorkflowHookRunner(options ...pegomock.Option) *MockPreWorkflowHookRunner { + mock := &MockPreWorkflowHookRunner{} + for _, option := range options { + option.Apply(mock) + } + return mock +} + +func (mock *MockPreWorkflowHookRunner) SetFailHandler(fh pegomock.FailHandler) { mock.fail = fh } +func (mock *MockPreWorkflowHookRunner) FailHandler() pegomock.FailHandler { return mock.fail } + +func (mock *MockPreWorkflowHookRunner) Run(ctx models.PreWorkflowHookCommandContext, command string, path string) (string, error) { + if mock == nil { + panic("mock must not be nil. Use myMock := NewMockPreWorkflowHookRunner().") + } + params := []pegomock.Param{ctx, command, path} + result := pegomock.GetGenericMockFrom(mock).Invoke("Run", params, []reflect.Type{reflect.TypeOf((*string)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 string + var ret1 error + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].(string) + } + if result[1] != nil { + ret1 = result[1].(error) + } + } + return ret0, ret1 +} + +func (mock *MockPreWorkflowHookRunner) VerifyWasCalledOnce() *VerifierMockPreWorkflowHookRunner { + return &VerifierMockPreWorkflowHookRunner{ + mock: mock, + invocationCountMatcher: pegomock.Times(1), + } +} + +func (mock *MockPreWorkflowHookRunner) VerifyWasCalled(invocationCountMatcher pegomock.InvocationCountMatcher) *VerifierMockPreWorkflowHookRunner { + return &VerifierMockPreWorkflowHookRunner{ + mock: mock, + invocationCountMatcher: invocationCountMatcher, + } +} + +func (mock *MockPreWorkflowHookRunner) VerifyWasCalledInOrder(invocationCountMatcher pegomock.InvocationCountMatcher, inOrderContext *pegomock.InOrderContext) *VerifierMockPreWorkflowHookRunner { + return &VerifierMockPreWorkflowHookRunner{ + mock: mock, + invocationCountMatcher: invocationCountMatcher, + inOrderContext: inOrderContext, + } +} + +func (mock *MockPreWorkflowHookRunner) VerifyWasCalledEventually(invocationCountMatcher pegomock.InvocationCountMatcher, timeout time.Duration) *VerifierMockPreWorkflowHookRunner { + return &VerifierMockPreWorkflowHookRunner{ + mock: mock, + invocationCountMatcher: invocationCountMatcher, + timeout: timeout, + } +} + +type VerifierMockPreWorkflowHookRunner struct { + mock *MockPreWorkflowHookRunner + invocationCountMatcher pegomock.InvocationCountMatcher + inOrderContext *pegomock.InOrderContext + timeout time.Duration +} + +func (verifier *VerifierMockPreWorkflowHookRunner) Run(ctx models.PreWorkflowHookCommandContext, command string, path string) *MockPreWorkflowHookRunner_Run_OngoingVerification { + params := []pegomock.Param{ctx, command, path} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "Run", params, verifier.timeout) + return &MockPreWorkflowHookRunner_Run_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +} + +type MockPreWorkflowHookRunner_Run_OngoingVerification struct { + mock *MockPreWorkflowHookRunner + methodInvocations []pegomock.MethodInvocation +} + +func (c *MockPreWorkflowHookRunner_Run_OngoingVerification) GetCapturedArguments() (models.PreWorkflowHookCommandContext, string, string) { + ctx, command, path := c.GetAllCapturedArguments() + return ctx[len(ctx)-1], command[len(command)-1], path[len(path)-1] +} + +func (c *MockPreWorkflowHookRunner_Run_OngoingVerification) GetAllCapturedArguments() (_param0 []models.PreWorkflowHookCommandContext, _param1 []string, _param2 []string) { + params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(params) > 0 { + _param0 = make([]models.PreWorkflowHookCommandContext, len(c.methodInvocations)) + for u, param := range params[0] { + _param0[u] = param.(models.PreWorkflowHookCommandContext) + } + _param1 = make([]string, len(c.methodInvocations)) + for u, param := range params[1] { + _param1[u] = param.(string) + } + _param2 = make([]string, len(c.methodInvocations)) + for u, param := range params[2] { + _param2[u] = param.(string) + } + } + return +} diff --git a/server/events/runtime/pre_workflow_hook_runner.go b/server/events/runtime/pre_workflow_hook_runner.go index fbe6ee0007..e2192fa474 100644 --- a/server/events/runtime/pre_workflow_hook_runner.go +++ b/server/events/runtime/pre_workflow_hook_runner.go @@ -8,9 +8,14 @@ import ( "github.com/runatlantis/atlantis/server/events/models" ) -type PreWorkflowHookRunner struct{} +//go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_pre_workflows_hook_runner.go PreWorkflowHookRunner +type PreWorkflowHookRunner interface { + Run(ctx models.PreWorkflowHookCommandContext, command string, path string) (string, error) +} + +type DefaultPreWorkflowHookRunner struct{} -func (wh *PreWorkflowHookRunner) Run(ctx models.PreWorkflowHookCommandContext, command string, path string) (string, error) { +func (wh DefaultPreWorkflowHookRunner) Run(ctx models.PreWorkflowHookCommandContext, command string, path string) (string, error) { cmd := exec.Command("sh", "-c", command) // #nosec cmd.Dir = path diff --git a/server/events/runtime/pre_workflow_hook_runner_test.go b/server/events/runtime/pre_workflow_hook_runner_test.go index 19f59b9994..dc80ed0444 100644 --- a/server/events/runtime/pre_workflow_hook_runner_test.go +++ b/server/events/runtime/pre_workflow_hook_runner_test.go @@ -70,7 +70,7 @@ func TestPreWorkflowHookRunner_Run(t *testing.T) { logger := logging.NewNoopLogger() - r := runtime.PreWorkflowHookRunner{} + r := runtime.DefaultPreWorkflowHookRunner{} t.Run(c.Command, func(t *testing.T) { tmpDir, cleanup := TempDir(t) defer cleanup() diff --git a/server/events/yaml/valid/global_cfg.go b/server/events/yaml/valid/global_cfg.go index dac71e19b2..520ed58ff2 100644 --- a/server/events/yaml/valid/global_cfg.go +++ b/server/events/yaml/valid/global_cfg.go @@ -94,13 +94,7 @@ var DefaultPlanStage = Stage{ }, } -// NewGlobalCfg returns a global config that respects the parameters. -// allowRepoCfg is true if users want to allow repos full config functionality. -// mergeableReq is true if users want to set the mergeable apply requirement -// for all repos. -// approvedReq is true if users want to set the approved apply requirement -// for all repos. -func NewGlobalCfg(allowRepoCfg bool, mergeableReq bool, approvedReq bool) GlobalCfg { +func NewGlobalCfgWithHooks(allowRepoCfg bool, mergeableReq bool, approvedReq bool, preWorkflowHooks []*PreWorkflowHook) GlobalCfg { defaultWorkflow := Workflow{ Name: DefaultWorkflowName, Apply: DefaultApplyStage, @@ -112,7 +106,6 @@ func NewGlobalCfg(allowRepoCfg bool, mergeableReq bool, approvedReq bool) Global applyReqs := []string{} allowedOverrides := []string{} allowedWorkflows := []string{} - preWorkflowHooks := make([]*PreWorkflowHook, 0) if mergeableReq { applyReqs = append(applyReqs, MergeableApplyReq) } @@ -144,6 +137,19 @@ func NewGlobalCfg(allowRepoCfg bool, mergeableReq bool, approvedReq bool) Global } } +// NewGlobalCfg returns a global config that respects the parameters. +// allowRepoCfg is true if users want to allow repos full config functionality. +// mergeableReq is true if users want to set the mergeable apply requirement +// for all repos. +// approvedReq is true if users want to set the approved apply requirement +// for all repos. + +func NewGlobalCfg(allowRepoCfg bool, mergeableReq bool, approvedReq bool) GlobalCfg { + preWorkflowHooks := make([]*PreWorkflowHook, 0) + + return NewGlobalCfgWithHooks(allowRepoCfg, mergeableReq, approvedReq, preWorkflowHooks) +} + // IDMatches returns true if the repo ID otherID matches this config. func (r Repo) IDMatches(otherID string) bool { if r.ID != "" { diff --git a/server/events_controller.go b/server/events_controller.go index 5dca5efaee..de08599083 100644 --- a/server/events_controller.go +++ b/server/events_controller.go @@ -45,13 +45,12 @@ const bitbucketServerSignatureHeader = "X-Hub-Signature" // EventsController handles all webhook requests which signify 'events' in the // VCS host, ex. GitHub. type EventsController struct { - PreWorkflowHooksCommandRunner events.PreWorkflowHooksCommandRunner - CommandRunner events.CommandRunner - PullCleaner events.PullCleaner - Logger *logging.SimpleLogger - Parser events.EventParsing - CommentParser events.CommentParsing - ApplyDisabled bool + CommandRunner events.CommandRunner + PullCleaner events.PullCleaner + Logger *logging.SimpleLogger + Parser events.EventParsing + CommentParser events.CommentParsing + ApplyDisabled bool // 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. @@ -344,9 +343,6 @@ func (e *EventsController) handlePullRequestEvent(w http.ResponseWriter, baseRep // closed. fmt.Fprintln(w, "Processing...") - e.Logger.Info("running pre workflow hooks if present") - e.PreWorkflowHooksCommandRunner.RunPreHooks(baseRepo, headRepo, pull, user) - e.Logger.Info("executing autoplan") if !e.TestingMode { go e.CommandRunner.RunAutoplanCommand(baseRepo, headRepo, pull, user) diff --git a/server/events_controller_e2e_test.go b/server/events_controller_e2e_test.go index fdadc2ec2f..861e5d272f 100644 --- a/server/events_controller_e2e_test.go +++ b/server/events_controller_e2e_test.go @@ -25,6 +25,8 @@ import ( "github.com/runatlantis/atlantis/server/events/mocks/matchers" "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/events/runtime" + runtimemocks "github.com/runatlantis/atlantis/server/events/runtime/mocks" + runtimematchers "github.com/runatlantis/atlantis/server/events/runtime/mocks/matchers" "github.com/runatlantis/atlantis/server/events/runtime/policy" "github.com/runatlantis/atlantis/server/events/terraform" vcsmocks "github.com/runatlantis/atlantis/server/events/vcs/mocks" @@ -37,6 +39,8 @@ import ( type NoopTFDownloader struct{} +var mockPreWorkflowHookRunner *runtimemocks.MockPreWorkflowHookRunner + func (m *NoopTFDownloader) GetFile(dst, src string, opts ...getter.ClientOption) error { return nil } @@ -368,6 +372,9 @@ func TestGitHubWorkflow(t *testing.T) { ctrl.Post(w, pullClosedReq) responseContains(t, w, 200, "Pull request cleaned successfully") + // Let's verify the pre-workflow hook was called for each comment including the pull request opened event + mockPreWorkflowHookRunner.VerifyWasCalled(Times(len(c.Comments)+1)).Run(runtimematchers.AnyModelsPreWorkflowHookCommandContext(), EqString("some dummy command"), AnyString()) + // Now we're ready to verify Atlantis made all the comments back (or // replies) that we expect. We expect each plan to have 1 comment, // and apply have 1 for each comment plus one for the locks deleted at the @@ -599,7 +606,12 @@ func setupE2E(t *testing.T, repoDir string, policyChecksEnabled bool) (server.Ev defaultTFVersion := terraformClient.DefaultVersion() locker := events.NewDefaultWorkingDirLocker() parser := &yaml.ParserValidator{} - globalCfg := valid.NewGlobalCfg(true, false, false) + globalCfg := valid.NewGlobalCfgWithHooks(true, false, false, []*valid.PreWorkflowHook{ + { + StepName: "global_hook", + RunCommand: "some dummy command", + }, + }) expCfgPath := filepath.Join(absRepoPath(t, repoDir), "repos.yaml") if _, err := os.Stat(expCfgPath); err == nil { globalCfg, err = parser.ParseGlobalCfg(expCfgPath, globalCfg) @@ -609,14 +621,13 @@ func setupE2E(t *testing.T, repoDir string, policyChecksEnabled bool) (server.Ev parallelPoolSize := 1 + mockPreWorkflowHookRunner = runtimemocks.NewMockPreWorkflowHookRunner() preWorkflowHooksCommandRunner := &events.DefaultPreWorkflowHooksCommandRunner{ VCSClient: e2eVCSClient, GlobalCfg: globalCfg, - Logger: logger, WorkingDirLocker: locker, WorkingDir: workingDir, - Drainer: drainer, - PreWorkflowHookRunner: &runtime.PreWorkflowHookRunner{}, + PreWorkflowHookRunner: mockPreWorkflowHookRunner, } projectCommandBuilder := events.NewProjectCommandBuilder( policyChecksEnabled, @@ -749,24 +760,24 @@ func setupE2E(t *testing.T, repoDir string, policyChecksEnabled bool) (server.Ev } commandRunner := &events.DefaultCommandRunner{ - EventParser: eventParser, - VCSClient: e2eVCSClient, - GithubPullGetter: e2eGithubGetter, - GitlabMergeRequestGetter: e2eGitlabGetter, - Logger: logger, - AllowForkPRs: allowForkPRs, - AllowForkPRsFlag: "allow-fork-prs", - CommentCommandRunnerByCmd: commentCommandRunnerByCmd, - Drainer: drainer, + EventParser: eventParser, + VCSClient: e2eVCSClient, + GithubPullGetter: e2eGithubGetter, + GitlabMergeRequestGetter: e2eGitlabGetter, + Logger: logger, + AllowForkPRs: allowForkPRs, + AllowForkPRsFlag: "allow-fork-prs", + CommentCommandRunnerByCmd: commentCommandRunnerByCmd, + Drainer: drainer, + PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, } repoAllowlistChecker, err := events.NewRepoAllowlistChecker("*") Ok(t, err) ctrl := server.EventsController{ - TestingMode: true, - PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, - CommandRunner: commandRunner, + TestingMode: true, + CommandRunner: commandRunner, PullCleaner: &events.PullClosedExecutor{ Locker: lockingClient, VCSClient: e2eVCSClient, diff --git a/server/events_controller_test.go b/server/events_controller_test.go index ebfa917ad7..af24490eb8 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -45,7 +45,7 @@ var secret = []byte("secret") 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() req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) e.Post(w, req) @@ -54,7 +54,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 req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "value") @@ -65,7 +65,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 req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") @@ -76,7 +76,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() req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "value") @@ -87,7 +87,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() req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") @@ -98,7 +98,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() req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "value") @@ -109,7 +109,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() req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") @@ -121,7 +121,7 @@ func TestPost_UnsupportedGitlabEvent(t *testing.T) { // Test that if the comment comes from a commit rather than a merge request, // we give an error and ignore it. func TestPost_GitlabCommentOnCommit(t *testing.T) { - e, _, gl, _, _, _, _, _, _ := setup(t) + e, _, gl, _, _, _, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) w := httptest.NewRecorder() req.Header.Set(gitlabHeader, "value") @@ -132,7 +132,7 @@ func TestPost_GitlabCommentOnCommit(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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "issue_comment") // comment action is deleted, not created @@ -145,7 +145,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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` @@ -158,7 +158,7 @@ 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, _, _, _, _, _, cp := setup(t) + e, _, gl, _, _, _, _, cp := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") When(gl.ParseAndValidate(req, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) @@ -170,7 +170,7 @@ 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, _, _, _, _, cp := setup(t) + e, v, _, p, _, _, _, cp := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` @@ -299,7 +299,7 @@ func TestPost_GithubCommentNotAllowlistedWithSilenceErrors(t *testing.T) { func TestPost_GitlabCommentResponse(t *testing.T) { // When the event is a gitlab comment that warrants a comment response we comment back. - e, _, gl, _, _, _, _, vcsClient, cp := setup(t) + e, _, gl, _, _, _, vcsClient, cp := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") When(gl.ParseAndValidate(req, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) @@ -312,7 +312,7 @@ 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, cp := setup(t) + e, v, _, p, _, _, vcsClient, cp := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` @@ -330,7 +330,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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") When(gl.ParseAndValidate(req, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) @@ -343,7 +343,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, _, _, _, cp := setup(t) + e, v, _, p, cr, _, _, cp := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "issue_comment") event := `{"action": "created"}` @@ -362,7 +362,7 @@ func TestPost_GithubCommentSuccess(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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "pull_request") @@ -376,7 +376,7 @@ func TestPost_GithubPullRequestInvalid(t *testing.T) { func TestPost_GitlabMergeRequestInvalid(t *testing.T) { t.Log("when the event is a gitlab merge request with invalid data we return a 400") - e, _, gl, p, _, _, _, _, _ := setup(t) + e, _, gl, p, _, _, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") When(gl.ParseAndValidate(req, secret)).ThenReturn(gitlab.MergeEvent{}, nil) @@ -390,7 +390,7 @@ func TestPost_GitlabMergeRequestInvalid(t *testing.T) { func TestPost_GithubPullRequestNotAllowlisted(t *testing.T) { t.Log("when the event is a github pull request to a non-allowlisted repo we return a 400") - e, v, _, _, _, _, _, _, _ := setup(t) + e, v, _, _, _, _, _, _ := setup(t) var err error e.RepoAllowlistChecker, err = events.NewRepoAllowlistChecker("github.com/nevermatch") Ok(t, err) @@ -406,7 +406,7 @@ func TestPost_GithubPullRequestNotAllowlisted(t *testing.T) { func TestPost_GitlabMergeRequestNotAllowlisted(t *testing.T) { t.Log("when the event is a gitlab merge request to a non-allowlisted repo we return a 400") - e, _, gl, p, _, _, _, _, _ := setup(t) + e, _, gl, p, _, _, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") @@ -425,7 +425,7 @@ func TestPost_GitlabMergeRequestNotAllowlisted(t *testing.T) { func TestPost_GithubPullRequestUnsupportedAction(t *testing.T) { t.Skip("relies too much on mocks, should use real event parser") - e, v, _, _, _, _, _, _, _ := setup(t) + e, v, _, _, _, _, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "pull_request") @@ -440,7 +440,7 @@ func TestPost_GithubPullRequestUnsupportedAction(t *testing.T) { func TestPost_GitlabMergeRequestUnsupportedAction(t *testing.T) { t.Skip("relies too much on mocks, should use real event parser") t.Log("when the event is a gitlab merge request to a non-allowlisted repo we return a 400") - e, _, gl, p, _, _, _, _, _ := setup(t) + e, _, gl, p, _, _, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") var event gitlab.MergeEvent @@ -537,7 +537,7 @@ func TestPost_GithubPullRequestClosedErrCleaningPull(t *testing.T) { t.Skip("relies too much on mocks, should use real event parser") t.Log("when the event is a closed 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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "pull_request") @@ -555,7 +555,7 @@ func TestPost_GithubPullRequestClosedErrCleaningPull(t *testing.T) { func TestPost_GitlabMergeRequestClosedErrCleaningPull(t *testing.T) { t.Skip("relies too much on mocks, should use real event parser") t.Log("when the event is a closed gitlab merge request and an error occurs calling CleanUpPull we return a 500") - e, _, gl, p, _, _, c, _, _ := setup(t) + e, _, gl, p, _, c, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") var event gitlab.MergeEvent @@ -573,7 +573,7 @@ func TestPost_GitlabMergeRequestClosedErrCleaningPull(t *testing.T) { func TestPost_GithubClosedPullRequestSuccess(t *testing.T) { t.Skip("relies too much on mocks, should use real event parser") 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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(githubHeader, "pull_request") @@ -591,7 +591,7 @@ func TestPost_GithubClosedPullRequestSuccess(t *testing.T) { func TestPost_GitlabMergeRequestSuccess(t *testing.T) { t.Skip("relies too much on mocks, should use real event parser") 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) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req.Header.Set(gitlabHeader, "value") When(gl.ParseAndValidate(req, secret)).ThenReturn(gitlab.MergeEvent{}, nil) @@ -710,7 +710,7 @@ func TestPost_PullOpenedOrUpdated(t *testing.T) { for _, c := range cases { t.Run(c.Description, func(t *testing.T) { - e, v, gl, p, cr, wh, _, _, _ := setup(t) + e, v, gl, p, cr, _, _, _ := setup(t) req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) var pullRequest models.PullRequest var repo models.Repo @@ -736,39 +736,36 @@ func TestPost_PullOpenedOrUpdated(t *testing.T) { w := httptest.NewRecorder() e.Post(w, req) responseContains(t, w, http.StatusOK, "Processing...") - wh.VerifyWasCalledOnce().RunPreHooks(models.Repo{}, models.Repo{}, models.PullRequest{State: models.ClosedPullState}, models.User{}) cr.VerifyWasCalledOnce().RunAutoplanCommand(models.Repo{}, models.Repo{}, models.PullRequest{State: models.ClosedPullState}, models.User{}) }) } } -func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParserValidator, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPreWorkflowHooksCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClient, *emocks.MockCommentParsing) { +func setup(t *testing.T) (server.EventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParserValidator, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClient, *emocks.MockCommentParsing) { RegisterMockTestingT(t) v := mocks.NewMockGithubRequestValidator() gl := mocks.NewMockGitlabRequestParserValidator() p := emocks.NewMockEventParsing() cp := emocks.NewMockCommentParsing() - wh := emocks.NewMockPreWorkflowHooksCommandRunner() cr := emocks.NewMockCommandRunner() c := emocks.NewMockPullCleaner() vcsmock := vcsmocks.NewMockClient() repoAllowlistChecker, err := events.NewRepoAllowlistChecker("*") Ok(t, err) e := server.EventsController{ - TestingMode: true, - Logger: logging.NewNoopLogger(), - GithubRequestValidator: v, - Parser: p, - CommentParser: cp, - CommandRunner: cr, - PreWorkflowHooksCommandRunner: wh, - PullCleaner: c, - GithubWebhookSecret: secret, - SupportedVCSHosts: []models.VCSHostType{models.Github, models.Gitlab}, - GitlabWebhookSecret: secret, - GitlabRequestParserValidator: gl, - RepoAllowlistChecker: repoAllowlistChecker, - VCSClient: vcsmock, + TestingMode: true, + Logger: logging.NewNoopLogger(), + GithubRequestValidator: v, + Parser: p, + CommentParser: cp, + CommandRunner: cr, + PullCleaner: c, + GithubWebhookSecret: secret, + SupportedVCSHosts: []models.VCSHostType{models.Github, models.Gitlab}, + GitlabWebhookSecret: secret, + GitlabRequestParserValidator: gl, + RepoAllowlistChecker: repoAllowlistChecker, + VCSClient: vcsmock, } - return e, v, gl, p, cr, wh, c, vcsmock, cp + return e, v, gl, p, cr, c, vcsmock, cp } diff --git a/server/server.go b/server/server.go index 153ce6bfc9..dfcabfec4f 100644 --- a/server/server.go +++ b/server/server.go @@ -397,11 +397,9 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { preWorkflowHooksCommandRunner := &events.DefaultPreWorkflowHooksCommandRunner{ VCSClient: vcsClient, GlobalCfg: globalCfg, - Logger: logger, WorkingDirLocker: workingDirLocker, WorkingDir: workingDir, - Drainer: drainer, - PreWorkflowHookRunner: &runtime.PreWorkflowHookRunner{}, + PreWorkflowHookRunner: runtime.DefaultPreWorkflowHookRunner{}, } projectCommandBuilder := events.NewProjectCommandBuilder( policyChecksEnabled, @@ -534,19 +532,20 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { } commandRunner := &events.DefaultCommandRunner{ - VCSClient: vcsClient, - GithubPullGetter: githubClient, - GitlabMergeRequestGetter: gitlabClient, - AzureDevopsPullGetter: azuredevopsClient, - CommentCommandRunnerByCmd: commentCommandRunnerByCmd, - EventParser: eventParser, - Logger: logger, - AllowForkPRs: userConfig.AllowForkPRs, - AllowForkPRsFlag: config.AllowForkPRsFlag, - SilenceForkPRErrors: userConfig.SilenceForkPRErrors, - SilenceForkPRErrorsFlag: config.SilenceForkPRErrorsFlag, - DisableAutoplan: userConfig.DisableAutoplan, - Drainer: drainer, + VCSClient: vcsClient, + GithubPullGetter: githubClient, + GitlabMergeRequestGetter: gitlabClient, + AzureDevopsPullGetter: azuredevopsClient, + CommentCommandRunnerByCmd: commentCommandRunnerByCmd, + EventParser: eventParser, + Logger: logger, + AllowForkPRs: userConfig.AllowForkPRs, + AllowForkPRsFlag: config.AllowForkPRsFlag, + SilenceForkPRErrors: userConfig.SilenceForkPRErrors, + SilenceForkPRErrorsFlag: config.SilenceForkPRErrorsFlag, + DisableAutoplan: userConfig.DisableAutoplan, + Drainer: drainer, + PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, } repoAllowlist, err := events.NewRepoAllowlistChecker(userConfig.RepoAllowlist) if err != nil { @@ -565,7 +564,6 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { DeleteLockCommand: deleteLockCommand, } eventsController := &EventsController{ - PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, CommandRunner: commandRunner, PullCleaner: pullClosedExecutor, Parser: eventParser,