Skip to content

Commit

Permalink
Mark outdated plan checkruns as aborted (#701)
Browse files Browse the repository at this point in the history
This change makes the PR workflow mark outdated plan checkruns as
aborted.

Why?
* During our testing, we encountered an issue where if commits were
rapidly created on a PR, some checkruns for older revisions could be
stuck in pending or in progress state.
  • Loading branch information
asubrocky authored Jun 29, 2023
1 parent 8e839e0 commit a3c243f
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 9 deletions.
3 changes: 3 additions & 0 deletions server/neptune/workflows/activities/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,9 @@ func getCheckStateAndConclusion(internalState internal.CheckRunState) (string, s
case internal.CheckRunSkipped:
state = "completed"
conclusion = "skipped"
case internal.CheckRunCancelled:
state = "completed"
conclusion = "cancelled"
default:
state = string(internalState)
}
Expand Down
1 change: 1 addition & 0 deletions server/neptune/workflows/activities/github/checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const (
CheckRunQueued CheckRunState = "queued"
CheckRunSkipped CheckRunState = "skipped"
CheckRunActionRequired CheckRunState = "action_required"
CheckRunCancelled CheckRunState = "cancelled"
CheckRunUnknown CheckRunState = ""

Approve PlanReviewActionType = "Approve"
Expand Down
53 changes: 44 additions & 9 deletions server/neptune/workflows/internal/pr/revision/processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import (
)

const (
ReviewSignalID = "pr-review"
ReviewSignalID = "pr-review"
CheckRunCancelled = "checkrun was cancelled"
)

type TFWorkflow func(ctx workflow.Context, request terraform.Request) (terraform.Response, error)
Expand Down Expand Up @@ -68,6 +69,18 @@ func (p *Processor) Process(ctx workflow.Context, prRevision Revision) {
futures = append(futures, future)
}

// Mark checkruns as aborted if the context was cancelled, this typically happens if revisions arrive in quick succession
defer func() {
if temporal.IsCanceledError(ctx.Err()) {
ctx, _ := workflow.NewDisconnectedContext(ctx)
p.markCheckRunsAborted(ctx, prRevision, roots)
return
}

// At this point, all workflows should be successful, and we can mark combined plan check run as success
p.markCombinedCheckRun(ctx, prRevision, github.CheckRunSuccess, "")
}()

terraformWorkflowResponses := p.awaitChildTerraformWorkflows(ctx, futures, roots)
// Count all policy successes/failures + handle any failures by listening for approvals in PolicyHandler
var failingTerraformWorkflowResponses []terraform.Response
Expand All @@ -83,8 +96,6 @@ func (p *Processor) Process(ctx workflow.Context, prRevision Revision) {
}
}
p.PolicyHandler.Handle(ctx, prRevision, roots, failingTerraformWorkflowResponses)
// At this point, all workflows should be successful, and we can mark combined plan check run as success
p.markCombinedCheckRunSuccessful(ctx, prRevision)
}

func (p *Processor) processRoot(ctx workflow.Context, root terraformActivities.Root, prRevision Revision, id uuid.UUID) workflow.ChildWorkflowFuture {
Expand Down Expand Up @@ -149,17 +160,18 @@ func (p *Processor) awaitChildTerraformWorkflows(ctx workflow.Context, futures [
return results
}

func (p *Processor) markCombinedCheckRunSuccessful(ctx workflow.Context, revision Revision) {
func (p *Processor) markCombinedCheckRun(ctx workflow.Context, revision Revision, state github.CheckRunState, summary string) {
ctx = workflow.WithRetryPolicy(ctx, temporal.RetryPolicy{
MaximumAttempts: 3,
})

request := notifier.GithubCheckRunRequest{
Title: notifier.CombinedPlanCheckRunTitle,
Sha: revision.Revision,
Repo: revision.Repo,
State: github.CheckRunSuccess,
Mode: terraformActivities.PR,
Title: notifier.CombinedPlanCheckRunTitle,
Sha: revision.Revision,
Repo: revision.Repo,
State: state,
Mode: terraformActivities.PR,
Summary: summary,
}
// ID is empty because we want to create a new check run
// TODO: do we want to create a new check run, are we persisting the original mark plan CR as queued in gateway?
Expand All @@ -168,3 +180,26 @@ func (p *Processor) markCombinedCheckRunSuccessful(ctx workflow.Context, revisio
workflow.GetLogger(ctx).Error("unable to update check run with validation error", internalContext.ErrKey, err)
}
}

func (p *Processor) markCheckRunsAborted(ctx workflow.Context, revision Revision, roots map[string]RootInfo) {
p.markCombinedCheckRun(ctx, revision, github.CheckRunCancelled, CheckRunCancelled)

for _, rootInfo := range roots {
ctx = workflow.WithRetryPolicy(ctx, temporal.RetryPolicy{
MaximumAttempts: 3,
})

request := notifier.GithubCheckRunRequest{
Title: notifier.BuildPlanCheckRunTitle(rootInfo.Root.Name),
Sha: revision.Revision,
Repo: revision.Repo,
State: github.CheckRunCancelled,
Mode: terraformActivities.PR,
Summary: CheckRunCancelled,
}
_, err := p.GithubCheckRunCache.CreateOrUpdate(ctx, rootInfo.ID.String(), request)
if err != nil {
workflow.GetLogger(ctx).Error("unable to update check run with validation error", internalContext.ErrKey, err)
}
}
}

0 comments on commit a3c243f

Please sign in to comment.