make steps return terminal errors under specific circumstances

Signed-off-by: Kent Rancourt <[email protected]>
akuity · Dec 11, 2024 · 6c5a5ca · 6c5a5ca
1 parent 694d79c
commit 6c5a5ca
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)
 			},
 		},