From 114129a738da619286912927b2d26cd5b9dbded6 Mon Sep 17 00:00:00 2001 From: Sarvar Muminov Date: Fri, 12 Feb 2021 23:14:56 -0800 Subject: [PATCH 1/2] Fixes pre-workflow-hooks not logging errors Prevents pre-workflow-hook from locking and cloning the dir if there are no hooks registered --- .../pre_workflow_hooks_command_runner.go | 20 ++++--- .../pre_workflow_hooks_command_runner_test.go | 57 ++++++++++++++++--- 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/server/events/pre_workflow_hooks_command_runner.go b/server/events/pre_workflow_hooks_command_runner.go index 739359b408..c37c0c88b6 100644 --- a/server/events/pre_workflow_hooks_command_runner.go +++ b/server/events/pre_workflow_hooks_command_runner.go @@ -53,6 +53,17 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks( log.Info("running pre hooks") + 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...) + } + } + + if len(preWorkflowHooks) == 0 { + return + } + unlockFn, err := w.WorkingDirLocker.TryLock(baseRepo.FullName, pull.Num, DefaultWorkspace) if err != nil { log.Warn("workspace is locked") @@ -67,13 +78,6 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks( return } - 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, @@ -100,7 +104,7 @@ func (w *DefaultPreWorkflowHooksCommandRunner) runHooks( _, err := w.PreWorkflowHookRunner.Run(ctx, hook.RunCommand, repoDir) if err != nil { - return nil + return err } } diff --git a/server/events/pre_workflow_hooks_command_runner_test.go b/server/events/pre_workflow_hooks_command_runner_test.go index e8290359c9..4aef0c16dd 100644 --- a/server/events/pre_workflow_hooks_command_runner_test.go +++ b/server/events/pre_workflow_hooks_command_runner_test.go @@ -12,6 +12,7 @@ import ( "github.com/runatlantis/atlantis/server/events/models/fixtures" "github.com/runatlantis/atlantis/server/events/runtime" 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" @@ -21,33 +22,75 @@ var wh events.DefaultPreWorkflowHooksCommandRunner var whWorkingDir *mocks.MockWorkingDir var whWorkingDirLocker *mocks.MockWorkingDirLocker var whDrainer *events.Drainer +var whGlobalCfg valid.GlobalCfg +var whLogger *logmocks.MockSimpleLogging +var whPullLogger *logging.SimpleLogger func preWorkflowHooksSetup(t *testing.T) *vcsmocks.MockClient { RegisterMockTestingT(t) vcsClient := vcsmocks.NewMockClient() - logger := logmocks.NewMockSimpleLogging() + whLogger = logmocks.NewMockSimpleLogging() whWorkingDir = mocks.NewMockWorkingDir() whWorkingDirLocker = mocks.NewMockWorkingDirLocker() whDrainer = &events.Drainer{} + pullLogger = logging.NewSimpleLogger("runatlantis/atlantis#1", true, logging.Info) + When(whLogger.GetLevel()).ThenReturn(logging.Info) + When(whLogger.NewLogger("runatlantis/atlantis#1", true, logging.Info)). + ThenReturn(pullLogger) wh = events.DefaultPreWorkflowHooksCommandRunner{ VCSClient: vcsClient, - Logger: logger, + Logger: whLogger, WorkingDirLocker: whWorkingDirLocker, WorkingDir: whWorkingDir, Drainer: whDrainer, PreWorkflowHookRunner: &runtime.PreWorkflowHookRunner{}, + GlobalCfg: valid.GlobalCfg{ + Repos: []valid.Repo{ + valid.Repo{ + ID: "github.com/runatlantis/atlantis", + PreWorkflowHooks: []*valid.PreWorkflowHook{ + &valid.PreWorkflowHook{ + StepName: "run", + RunCommand: `echo "exit with error"; exit 1`, + }, + }, + }, + }, + }, } return vcsClient } +func TestPreWorkflowHooksCommandLogErrors(t *testing.T) { + preWorkflowHooksSetup(t) + + wh.RunPreHooks(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) + Assert(t, strings.Contains(pullLogger.History.String(), "[EROR] Pre workflow hook run error results: exit status 1:"), "Failing pre workflow should log error") +} + +func TestPreWorkflowHooksDoesntRun(t *testing.T) { + preWorkflowHooksSetup(t) + wh.GlobalCfg = valid.GlobalCfg{ + Repos: []valid.Repo{ + valid.Repo{ + ID: "github.com/runatlantis/atlantis", + PreWorkflowHooks: []*valid.PreWorkflowHook{}, + }, + }, + } + + wh.RunPreHooks(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) + whWorkingDirLocker.VerifyWasCalled(Times(0)).TryLock(AnyString(), AnyInt(), AnyString()) + whWorkingDir.VerifyWasCalled(Times(0)).Clone(matchers.AnyPtrToLoggingSimpleLogger(), matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), AnyString()) +} + 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, + pullLogger, fixtures.GithubRepo, fixtures.Pull, events.DefaultWorkspace, @@ -62,11 +105,9 @@ func TestPreWorkflowHooksCommand_LogPanics(t *testing.T) { // 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) + whWorkingDir.VerifyWasCalledOnce().Clone(pullLogger, fixtures.GithubRepo, fixtures.Pull, events.DefaultWorkspace) } func TestRunPreHooks_DrainOngoing(t *testing.T) { @@ -82,6 +123,6 @@ func TestRunPreHooks_DrainNotOngoing(t *testing.T) { 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) + whWorkingDir.VerifyWasCalledOnce().Clone(pullLogger, fixtures.GithubRepo, fixtures.Pull, events.DefaultWorkspace) Equals(t, 0, whDrainer.GetStatus().InProgressOps) } From 2d8678ea27e91b6f38091e239479b67b19043caa Mon Sep 17 00:00:00 2001 From: Sarvar Muminov Date: Fri, 12 Feb 2021 23:38:17 -0800 Subject: [PATCH 2/2] Cleanup tests --- server/events/pre_workflow_hooks_command_runner_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/events/pre_workflow_hooks_command_runner_test.go b/server/events/pre_workflow_hooks_command_runner_test.go index 4aef0c16dd..ce3afb0dbb 100644 --- a/server/events/pre_workflow_hooks_command_runner_test.go +++ b/server/events/pre_workflow_hooks_command_runner_test.go @@ -22,9 +22,7 @@ var wh events.DefaultPreWorkflowHooksCommandRunner var whWorkingDir *mocks.MockWorkingDir var whWorkingDirLocker *mocks.MockWorkingDirLocker var whDrainer *events.Drainer -var whGlobalCfg valid.GlobalCfg var whLogger *logmocks.MockSimpleLogging -var whPullLogger *logging.SimpleLogger func preWorkflowHooksSetup(t *testing.T) *vcsmocks.MockClient { RegisterMockTestingT(t)