Repository: akuity/kargo
Pull request #1702 (2024-03-27 22:47:33)

fix(controller): keep Stage history up-to-date
This ensures that besides the initial push of a `FreightReference` to the history of a `Stage`, it is actually kept up-to-date with any new changes at the end of a reconciliation run. By doing this, we can provide the user with historical bookkeeping of verification information for the past `Freight` of a `Stage`.

Required to properly address #1694 

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *docs-kargo-akuity-io* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d2a577c279f3287f39513e43b533047b2d161295 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/6604a207960f2f0008bf801f |
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/1702?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) Report
Attention: Patch coverage is `92.85714%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 44.17%. Comparing base [(`a9d3273`)](https://app.codecov.io/gh/akuity/kargo/commit/a9d3273415676688c2b39573d54c4ce849abaa9d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) to head [(`d2a577c`)](https://app.codecov.io/gh/akuity/kargo/pull/1702?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).

| [Files](https://app.codecov.io/gh/akuity/kargo/pull/1702?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) | Patch % | Lines |
|---|---|---|
| [internal/controller/promotions/promotions.go](https://app.codecov.io/gh/akuity/kargo/pull/1702?src=pr&el=tree&filepath=internal%2Fcontroller%2Fpromotions%2Fpromotions.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity#diff-aW50ZXJuYWwvY29udHJvbGxlci9wcm9tb3Rpb25zL3Byb21vdGlvbnMuZ28=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/akuity/kargo/pull/1702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1702      +/-   ##
==========================================
- Coverage   44.18%   44.17%   -0.01%     
==========================================
  Files         206      206              
  Lines       13137    13136       -1     
==========================================
- Hits         5804     5803       -1     
  Misses       7083     7083              
  Partials      250      250              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/1702?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).


Patch:
diff --git a/api/v1alpha1/stage_types.go b/api/v1alpha1/stage_types.go
index 35c296a1..a2dddf73 100644
--- a/api/v1alpha1/stage_types.go
+++ b/api/v1alpha1/stage_types.go
@@ -544,46 +544,31 @@ type FreightReference struct {
 
 type FreightReferenceStack []FreightReference
 
-// Empty returns a bool indicating whether the FreightReferenceStack is
-// empty. Nil counts as empty.
-func (f FreightReferenceStack) Empty() bool {
-	return len(f) == 0
-}
-
-// Pop removes and returns the leading element from a FreightReferenceStack. If
-// the FreightReferenceStack is empty, the FreightReferenceStack is not modified
-// and an empty FreightReference is returned instead. A boolean is also returned
-// indicating whether the returned FreightReference came from the top of the
-// stack (true) or is a zero value for that type (false).
-func (f *FreightReferenceStack) Pop() (FreightReference, bool) {
-	item, ok := f.Top()
-	if ok {
-		*f = (*f)[1:]
+// UpdateOrPush updates the FreightReference with the same name as the provided
+// FreightReference or appends the provided FreightReference to the stack if no
+// such FreightReference is found.
+//
+// The order of existing items in the stack is preserved, and new items without
+// a matching name are appended to the top of the stack. If the stack grows
+// beyond 10 items, the bottom items are removed.
+func (f *FreightReferenceStack) UpdateOrPush(freight ...FreightReference) {
+	var newStack FreightReferenceStack
+	for _, i := range freight {
+		var found bool
+		for fi, item := range *f {
+			if i.Name == item.Name {
+				(*f)[fi] = i
+				found = true
+				break
+			}
+		}
+		if !found {
+			newStack = append(newStack, i)
+		}
 	}
-	return item, ok
-}
 
-// Top returns the leading element from a FreightReferenceStack without
-// modifying the FreightReferenceStack. If the FreightReferenceStack is empty,
-// an empty FreightReference is returned instead. A boolean is also returned
-// indicating whether the returned FreightReference came from the top of the
-// stack (true) or is a zero value for that type (false).
-func (f FreightReferenceStack) Top() (FreightReference, bool) {
-	if f.Empty() {
-		return FreightReference{}, false
-	}
-	item := *f[0].DeepCopy()
-	return item, true
-}
-
-// Push pushes one or more Freight onto the FreightReferenceStack. The order of
-// the new elements at the top of the stack will be equal to the order in which
-// they were passed to this function. i.e. The first new element passed will be
-// the element at the top of the stack. If resulting modification grow the depth
-// of the stack beyond 10 elements, the stack is truncated at the bottom. i.e.
-// Modified to contain only the top 10 elements.
-func (f *FreightReferenceStack) Push(freight ...FreightReference) {
-	*f = append(freight, *f...)
+	*f = append(newStack, *f...)
+
 	const maxSize = 10
 	if len(*f) > maxSize {
 		*f = (*f)[:maxSize]
diff --git a/internal/controller/promotions/promotions.go b/internal/controller/promotions/promotions.go
index f137629b..10700134 100644
--- a/internal/controller/promotions/promotions.go
+++ b/internal/controller/promotions/promotions.go
@@ -368,7 +368,7 @@ func (r *reconciler) promote(
 			// control-flow stages in the first place)
 			if stage.Spec.PromotionMechanisms != nil {
 				status.CurrentFreight = &nextFreight
-				status.History.Push(nextFreight)
+				status.History.UpdateOrPush(nextFreight)
 			}
 		})
 		if err != nil {
diff --git a/internal/controller/stages/stages.go b/internal/controller/stages/stages.go
index cdedd231..533cd6ed 100644
--- a/internal/controller/stages/stages.go
+++ b/internal/controller/stages/stages.go
@@ -717,6 +717,12 @@ func (r *reconciler) syncNormalStage(
 	} else {
 		freightLogger := logger.WithField("freight", status.CurrentFreight.Name)
 
+		// Push the latest state of the current Freight to the history at the
+		// end of each reconciliation loop.
+		defer func() {
+			status.History.UpdateOrPush(*status.CurrentFreight)
+		}()
+
 		// Check health
 		status.Health = r.checkHealthFn(
 			ctx,


Test patch:
diff --git a/api/v1alpha1/stage_types_test.go b/api/v1alpha1/stage_types_test.go
index ae9a759c..07e5c024 100644
--- a/api/v1alpha1/stage_types_test.go
+++ b/api/v1alpha1/stage_types_test.go
@@ -6,113 +6,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestFreightReferenceStackEmpty(t *testing.T) {
-	testCases := []struct {
-		name           string
-		stack          FreightReferenceStack
-		expectedResult bool
-	}{
-		{
-			name:           "stack is nil",
-			stack:          nil,
-			expectedResult: true,
-		},
-		{
-			name:           "stack is empty",
-			stack:          FreightReferenceStack{},
-			expectedResult: true,
-		},
-		{
-			name:           "stack has items",
-			stack:          FreightReferenceStack{{Name: "foo"}},
-			expectedResult: false,
-		},
-	}
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			require.Equal(t, testCase.expectedResult, testCase.stack.Empty())
-		})
-	}
-}
-
-func TestFreightReferenceStackPop(t *testing.T) {
-	testCases := []struct {
-		name            string
-		stack           FreightReferenceStack
-		expectedStack   FreightReferenceStack
-		expectedFreight FreightReference
-		expectedOK      bool
-	}{
-		{
-			name:            "stack is nil",
-			stack:           nil,
-			expectedStack:   nil,
-			expectedFreight: FreightReference{},
-			expectedOK:      false,
-		},
-		{
-			name:            "stack is empty",
-			stack:           FreightReferenceStack{},
-			expectedStack:   FreightReferenceStack{},
-			expectedFreight: FreightReference{},
-			expectedOK:      false,
-		},
-		{
-			name:            "stack has items",
-			stack:           FreightReferenceStack{{Name: "foo"}, {Name: "bar"}},
-			expectedStack:   FreightReferenceStack{{Name: "bar"}},
-			expectedFreight: FreightReference{Name: "foo"},
-			expectedOK:      true,
-		},
-	}
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			freight, ok := testCase.stack.Pop()
-			require.Equal(t, testCase.expectedStack, testCase.stack)
-			require.Equal(t, testCase.expectedFreight, freight)
-			require.Equal(t, testCase.expectedOK, ok)
-		})
-	}
-}
-
-func TestFreightReferenceStackTop(t *testing.T) {
-	testCases := []struct {
-		name            string
-		stack           FreightReferenceStack
-		expectedFreight FreightReference
-		expectedOK      bool
-	}{
-		{
-			name:            "stack is nil",
-			stack:           nil,
-			expectedFreight: FreightReference{},
-			expectedOK:      false,
-		},
-		{
-			name:            "stack is empty",
-			stack:           FreightReferenceStack{},
-			expectedFreight: FreightReference{},
-			expectedOK:      false,
-		},
-		{
-			name:            "stack has items",
-			stack:           FreightReferenceStack{{Name: "foo"}, {Name: "bar"}},
-			expectedFreight: FreightReference{Name: "foo"},
-			expectedOK:      true,
-		},
-	}
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			initialLen := len(testCase.stack)
-			freight, ok := testCase.stack.Top()
-			require.Len(t, testCase.stack, initialLen)
-			require.Equal(t, testCase.expectedFreight, freight)
-			require.Equal(t, testCase.expectedOK, ok)
-		})
-	}
-}
-
-func TestFreightReferenceStackPush(t *testing.T) {
+func TestFreightReferenceStackUpdateOrPush(t *testing.T) {
 	testCases := []struct {
 		name          string
 		stack         FreightReferenceStack
@@ -131,6 +25,17 @@ func TestFreightReferenceStackPush(t *testing.T) {
 			newFreight:    []FreightReference{{Name: "bar"}},
 			expectedStack: FreightReferenceStack{{Name: "bar"}, {Name: "foo"}},
 		},
+		{
+			name:       "initial stack has matching names",
+			stack:      FreightReferenceStack{{Name: "foo"}, {Name: "bar"}},
+			newFreight: []FreightReference{{Name: "bar", Warehouse: "update"}, {Name: "baz"}, {Name: "zab"}},
+			expectedStack: FreightReferenceStack{
+				{Name: "baz"},
+				{Name: "zab"},
+				{Name: "foo"},
+				{Name: "bar", Warehouse: "update"},
+			},
+		},
 		{
 			name: "initial stack is full",
 			stack: FreightReferenceStack{
@@ -144,7 +49,7 @@ func TestFreightReferenceStackPush(t *testing.T) {
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			testCase.stack.Push(testCase.newFreight...)
+			testCase.stack.UpdateOrPush(testCase.newFreight...)
 			require.Equal(t, testCase.expectedStack, testCase.stack)
 		})
 	}