Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ORCA-4531] Improve pre workflow hook error handling #228

Merged
merged 11 commits into from
Apr 1, 2022
Merged

[ORCA-4531] Improve pre workflow hook error handling #228

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a74702a
add instrumentation to pre workflow hooks
msarvar Mar 30, 2022
2a72796
add error handling and failure
msarvar Mar 30, 2022
8a05a3c
use interface instead of struct
msarvar Mar 30, 2022
c3c1b79
minor fix
msarvar Mar 30, 2022
47dc75c
add error messaging to pre workflow hooks
msarvar Mar 31, 2022
3cd4004
use underscores
msarvar Mar 31, 2022
09714de
fail pr status check instead of commenting
msarvar Mar 31, 2022
bd63649
pass in context and use new logger
msarvar Mar 31, 2022
268b563
fix test
msarvar Mar 31, 2022
e43085c
address feedback
msarvar Apr 1, 2022
d522a3a
rename logging/helpers to logging/fields
msarvar Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions server/events/command/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ type Context struct {
Pull models.PullRequest
Scope tally.Scope
// User is the user that triggered this command.
User models.User
Log logging.SimpleLogging
User models.User
Log logging.SimpleLogging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this? Why have both? We're gonna need to do this for the whole codebase at some point anyways, might as well do it per file as we come across it.

CtxLog logging.Logger

// Current PR state
PullRequestStatus models.PullReqStatus
Expand Down
24 changes: 17 additions & 7 deletions server/events/command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package events

import (
"context"
"fmt"
"strconv"
"time"
Expand Down Expand Up @@ -129,8 +130,10 @@ type DefaultCommandRunner struct {
CommentCommandRunnerByCmd map[command.Name]command.Runner
Drainer *Drainer
PreWorkflowHooksCommandRunner PreWorkflowHooksCommandRunner
CommitStatusUpdater CommitStatusUpdater
PullStatusFetcher PullStatusFetcher
StaleCommandChecker StaleCommandChecker
Logger logging.Logger
}

// RunAutoplanCommand runs plan and policy_checks when a pull request is opened or updated.
Expand Down Expand Up @@ -158,6 +161,7 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(logger logging.SimpleLogging,
ctx := &command.Context{
User: user,
Log: log,
CtxLog: c.Logger,
Scope: scope,
Pull: pull,
HeadRepo: headRepo,
Expand All @@ -176,10 +180,12 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(logger logging.SimpleLogging,
return
}

err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx)

if err != nil {
ctx.Log.Errorf("Error running pre-workflow hooks %s. Proceeding with %s command.", err, command.Plan)
if err := c.PreWorkflowHooksCommandRunner.RunPreHooks(context.TODO(), ctx); err != nil {
c.Logger.ErrorContext(context.TODO(), "Error running pre-workflow hooks", map[string]interface{}{
"err": err,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add repository and pull num here? similar to here:
https://github.com/lyft/atlantis/blob/release-v0.17.3-lyft.1/server/events/vcs/instrumented_client.go#L346-L348

Eventually this can be added to ctx and ripped out from there automatically but for now we need this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my thinking was that it will be covered by the context, but you're right in the meantime we need a workaround.

})
c.CommitStatusUpdater.UpdateCombined(context.TODO(), ctx.HeadRepo, ctx.Pull, models.FailedCommitStatus, command.Plan)
return
}

autoPlanRunner := buildCommentCommandRunner(c, command.Plan)
Expand Down Expand Up @@ -226,6 +232,7 @@ func (c *DefaultCommandRunner) RunCommentCommand(logger logging.SimpleLogging, b
ctx := &command.Context{
User: user,
Log: log,
CtxLog: c.Logger,
Pull: pull,
PullStatus: status,
HeadRepo: headRepo,
Expand All @@ -243,10 +250,13 @@ func (c *DefaultCommandRunner) RunCommentCommand(logger logging.SimpleLogging, b
return
}

err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx)
if err := c.PreWorkflowHooksCommandRunner.RunPreHooks(context.TODO(), ctx); err != nil {
c.Logger.ErrorContext(context.TODO(), "Error running pre-workflow hooks", map[string]interface{}{
"err": err,
})

if err != nil {
ctx.Log.Errorf("Error running pre-workflow hooks %s. Proceeding with %s command.", err, cmd.Name.String())
c.CommitStatusUpdater.UpdateCombined(context.TODO(), ctx.HeadRepo, ctx.Pull, models.FailedCommitStatus, cmd.Name)
return
}

cmdRunner := buildCommentCommandRunner(c, cmd.CommandName())
Expand Down
52 changes: 51 additions & 1 deletion server/events/command_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func setup(t *testing.T) *vcsmocks.MockClient {

preWorkflowHooksCommandRunner = mocks.NewMockPreWorkflowHooksCommandRunner()

When(preWorkflowHooksCommandRunner.RunPreHooks(matchers.AnyPtrToEventsCommandContext())).ThenReturn(nil)
When(preWorkflowHooksCommandRunner.RunPreHooks(matchers.AnyContextContext(), matchers.AnyPtrToEventsCommandContext())).ThenReturn(nil)

globalCfg := valid.NewGlobalCfgFromArgs(valid.GlobalCfgArgs{})
scope, _, _ := metrics.NewLoggingScope(logger, "atlantis")
Expand All @@ -199,6 +199,7 @@ func setup(t *testing.T) *vcsmocks.MockClient {
ch = events.DefaultCommandRunner{
VCSClient: vcsClient,
CommentCommandRunnerByCmd: commentCommandRunnerByCmd,
Logger: logging.NewNoopCtxLogger(t),
EventParser: eventParsing,
GithubPullGetter: githubGetter,
GitlabMergeRequestGetter: gitlabGetter,
Expand All @@ -211,6 +212,7 @@ func setup(t *testing.T) *vcsmocks.MockClient {
PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner,
PullStatusFetcher: defaultBoltDB,
StaleCommandChecker: staleCommandChecker,
CommitStatusUpdater: commitUpdater,
}
return vcsClient
}
Expand Down Expand Up @@ -299,6 +301,32 @@ func TestRunCommentCommand_ForkPRDisabled_SilenceEnabled(t *testing.T) {
vcsClient.VerifyWasCalled(Never()).CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), AnyString())
}

func TestRunCommentCommand_PreWorkflowHookError(t *testing.T) {
t.Log("if pre workflow hook errors out stop the execution")
for _, cmd := range []command.Name{command.Plan, command.Apply} {
t.Run(cmd.String(), func(t *testing.T) {
RegisterMockTestingT(t)
_ = setup(t)
log := logging.NewNoopLogger(t)
var pull github.PullRequest
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, Num: fixtures.Pull.Num, State: models.OpenPullState}
preWorkflowHooksCommandRunner = mocks.NewMockPreWorkflowHooksCommandRunner()

When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)
When(eventParsing.ParseGithubPull(&pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)
When(staleCommandChecker.CommandIsStale(matchers.AnyPtrToModelsCommandContext())).ThenReturn(false)
When(preWorkflowHooksCommandRunner.RunPreHooks(matchers.AnyContextContext(), matchers.AnyPtrToEventsCommandContext())).ThenReturn(fmt.Errorf("catastrophic error"))

ch.PreWorkflowHooksCommandRunner = preWorkflowHooksCommandRunner

ch.RunCommentCommand(log, fixtures.GithubRepo, nil, nil, fixtures.User, fixtures.Pull.Num, &command.Comment{Name: cmd}, time.Now())
_, _, _, status, cmdName := commitUpdater.VerifyWasCalledOnce().UpdateCombined(matchers.AnyContextContext(), matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), matchers.AnyModelsCommitStatus(), matchers.AnyCommandName()).GetCapturedArguments()
Equals(t, models.FailedCommitStatus, status)
Equals(t, cmd, cmdName)
})
}
}

func TestRunCommentCommandPlan_NoProjects_SilenceEnabled(t *testing.T) {
t.Log("if a plan command is run on a pull request and SilenceNoProjects is enabled and we are silencing all comments if the modified files don't have a matching project")
vcsClient := setup(t)
Expand Down Expand Up @@ -534,6 +562,28 @@ func TestRunUnlockCommandFail_VCSComment(t *testing.T) {
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, fixtures.Pull.Num, "Failed to delete PR locks", "unlock")
}

func TestRunAutoplanCommand_PreWorkflowHookError(t *testing.T) {
t.Log("if pre workflow hook errors out stop the execution")
setup(t)
log := logging.NewNoopLogger(t)
var pull github.PullRequest
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, Num: fixtures.Pull.Num, State: models.OpenPullState}
preWorkflowHooksCommandRunner = mocks.NewMockPreWorkflowHooksCommandRunner()

When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)
When(eventParsing.ParseGithubPull(&pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)
When(staleCommandChecker.CommandIsStale(matchers.AnyPtrToModelsCommandContext())).ThenReturn(false)
When(preWorkflowHooksCommandRunner.RunPreHooks(matchers.AnyContextContext(), matchers.AnyPtrToEventsCommandContext())).ThenReturn(fmt.Errorf("catastrophic error"))
When(projectCommandRunner.Plan(matchers.AnyCommandProjectContext())).ThenReturn(command.ProjectResult{PlanSuccess: &models.PlanSuccess{}})

ch.PreWorkflowHooksCommandRunner = preWorkflowHooksCommandRunner

ch.RunAutoplanCommand(log, fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User, time.Now())
_, _, _, status, cmdName := commitUpdater.VerifyWasCalledOnce().UpdateCombined(matchers.AnyContextContext(), matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest(), matchers.AnyModelsCommitStatus(), matchers.AnyCommandName()).GetCapturedArguments()
Equals(t, models.FailedCommitStatus, status)
Equals(t, command.Plan, cmdName)
}

// Test that if one plan fails and we are using automerge, that
// we delete the plans.
func TestRunAutoplanCommand_DeletePlans(t *testing.T) {
Expand Down
38 changes: 21 additions & 17 deletions server/events/mocks/mock_pre_workflows_hooks_command_runner.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 15 additions & 11 deletions server/events/pre_workflow_hooks_command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package events

import (
"context"

"github.com/pkg/errors"
"github.com/runatlantis/atlantis/server/core/config/valid"
"github.com/runatlantis/atlantis/server/core/runtime"
"github.com/runatlantis/atlantis/server/events/command"
Expand All @@ -11,7 +14,7 @@ import (
//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(ctx *command.Context) error
RunPreHooks(ctx context.Context, cmdCtx *command.Context) error
}

// DefaultPreWorkflowHooksCommandRunner is the first step when processing a workflow hook commands.
Expand All @@ -25,13 +28,14 @@ type DefaultPreWorkflowHooksCommandRunner struct {

// RunPreHooks runs pre_workflow_hooks when PR is opened or updated.
func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(
ctx *command.Context,
ctx context.Context,
cmdCtx *command.Context,
) error {
pull := ctx.Pull
pull := cmdCtx.Pull
baseRepo := pull.BaseRepo
headRepo := ctx.HeadRepo
user := ctx.User
log := ctx.Log
headRepo := cmdCtx.HeadRepo
user := cmdCtx.User
log := cmdCtx.Log

preWorkflowHooks := make([]*valid.PreWorkflowHook, 0)
for _, repo := range w.GlobalCfg.Repos {
Expand All @@ -49,14 +53,14 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(

unlockFn, err := w.WorkingDirLocker.TryLock(baseRepo.FullName, pull.Num, DefaultWorkspace)
if err != nil {
return err
return errors.Wrap(err, "locking working dir")
}
log.Debugf("got workspace lock")
defer unlockFn()

repoDir, _, err := w.WorkingDir.Clone(log, headRepo, pull, DefaultWorkspace)
if err != nil {
return err
return errors.Wrap(err, "cloning repository")
}

err = w.runHooks(
Expand All @@ -71,20 +75,20 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(
preWorkflowHooks, repoDir)

if err != nil {
return err
return errors.Wrap(err, "running pre workflow hooks")
}

return nil
}

func (w *DefaultPreWorkflowHooksCommandRunner) runHooks(
ctx models.PreWorkflowHookCommandContext,
cmdCtx models.PreWorkflowHookCommandContext,
preWorkflowHooks []*valid.PreWorkflowHook,
repoDir string,
) error {

for _, hook := range preWorkflowHooks {
_, err := w.PreWorkflowHookRunner.Run(ctx, hook.RunCommand, repoDir)
_, err := w.PreWorkflowHookRunner.Run(cmdCtx, hook.RunCommand, repoDir)

if err != nil {
return err
Expand Down
Loading