From c36b0cf9a82e1852d914b64f9e15a212bdd311fe Mon Sep 17 00:00:00 2001 From: X-Guardian Date: Fri, 17 Jan 2025 15:36:06 +0000 Subject: [PATCH 1/4] Pre Workflow Hook Status Check Fix Signed-off-by: X-Guardian --- server/controllers/api_controller.go | 11 +++++++++ server/events/command_runner.go | 24 +++++++++++++++++++ server/events/plan_command_runner.go | 9 ------- .../pre_workflow_hooks_command_runner.go | 12 ---------- 4 files changed, 35 insertions(+), 21 deletions(-) diff --git a/server/controllers/api_controller.go b/server/controllers/api_controller.go index ff85371469..29037ec9a0 100644 --- a/server/controllers/api_controller.go +++ b/server/controllers/api_controller.go @@ -33,6 +33,7 @@ type APIController struct { RepoAllowlistChecker *events.RepoAllowlistChecker Scope tally.Scope VCSClient vcs.Client + CommitStatusUpdater events.CommitStatusUpdater } type APIRequest struct { @@ -150,6 +151,11 @@ func (a *APIController) apiPlan(request *APIRequest, ctx *command.Context) (*com return nil, err } + // Update the combined plan commit status to pending + if err := a.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { + ctx.Log.Warn("unable to update plan commit status: %s", err) + } + var projectResults []command.ProjectResult for i, cmd := range cmds { err = a.PreWorkflowHooksCommandRunner.RunPreHooks(ctx, cc[i]) @@ -173,6 +179,11 @@ func (a *APIController) apiApply(request *APIRequest, ctx *command.Context) (*co return nil, err } + // Update the combined apply commit status to pending + if err := a.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Apply); err != nil { + ctx.Log.Warn("unable to update apply commit status: %s", err) + } + var projectResults []command.ProjectResult for i, cmd := range cmds { err = a.PreWorkflowHooksCommandRunner.RunPreHooks(ctx, cc[i]) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index fd544baabf..5a5d99e316 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -202,6 +202,12 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo cmd := &CommentCommand{ Name: command.Autoplan, } + + // Update the combined plan commit status to pending + if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { + ctx.Log.Warn("unable to update plan commit status: %s", err) + } + err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx, cmd) if err != nil { @@ -354,6 +360,24 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead return } + // Update the combined plan or apply commit status to pending + switch cmd.Name { + case command.Plan: + // if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { + // ctx.Log.Warn("unable to update plan commit status: %s", err) + // } + case command.Apply: + // if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Apply); err != nil { + // ctx.Log.Warn("unable to update apply commit status: %s", err) + // } + log.Info("%v", ctx.Log) + log.Info("%v", ctx.Pull.BaseRepo) + log.Info("%v", ctx.Pull) + log.Info("%v", models.FailedCommitStatus) + log.Info("%v", command.Apply) + c.CommitStatusUpdater.UpdateCombinedCount(ctx.Log, baseRepo, pull, models.SuccessCommitStatus, command.Apply, 0, 0) +} + err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx, cmd) if err != nil { diff --git a/server/events/plan_command_runner.go b/server/events/plan_command_runner.go index c1cc3e81e0..a9652b0cab 100644 --- a/server/events/plan_command_runner.go +++ b/server/events/plan_command_runner.go @@ -114,11 +114,6 @@ func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) { return } - // At this point we are sure Atlantis has work to do, so set commit status to pending - if err := p.commitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { - ctx.Log.Warn("unable to update plan commit status: %s", err) - } - // discard previous plans that might not be relevant anymore ctx.Log.Debug("deleting previous plans and locks") p.deletePlans(ctx) @@ -188,10 +183,6 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) { } } - if err = p.commitStatusUpdater.UpdateCombined(ctx.Log, baseRepo, pull, models.PendingCommitStatus, command.Plan); err != nil { - ctx.Log.Warn("unable to update commit status: %s", err) - } - projectCmds, err := p.prjCmdBuilder.BuildPlanCommands(ctx, cmd) if err != nil { if statusErr := p.commitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.FailedCommitStatus, command.Plan); statusErr != nil { diff --git a/server/events/pre_workflow_hooks_command_runner.go b/server/events/pre_workflow_hooks_command_runner.go index 7d152c7328..be175d0b7d 100644 --- a/server/events/pre_workflow_hooks_command_runner.go +++ b/server/events/pre_workflow_hooks_command_runner.go @@ -69,18 +69,6 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(ctx *command.Context, escapedArgs = escapeArgs(cmd.Flags) } - // Update the plan or apply commit status to pending whilst the pre workflow hook is running - switch cmd.Name { - case command.Plan: - if err := w.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { - ctx.Log.Warn("unable to update plan commit status: %s", err) - } - case command.Apply: - if err := w.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Apply); err != nil { - ctx.Log.Warn("unable to update apply commit status: %s", err) - } - } - err = w.runHooks( models.WorkflowHookCommandContext{ BaseRepo: ctx.Pull.BaseRepo, From 7fd7a90bb0fb697bc5526321f93d7e81c8c990d1 Mon Sep 17 00:00:00 2001 From: X-Guardian <32168619+X-Guardian@users.noreply.github.com> Date: Sun, 26 Jan 2025 17:52:15 +0000 Subject: [PATCH 2/4] Fix command runner and update tests Signed-off-by: X-Guardian <32168619+X-Guardian@users.noreply.github.com> --- server/controllers/api_controller_test.go | 5 ++++ server/events/apply_command_runner.go | 4 --- server/events/command_runner.go | 20 +++++---------- server/events/command_runner_test.go | 31 +++++++++-------------- 4 files changed, 24 insertions(+), 36 deletions(-) diff --git a/server/controllers/api_controller_test.go b/server/controllers/api_controller_test.go index 3b3aa520aa..778c41bee2 100644 --- a/server/controllers/api_controller_test.go +++ b/server/controllers/api_controller_test.go @@ -94,6 +94,10 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, When(postWorkflowHooksCommandRunner.RunPostHooks(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn(nil) + commitStatusUpdater := NewMockCommitStatusUpdater() + + When(commitStatusUpdater.UpdateCombined(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[models.CommitStatus](), Any[command.Name]())).ThenReturn(nil) + ac := controllers.APIController{ APISecret: []byte(atlantisToken), Locker: locker, @@ -107,6 +111,7 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, VCSClient: vcsClient, RepoAllowlistChecker: repoAllowlistChecker, + CommitStatusUpdater: commitStatusUpdater, } return ac, projectCommandBuilder, projectCommandRunner } diff --git a/server/events/apply_command_runner.go b/server/events/apply_command_runner.go index 6c69032910..c68110e518 100644 --- a/server/events/apply_command_runner.go +++ b/server/events/apply_command_runner.go @@ -94,10 +94,6 @@ func (a *ApplyCommandRunner) Run(ctx *command.Context, cmd *CommentCommand) { return } - if err = a.commitStatusUpdater.UpdateCombined(ctx.Log, baseRepo, pull, models.PendingCommitStatus, cmd.CommandName()); err != nil { - ctx.Log.Warn("unable to update commit status: %s", err) - } - // Get the mergeable status before we set any build statuses of our own. // We do this here because when we set a "Pending" status, if users have // required the Atlantis status checks to pass, then we've now changed diff --git a/server/events/command_runner.go b/server/events/command_runner.go index b5f578d970..645cc10acf 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -360,22 +360,16 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead return } - // Update the combined plan or apply commit status to pending +// Update the combined plan or apply commit status to pending switch cmd.Name { case command.Plan: - // if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { - // ctx.Log.Warn("unable to update plan commit status: %s", err) - // } + if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { + ctx.Log.Warn("unable to update plan commit status: %s", err) + } case command.Apply: - // if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Apply); err != nil { - // ctx.Log.Warn("unable to update apply commit status: %s", err) - // } - log.Info("%v", ctx.Log) - log.Info("%v", ctx.Pull.BaseRepo) - log.Info("%v", ctx.Pull) - log.Info("%v", models.FailedCommitStatus) - log.Info("%v", command.Apply) - c.CommitStatusUpdater.UpdateCombinedCount(ctx.Log, baseRepo, pull, models.SuccessCommitStatus, command.Apply, 0, 0) + if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Apply); err != nil { + ctx.Log.Warn("unable to update apply commit status: %s", err) + } } err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx, cmd) diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index b3b88c40d0..4e32994824 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -253,6 +253,7 @@ func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.Mock PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, PullStatusFetcher: testConfig.backend, + CommitStatusUpdater: commitUpdater, } return vcsClient @@ -440,15 +441,8 @@ func TestRunCommentCommandApply_NoProjects_SilenceEnabled(t *testing.T) { ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Apply}) vcsClient.VerifyWasCalled(Never()).CreateComment( Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]()) - commitUpdater.VerifyWasCalledOnce().UpdateCombinedCount( - Any[logging.SimpleLogging](), - Any[models.Repo](), - Any[models.PullRequest](), - Eq[models.CommitStatus](models.SuccessCommitStatus), - Eq[command.Name](command.Apply), - Eq(0), - Eq(0), - ) + commitUpdater.VerifyWasCalledOnce().UpdateCombined( + Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq(models.PendingCommitStatus), Eq(command.Apply)) } func TestRunCommentCommandApprovePolicy_NoProjects_SilenceEnabled(t *testing.T) { @@ -463,15 +457,6 @@ func TestRunCommentCommandApprovePolicy_NoProjects_SilenceEnabled(t *testing.T) ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.ApprovePolicies}) vcsClient.VerifyWasCalled(Never()).CreateComment( Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]()) - commitUpdater.VerifyWasCalledOnce().UpdateCombinedCount( - Any[logging.SimpleLogging](), - Any[models.Repo](), - Any[models.PullRequest](), - Eq[models.CommitStatus](models.SuccessCommitStatus), - Eq[command.Name](command.PolicyCheck), - Eq(0), - Eq(0), - ) } func TestRunCommentCommandUnlock_NoProjects_SilenceEnabled(t *testing.T) { @@ -485,6 +470,8 @@ func TestRunCommentCommandUnlock_NoProjects_SilenceEnabled(t *testing.T) { ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Unlock}) vcsClient.VerifyWasCalled(Never()).CreateComment(Any[logging.SimpleLogging](), Any[models.Repo](), Any[int](), Any[string](), Any[string]()) + commitUpdater.VerifyWasCalled(Never()).UpdateCombined( + Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Eq(models.PendingCommitStatus), Any[command.Name]()) } func TestRunCommentCommandImport_NoProjects_SilenceEnabled(t *testing.T) { @@ -535,7 +522,7 @@ func TestRunCommentCommand_DisableAutoplan(t *testing.T) { CommandName: command.Plan, }, }, nil) - + When(commitUpdater.UpdateCombinedCount(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[models.CommitStatus](), Any[command.Name](), Any[int](), Any[int]())).ThenReturn(nil) ch.RunAutoplanCommand(testdata.GithubRepo, testdata.GithubRepo, modelPull, testdata.User) projectCommandBuilder.VerifyWasCalled(Never()).BuildAutoplanCommands(Any[*command.Context]()) } @@ -831,6 +818,10 @@ func TestRunAutoplanCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_Fal ch.RunAutoplanCommand(testdata.GithubRepo, testdata.GithubRepo, testdata.Pull, testdata.User) pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp) lockingLocker.VerifyWasCalledOnce().UnlockByPull(testdata.Pull.BaseRepo.FullName, testdata.Pull.Num) + commitUpdater.VerifyWasCalledOnce().UpdateCombined(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), + Eq(models.PendingCommitStatus), Eq(command.Plan)) + commitUpdater.VerifyWasCalled(Never()).UpdateCombined(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), + Eq(models.FailedCommitStatus), Any[command.Name]()) } func TestRunAutoplanCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_True(t *testing.T) { @@ -853,6 +844,8 @@ func TestRunAutoplanCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_Tru ch.RunAutoplanCommand(testdata.GithubRepo, testdata.GithubRepo, testdata.Pull, testdata.User) pendingPlanFinder.VerifyWasCalled(Never()).DeletePlans(Any[string]()) lockingLocker.VerifyWasCalled(Never()).UnlockByPull(Any[string](), Any[int]()) + commitUpdater.VerifyWasCalledOnce().UpdateCombined(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), + Eq(models.PendingCommitStatus), Eq(command.Plan)) } func TestRunCommentCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_False(t *testing.T) { From 32fb07e8229c50cbcd3504d6ce7ccb91be6e5e2b Mon Sep 17 00:00:00 2001 From: X-Guardian <32168619+X-Guardian@users.noreply.github.com> Date: Sun, 26 Jan 2025 19:29:16 +0000 Subject: [PATCH 3/4] Fix formatting Signed-off-by: X-Guardian <32168619+X-Guardian@users.noreply.github.com> --- server/events/command_runner.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 645cc10acf..30a82105a9 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -360,7 +360,7 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead return } -// Update the combined plan or apply commit status to pending + // Update the combined plan or apply commit status to pending switch cmd.Name { case command.Plan: if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Plan); err != nil { @@ -370,7 +370,7 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead if err := c.CommitStatusUpdater.UpdateCombined(ctx.Log, ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, command.Apply); err != nil { ctx.Log.Warn("unable to update apply commit status: %s", err) } -} + } err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx, cmd) From acdccbc911739e7347f0683ae3303ab73d9366ef Mon Sep 17 00:00:00 2001 From: X-Guardian <32168619+X-Guardian@users.noreply.github.com> Date: Sun, 26 Jan 2025 19:29:57 +0000 Subject: [PATCH 4/4] Fix events controller e2e tests Signed-off-by: X-Guardian <32168619+X-Guardian@users.noreply.github.com> --- server/controllers/events/events_controller_e2e_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index 4588b04127..d06b237b9f 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -1647,6 +1647,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, PullStatusFetcher: backend, DisableAutoplan: opt.disableAutoplan, + CommitStatusUpdater: commitStatusUpdater, } repoAllowlistChecker, err := events.NewRepoAllowlistChecker("*")