Fixes for pre-workflow-hook

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
 		}
 	}
 

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,73 @@ var wh events.DefaultPreWorkflowHooksCommandRunner
 var whWorkingDir *mocks.MockWorkingDir
 var whWorkingDirLocker *mocks.MockWorkingDirLocker
 var whDrainer *events.Drainer
+var whLogger *logmocks.MockSimpleLogging
 
 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 +103,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 +121,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)
 }