From 6c5a5ca1fe561a10bcfb9ff4074d741719a4db52 Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Tue, 10 Dec 2024 23:38:52 -0500 Subject: [PATCH] make steps return terminal errors under specific circumstances Signed-off-by: Kent Rancourt --- internal/directives/git_pr_waiter.go | 6 ++---- internal/directives/git_pr_waiter_test.go | 4 ++-- internal/directives/git_pusher.go | 13 +++++++++++-- internal/directives/http_requester.go | 3 ++- internal/directives/http_requester_test.go | 2 ++ 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/internal/directives/git_pr_waiter.go b/internal/directives/git_pr_waiter.go index 748bdc581..565d15459 100644 --- a/internal/directives/git_pr_waiter.go +++ b/internal/directives/git_pr_waiter.go @@ -114,10 +114,8 @@ func (g *gitPRWaiter) runPromotionStep( return PromotionStepResult{Status: kargoapi.PromotionPhaseRunning}, nil } if !pr.Merged { - return PromotionStepResult{ - Status: kargoapi.PromotionPhaseFailed, - Message: fmt.Sprintf("pull request %d was closed without being merged", prNumber), - }, err + return PromotionStepResult{Status: kargoapi.PromotionPhaseFailed}, + &terminalError{err: fmt.Errorf("pull request %d was closed without being merged", prNumber)} } return PromotionStepResult{ Status: kargoapi.PromotionPhaseSucceeded, diff --git a/internal/directives/git_pr_waiter_test.go b/internal/directives/git_pr_waiter_test.go index e4610f374..f56507532 100644 --- a/internal/directives/git_pr_waiter_test.go +++ b/internal/directives/git_pr_waiter_test.go @@ -150,8 +150,8 @@ func Test_gitPRWaiter_runPromotionStep(t *testing.T) { }, }, assertions: func(t *testing.T, res PromotionStepResult, err error) { - require.NoError(t, err) - require.Contains(t, res.Message, "closed without being merged") + require.ErrorContains(t, err, "closed without being merged") + require.True(t, isTerminal(err)) require.Equal(t, kargoapi.PromotionPhaseFailed, res.Status) }, }, diff --git a/internal/directives/git_pusher.go b/internal/directives/git_pusher.go index 9f835e72a..262968182 100644 --- a/internal/directives/git_pusher.go +++ b/internal/directives/git_pusher.go @@ -106,8 +106,11 @@ func (g *gitPushPusher) runPromotionStep( fmt.Errorf("error loading working tree from %s: %w", cfg.Path, err) } pushOpts := &git.PushOptions{ - // Start with whatever was specified in the config, which may be empty + // Start with whatever was specified in the config, which may be empty. TargetBranch: cfg.TargetBranch, + // Attempt to rebase on top of the state of the remote branch to help + // avoid conflicts. + PullRebase: true, } // If we're supposed to generate a target branch name, do so if cfg.GenerateTargetBranch { @@ -116,7 +119,7 @@ func (g *gitPushPusher) runPromotionStep( } targetBranch := pushOpts.TargetBranch if targetBranch == "" { - // If retBranch is still empty, we want to set it to the current branch + // If targetBranch is still empty, we want to set it to the current branch // because we will want to return the branch that was pushed to, but we // don't want to mess with the options any further. if targetBranch, err = workTree.CurrentBranch(); err != nil { @@ -125,6 +128,12 @@ func (g *gitPushPusher) runPromotionStep( } } if err = workTree.Push(pushOpts); err != nil { + if git.IsMergeConflict(err) { + // Special case: A merge conflict requires manual resolution and no amount + // of retries will fix that. + return PromotionStepResult{Status: kargoapi.PromotionPhaseFailed}, + &terminalError{err: err} + } return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf("error pushing commits to remote: %w", err) } diff --git a/internal/directives/http_requester.go b/internal/directives/http_requester.go index 6423e26ba..ac9bd0dfc 100644 --- a/internal/directives/http_requester.go +++ b/internal/directives/http_requester.go @@ -5,6 +5,7 @@ import ( "context" "crypto/tls" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -115,7 +116,7 @@ func (h *httpRequester) runPromotionStep( }, nil case failure: return PromotionStepResult{Status: kargoapi.PromotionPhaseFailed}, - fmt.Errorf("HTTP response met failure criteria") + &terminalError{err: errors.New("HTTP response met failure criteria")} default: return PromotionStepResult{Status: kargoapi.PromotionPhaseRunning}, nil } diff --git a/internal/directives/http_requester_test.go b/internal/directives/http_requester_test.go index f5bc8b320..6a1df26a0 100644 --- a/internal/directives/http_requester_test.go +++ b/internal/directives/http_requester_test.go @@ -334,6 +334,7 @@ func Test_httpRequester_runPromotionStep(t *testing.T) { }, assertions: func(t *testing.T, res PromotionStepResult, err error) { require.ErrorContains(t, err, "HTTP response met failure criteria") + require.True(t, isTerminal(err)) require.Equal(t, kargoapi.PromotionPhaseFailed, res.Status) }, }, @@ -346,6 +347,7 @@ func Test_httpRequester_runPromotionStep(t *testing.T) { }, assertions: func(t *testing.T, res PromotionStepResult, err error) { require.ErrorContains(t, err, "HTTP response met failure criteria") + require.True(t, isTerminal(err)) require.Equal(t, kargoapi.PromotionPhaseFailed, res.Status) }, },