Repository: akuity/kargo

Issue:
Add step numbers to the left of the steps on the promotion details page
# Proposed Feature

Can we add step numbers to the left of the steps on the promotion details page? It would make it easier to determine which one the error is referring to (I know there is already and X, but the numbers would also be nice because it uses the step numbers in the error message).

Speaking about the error messages, the step numbers are not correct for human reading since it is 0-based. we should add 1 to the step number in the error message so that it is accurate.

<img width="777" alt="Image" src="https://github.com/user-attachments/assets/50fd2ee7-cf8d-4b99-af25-0b8492d3e88d" />

# Motivation

It will make it easier to read and use.

# Suggested Implementation

- Add numbers next to the steps
- fix the step numbers in the error message so it is correct (add 1, since it is 0-based)


Discussion:
@github-actions: This issue has been automatically marked as stale because it had no activity for 90 days. It will be closed if no activity occurs in the next 30 days but can be reopened if it becomes relevant again.@kencochrane: @Marvin9 @rpelczar what do you think about this recommendation?@krancour: I like this, but I think there might be some back end work that needs to precede this. There is also the possibility that some back end changes could obviate the need for any UI work here...

You can see in the screenshot above that step indices do appear in error messages at times. If we put 1-based numbers next to each step zero-based indices appear in error messages, it creates a disconnect of sorts. "Step 5" may have a red X next to it, but the error message says the problem is with step 4...

It's tempting to just add 1 to indices when preparing such error messages and that will address the mismatch. The thing I'm _then_ a bit uncomfortable with is the accuracy of the error message will feel somewhat reduced. If you looked at that error message in a vacuum, without the UI in front of you, and (reasonably) assumed 0-based indices and you'd end up investigating a failure in the wrong step...

The thing that I think can make _all_ of this consistent and accurate, with (potentially) no UI changes needed, is error messages could use the _alias_ to identify a failed step instead of using its index. Aliases are already displayed in the UI (the blue, label-looking things on the right) so the error messages would match what you see in the UI. If you saw the message outside the context of the UI, you wouldn't be confused as to its meaning.

You may note that not all steps appear to _have_ aliases, but the system does assign aliases to steps that aren't explicitly assigned one by the user. I can look into why they're not showing up here. (I don't think it's a UI issue. I have a feeling they're accidentally not persisted.)

Let me try to make these changes first and see how we feel about things then, and if we decide we still want to put numbers next to each step, we can do so without worry that they won't line up with error messages.







@kencochrane: I agree, there is more than one way to fix this issue, and if we can fix it a different way, then I'm OK with that. I like using the alias names, it would make it more human readable, and work for both UI and CLI.@krancour: So here's what it would look like. This was quick-n-dirty and would need some cleanup, but it was entirely back-end changes:

![Image](https://github.com/user-attachments/assets/dfaae0ac-3b47-4483-a8b4-f44ce9fd2927)

(If you're trying to interpret this, "base" was a promotion task that got expanded here.)@kencochrane: @krancour thanks, that is much better. @krancour: I'll PR it tomorrow after it's cleaned up. We can easily sneak it into v1.5.0.

Solution patch:
diff --git a/api/v1alpha1/promotion_types.go b/api/v1alpha1/promotion_types.go
index eb9c5105..75cb9878 100644
--- a/api/v1alpha1/promotion_types.go
+++ b/api/v1alpha1/promotion_types.go
@@ -288,9 +288,9 @@ func (s *PromotionStep) GetAlias(i int) string {
 	case s.As != "":
 		return s.As
 	case s.Task != nil:
-		return fmt.Sprintf("task-%d", i)
+		return fmt.Sprintf("task-%d", i+1)
 	default:
-		return fmt.Sprintf("step-%d", i)
+		return fmt.Sprintf("step-%d", i+1)
 	}
 }
 
diff --git a/internal/kargo/promotion_builder.go b/internal/kargo/promotion_builder.go
index a368bf66..c952ead8 100644
--- a/internal/kargo/promotion_builder.go
+++ b/internal/kargo/promotion_builder.go
@@ -113,6 +113,7 @@ func (b *PromotionBuilder) InflateSteps(ctx context.Context, promo *kargoapi.Pro
 			}
 			steps = append(steps, taskSteps...)
 		default:
+			step.As = step.GetAlias(i)
 			steps = append(steps, step)
 		}
 	}
diff --git a/internal/promotion/simple_engine.go b/internal/promotion/simple_engine.go
index 3c8834de..af8304bc 100644
--- a/internal/promotion/simple_engine.go
+++ b/internal/promotion/simple_engine.go
@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"os"
 	"regexp"
-	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -131,21 +130,11 @@ stepLoop:
 		default:
 		}
 
-		// Prepare the step for execution by setting the alias.
-		if step.Alias, err = e.stepAlias(step.Alias, i); err != nil {
-			stepExecMeta.Status = kargoapi.PromotionStepStatusErrored
-			stepExecMeta.Message = fmt.Sprintf("error getting step alias: %s", err)
-			// Continue, because despite this failure, some steps' "if" conditions may
-			// still allow them to run.
-			continue
-		}
-		stepExecMeta.Alias = step.Alias
-
 		// Check if the step should be skipped.
 		var skip bool
 		if skip, err = step.Skip(ctx, e.kargoClient, promoCtx, state); err != nil {
 			stepExecMeta.Status = kargoapi.PromotionStepStatusErrored
-			stepExecMeta.Message = fmt.Sprintf("error checking if step %d should be skipped: %s", i, err)
+			stepExecMeta.Message = fmt.Sprintf("error checking if step %q should be skipped: %s", step.Alias, err)
 			// Continue, because despite this failure, some steps' "if" conditions may
 			// still allow them to run.
 			continue
@@ -196,7 +185,7 @@ stepLoop:
 			// Deal with statuses that no step should have returned.
 			stepExecMeta.FinishedAt = ptr.To(metav1.Now())
 			stepExecMeta.Status = kargoapi.PromotionStepStatusErrored
-			stepExecMeta.Message = fmt.Sprintf("step %d returned an invalid status: %s", i, result.Status)
+			stepExecMeta.Message = fmt.Sprintf("step %q returned an invalid status: %s", step.Alias, result.Status)
 			// Continue, because despite this failure, some steps' "if" conditions may
 			// still allow them to run.
 			continue
@@ -221,7 +210,7 @@ stepLoop:
 			if message == "" {
 				message = "no details provided"
 			}
-			err = fmt.Errorf("step %d errored: %s", i, message)
+			err = fmt.Errorf("step %q errored: %s", step.Alias, message)
 		}
 
 		// At this point, we've sorted out any discrepancies between the status and
@@ -253,7 +242,7 @@ stepLoop:
 				stepExecMeta.FinishedAt = ptr.To(metav1.Now())
 				stepExecMeta.Status = kargoapi.PromotionStepStatusErrored
 				stepExecMeta.Message = fmt.Sprintf(
-					"step %d met error threshold of %d: %s", i,
+					"step %q met error threshold of %d: %s", step.Alias,
 					errorThreshold, stepExecMeta.Message,
 				)
 				// Continue, because despite this failure, some steps' "if" conditions
@@ -272,7 +261,7 @@ stepLoop:
 			// Timeout has elapsed.
 			stepExecMeta.FinishedAt = ptr.To(metav1.Now())
 			stepExecMeta.Status = kargoapi.PromotionStepStatusErrored
-			stepExecMeta.Message = fmt.Sprintf("step %d timed out after %s", i, timeout.String())
+			stepExecMeta.Message = fmt.Sprintf("step %q timed out after %s", step.Alias, timeout.String())
 			// Continue, because despite this failure, some steps' "if" conditions may
 			// still allow them to run.
 			continue
@@ -369,7 +358,7 @@ func (e *simpleEngine) executeStep(
 
 	result, err := runner.Run(ctx, stepCtx)
 	if err != nil {
-		err = fmt.Errorf("error running step %q: %w", step.Kind, err)
+		err = fmt.Errorf("error running step %q: %w", step.Alias, err)
 	}
 	return result, err
 }
@@ -403,21 +392,6 @@ func (e *simpleEngine) prepareStepContext(
 	}, nil
 }
 
-// stepAlias returns the alias for a step. If the alias is empty, a default
-// alias is returned based on the step index.
-func (e *simpleEngine) stepAlias(alias string, index int64) (string, error) {
-	if alias = strings.TrimSpace(alias); alias != "" {
-		// A webhook enforces this regex as well, but we're checking here to
-		// account for the possibility of EXISTING Stages with a promotionTemplate
-		// containing a step with a now-reserved alias.
-		if ReservedStepAliasRegex.MatchString(alias) {
-			return "", fmt.Errorf("step alias %q is forbidden", alias)
-		}
-		return alias, nil
-	}
-	return fmt.Sprintf("step-%d", index), nil
-}
-
 // setupWorkDir creates a temporary working directory if one is not provided.
 func (e *simpleEngine) setupWorkDir(existingDir string) (string, error) {
 	if existingDir != "" {



Test patch:
diff --git a/internal/kargo/promotion_builder_test.go b/internal/kargo/promotion_builder_test.go
index 666c6be0..204e4459 100644
--- a/internal/kargo/promotion_builder_test.go
+++ b/internal/kargo/promotion_builder_test.go
@@ -560,8 +560,8 @@ func TestPromotionBuilder_inflateTaskSteps(t *testing.T) {
 				require.NoError(t, err)
 				require.Len(t, steps, 2)
 
-				assert.Equal(t, "custom-alias::step-0", steps[0].As)
-				assert.Equal(t, "custom-alias::step-1", steps[1].As)
+				assert.Equal(t, "custom-alias::step-1", steps[0].As)
+				assert.Equal(t, "custom-alias::step-2", steps[1].As)
 			},
 		},
 		{
diff --git a/internal/promotion/simple_engine_test.go b/internal/promotion/simple_engine_test.go
index a24ac865..8691ecfd 100644
--- a/internal/promotion/simple_engine_test.go
+++ b/internal/promotion/simple_engine_test.go
@@ -146,24 +146,6 @@ func TestSimpleEngine_executeSteps(t *testing.T) {
 		steps       []Step
 		assertions  func(*testing.T, Result)
 	}{
-		{
-			name: "fail on invalid step alias",
-			steps: []Step{
-				{Kind: "success-step", Alias: "step-1"},
-			},
-			assertions: func(t *testing.T, result Result) {
-				assert.Equal(t, kargoapi.PromotionPhaseErrored, result.Status)
-				assert.Contains(t, result.Message, "is forbidden")
-				assert.Equal(t, int64(0), result.CurrentStep)
-
-				assert.Len(t, result.StepExecutionMetadata, 1)
-
-				assert.Equal(t, kargoapi.PromotionStepStatusErrored, result.StepExecutionMetadata[0].Status)
-				assert.Contains(t, result.StepExecutionMetadata[0].Message, "is forbidden")
-				assert.Nil(t, result.StepExecutionMetadata[0].StartedAt)
-				assert.Nil(t, result.StepExecutionMetadata[0].FinishedAt)
-			},
-		},
 		{
 			name:  "runner not found",
 			steps: []Step{{Kind: "unknown-step"}},
@@ -767,7 +749,10 @@ func TestSimpleEngine_executeStep(t *testing.T) {
 		},
 		{
 			name: "step execution failure",
-			step: Step{Kind: "error-step"},
+			step: Step{
+				Kind:  "error-step",
+				Alias: "my-step",
+			},
 			runner: &mockStepRunner{
 				name: "error-step",
 				runResult: promotion.StepResult{
@@ -776,7 +761,7 @@ func TestSimpleEngine_executeStep(t *testing.T) {
 				runErr: errors.New("something went wrong"),
 			},
 			assertions: func(t *testing.T, result promotion.StepResult, err error) {
-				assert.ErrorContains(t, err, "error running step \"error-step\"")
+				assert.ErrorContains(t, err, "error running step \"my-step\"")
 				assert.ErrorContains(t, err, "something went wrong")
 				assert.Equal(t, kargoapi.PromotionStepStatusErrored, result.Status)
 			},
@@ -844,55 +829,6 @@ func TestSimpleEngine_prepareStepContext(t *testing.T) {
 	}
 }
 
-func TestSimpleEngine_stepAlias(t *testing.T) {
-	tests := []struct {
-		name       string
-		alias      string
-		index      int64
-		assertions func(*testing.T, string, error)
-	}{
-		{
-			name:  "use provided alias",
-			alias: "custom-step",
-			assertions: func(t *testing.T, alias string, err error) {
-				assert.NoError(t, err)
-				assert.Equal(t, "custom-step", alias)
-			},
-		},
-		{
-			name:  "generate default alias",
-			index: 42,
-			assertions: func(t *testing.T, alias string, err error) {
-				assert.NoError(t, err)
-				assert.Equal(t, "step-42", alias)
-			},
-		},
-		{
-			name:  "reject reserved alias",
-			alias: "step-1",
-			assertions: func(t *testing.T, _ string, err error) {
-				assert.ErrorContains(t, err, "forbidden")
-			},
-		},
-		{
-			name:  "trim whitespace",
-			alias: "  step  ",
-			assertions: func(t *testing.T, alias string, err error) {
-				assert.NoError(t, err)
-				assert.Equal(t, "step", alias)
-			},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			engine := &simpleEngine{}
-			alias, err := engine.stepAlias(tt.alias, tt.index)
-			tt.assertions(t, alias, err)
-		})
-	}
-}
-
 func TestSimpleEngine_setupWorkDir(t *testing.T) {
 	tmpDir := t.TempDir()