fix: handle annotated git tags correctly in repo server cache (#21771)

applicationset/controllers/applicationset_controller.go

@@ -1135,7 +1135,7 @@ func (r *ApplicationSetReconciler) updateApplicationSetApplicationStatus(ctx con
 			// upgrade any existing AppStatus that might have been set by an older argo-cd version
 			// note: currentAppStatus.TargetRevisions may be set to empty list earlier during migrations,
 			// to prevent other usage of r.Client.Status().Update to fail before reaching here.
-			if currentAppStatus.TargetRevisions == nil || len(currentAppStatus.TargetRevisions) == 0 {
+			if len(currentAppStatus.TargetRevisions) == 0 {
 				currentAppStatus.TargetRevisions = app.Status.GetRevisions()
 			}
 		}

applicationset/generators/git.go

@@ -78,7 +78,7 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
 			return nil, fmt.Errorf("error getting project %s: %w", project, err)
 		}
 		// we need to verify the signature on the Git revision if GPG is enabled
-		verifyCommit = appProject.Spec.SignatureKeys != nil && len(appProject.Spec.SignatureKeys) > 0 && gpg.IsGPGEnabled()
+		verifyCommit = len(appProject.Spec.SignatureKeys) > 0 && gpg.IsGPGEnabled()
 	}
 
 	var err error

applicationset/generators/plugin_test.go

@@ -694,7 +694,7 @@ func TestPluginGenerateParams(t *testing.T) {
 				require.NoError(t, err)
 				gotJson, err := json.Marshal(got)
 				require.NoError(t, err)
-				assert.Equal(t, string(expectedJson), string(gotJson))
+				assert.JSONEq(t, string(expectedJson), string(gotJson))
 			}
 		})
 	}

applicationset/utils/createOrUpdate_test.go

@@ -229,7 +229,7 @@ spec:
 			require.NoError(t, err)
 			yamlExpected, err := yaml.Marshal(tc.expectedApp)
 			require.NoError(t, err)
-			assert.Equal(t, string(yamlExpected), string(yamlFound))
+			assert.YAMLEq(t, string(yamlExpected), string(yamlFound))
 		})
 	}
 }

applicationset/utils/utils.go

@@ -268,7 +268,7 @@ func (r *Render) RenderTemplateParams(tmpl *argoappsv1.Application, syncPolicy *
 	// b) there IS a syncPolicy, but preserveResourcesOnDeletion is set to false
 	// See TestRenderTemplateParamsFinalizers in util_test.go for test-based definition of behaviour
 	if (syncPolicy == nil || !syncPolicy.PreserveResourcesOnDeletion) &&
-		(replacedTmpl.ObjectMeta.Finalizers == nil || len(replacedTmpl.ObjectMeta.Finalizers) == 0) {
+		len(replacedTmpl.ObjectMeta.Finalizers) == 0 {
 		replacedTmpl.ObjectMeta.Finalizers = []string{"resources-finalizer.argocd.argoproj.io"}
 	}
 

cmd/argocd/commands/admin/project_allowlist_test.go

@@ -17,5 +17,5 @@ func TestProjectAllowListGen(t *testing.T) {
 
 	globalProj, err := generateProjectAllowList(resourceList, "testdata/test_clusterrole.yaml", "testproj")
 	require.NoError(t, err)
-	assert.Positive(t, len(globalProj.Spec.NamespaceResourceWhitelist))
+	assert.NotEmpty(t, globalProj.Spec.NamespaceResourceWhitelist)
 }

cmd/argocd/commands/app.go

@@ -1256,7 +1256,7 @@ func findandPrintDiff(ctx context.Context, app *argoappv1.Application, proj *arg
 	if diffOptions.local != "" {
 		localObjs := groupObjsByKey(getLocalObjects(ctx, app, proj, diffOptions.local, diffOptions.localRepoRoot, argoSettings.AppLabelKey, diffOptions.cluster.Info.ServerVersion, diffOptions.cluster.Info.APIVersions, argoSettings.KustomizeOptions, argoSettings.TrackingMethod), liveObjs, app.Spec.Destination.Namespace)
 		items = groupObjsForDiff(resources, localObjs, items, argoSettings, app.InstanceName(argoSettings.ControllerNamespace), app.Spec.Destination.Namespace)
-	} else if diffOptions.revision != "" || (diffOptions.revisions != nil && len(diffOptions.revisions) > 0) {
+	} else if diffOptions.revision != "" || len(diffOptions.revisions) > 0 {
 		var unstructureds []*unstructured.Unstructured
 		for _, mfst := range diffOptions.res.Manifests {
 			obj, err := argoappv1.UnmarshalToUnstructured(mfst)

cmd/argocd/commands/common_test.go

@@ -81,14 +81,14 @@ func Test_PrintResource(t *testing.T) {
 		return err
 	})
 	require.NoError(t, err)
-	assert.Equal(t, expectYamlSingle, str)
+	assert.YAMLEq(t, expectYamlSingle, str)
 
 	str, err = captureOutput(func() error {
 		err := PrintResource(testResource, "json")
 		return err
 	})
 	require.NoError(t, err)
-	assert.Equal(t, expectJsonSingle, str)
+	assert.JSONEq(t, expectJsonSingle, str)
 
 	err = PrintResource(testResource, "unknown")
 	require.Error(t, err)
@@ -116,28 +116,28 @@ func Test_PrintResourceList(t *testing.T) {
 		return err
 	})
 	require.NoError(t, err)
-	assert.Equal(t, expectYamlList, str)
+	assert.YAMLEq(t, expectYamlList, str)
 
 	str, err = captureOutput(func() error {
 		err := PrintResourceList(testResource, "json", false)
 		return err
 	})
 	require.NoError(t, err)
-	assert.Equal(t, expectJsonList, str)
+	assert.JSONEq(t, expectJsonList, str)
 
 	str, err = captureOutput(func() error {
 		err := PrintResourceList(testResource2, "yaml", true)
 		return err
 	})
 	require.NoError(t, err)
-	assert.Equal(t, expectYamlSingle, str)
+	assert.YAMLEq(t, expectYamlSingle, str)
 
 	str, err = captureOutput(func() error {
 		err := PrintResourceList(testResource2, "json", true)
 		return err
 	})
 	require.NoError(t, err)
-	assert.Equal(t, expectJsonSingle, str)
+	assert.JSONEq(t, expectJsonSingle, str)
 
 	err = PrintResourceList(testResource, "unknown", false)
 	require.Error(t, err)

cmd/util/cluster_test.go

@@ -1,7 +1,6 @@
 package util
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -53,8 +52,8 @@ func Test_newCluster(t *testing.T) {
 		&v1alpha1.AWSAuthConfig{},
 		&v1alpha1.ExecProviderConfig{}, labels, nil)
 
-	assert.True(t, strings.Contains(string(clusterWithFiles.Config.CertData), "test-cert-data"))
-	assert.True(t, strings.Contains(string(clusterWithFiles.Config.KeyData), "test-key-data"))
+	assert.Contains(t, string(clusterWithFiles.Config.CertData), "test-cert-data")
+	assert.Contains(t, string(clusterWithFiles.Config.KeyData), "test-key-data")
 	assert.Equal(t, "", clusterWithFiles.Config.BearerToken)
 	assert.Equal(t, labels, clusterWithFiles.Labels)
 	assert.Nil(t, clusterWithFiles.Annotations)

controller/appcontroller_test.go

@@ -2122,7 +2122,7 @@ func TestHelmValuesObjectHasReplaceStrategy(t *testing.T) {
 		app,
 		appModified)
 	require.NoError(t, err)
-	assert.Equal(t, `{"status":{"sync":{"comparedTo":{"source":{"helm":{"valuesObject":{"key":["value-modified1"]}}}}}}}`, string(patch))
+	assert.JSONEq(t, `{"status":{"sync":{"comparedTo":{"source":{"helm":{"valuesObject":{"key":["value-modified1"]}}}}}}}`, string(patch))
 }
 
 func TestAppStatusIsReplaced(t *testing.T) {

controller/metrics/metrics_test.go

@@ -380,7 +380,7 @@ func assertMetricsNotPrinted(t *testing.T, expectedLines, body string) {
 		if line == "" {
 			continue
 		}
-		assert.False(t, strings.Contains(body, expectedLines))
+		assert.NotContains(t, body, expectedLines)
 	}
 }
 

controller/state.go

@@ -433,7 +433,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 
 	// When signature keys are defined in the project spec, we need to verify the signature on the Git revision
 	verifySignature := false
-	if project.Spec.SignatureKeys != nil && len(project.Spec.SignatureKeys) > 0 && gpg.IsGPGEnabled() {
+	if len(project.Spec.SignatureKeys) > 0 && gpg.IsGPGEnabled() {
 		verifySignature = true
 	}
 

pkg/apis/application/v1alpha1/types.go

@@ -233,7 +233,7 @@ func (a *ApplicationSpec) GetSources() ApplicationSources {
 }
 
 func (a *ApplicationSpec) HasMultipleSources() bool {
-	return a.Sources != nil && len(a.Sources) > 0
+	return len(a.Sources) > 0
 }
 
 func (a *ApplicationSpec) GetSourcePtrByPosition(sourcePosition int) *ApplicationSource {

pkg/apis/application/v1alpha1/types_test.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"path"
 	"reflect"
-	"strings"
 	"testing"
 	"time"
 
@@ -440,7 +439,7 @@ func TestAppProject_IsDestinationPermitted_PermitOnlyProjectScopedClusters(t *te
 		return nil, errors.New("some error")
 	})
 	require.Error(t, err)
-	assert.True(t, strings.Contains(err.Error(), "could not retrieve project clusters"))
+	assert.Contains(t, err.Error(), "could not retrieve project clusters")
 }
 
 func TestAppProject_IsGroupKindPermitted(t *testing.T) {

reposerver/cache/cache_test.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"strings"
 	"testing"
 	"time"
 
@@ -337,7 +336,7 @@ func TestCachedManifestResponse_ShallowCopyExpectedFields(t *testing.T) {
 	// go do that first :)
 
 	for _, expectedField := range expectedFields {
-		assert.Truef(t, strings.Contains(string(str), "\""+expectedField+"\""), "Missing field: %s", expectedField)
+		assert.Containsf(t, string(str), "\""+expectedField+"\"", "Missing field: %s", expectedField)
 	}
 }
 

reposerver/repository/repository_test.go

@@ -340,7 +340,7 @@ func TestGenerateManifests_EmptyCache(t *testing.T) {
 
 	res, err := service.GenerateManifest(context.Background(), &q)
 	require.NoError(t, err)
-	assert.Positive(t, len(res.Manifests))
+	assert.NotEmpty(t, res.Manifests)
 	mockCache.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{
 		ExternalSets:    2,
 		ExternalGets:    2,

server/application/application_test.go

@@ -2105,7 +2105,7 @@ func TestSplitStatusPatch(t *testing.T) {
 		otherFields := `{"operation":{"eee":"fff"},"spec":{"aaa":"bbb"},"status":{"ccc":"ddd"}}`
 		nonStatus, status, err := splitStatusPatch([]byte(otherFields))
 		require.NoError(t, err)
-		assert.Equal(t, `{"operation":{"eee":"fff"},"spec":{"aaa":"bbb"}}`, string(nonStatus))
+		assert.JSONEq(t, `{"operation":{"eee":"fff"},"spec":{"aaa":"bbb"}}`, string(nonStatus))
 		assert.Equal(t, statusPatch, string(status))
 	}
 }

server/logout/logout_test.go

@@ -328,7 +328,7 @@ func TestHandlerConstructLogoutURL(t *testing.T) {
 			tt.handler.ServeHTTP(tt.responseRecorder, tt.request)
 			if status := tt.responseRecorder.Code; status != http.StatusSeeOther {
 				if !tt.wantErr {
-					t.Errorf(tt.responseRecorder.Body.String())
+					t.Error(tt.responseRecorder.Body.String())
 					t.Errorf("handler returned wrong status code: " + fmt.Sprintf("%d", tt.responseRecorder.Code))
 				}
 			} else {

test/e2e/accounts_test.go

@@ -2,7 +2,6 @@ package e2e
 
 import (
 	"context"
-	"strings"
 	"testing"
 
 	"github.com/argoproj/pkg/errors"
@@ -50,7 +49,7 @@ func TestCanIGetLogsAllowNoSwitch(t *testing.T) {
 		CanIGetLogs().
 		Then().
 		AndCLIOutput(func(output string, err error) {
-			assert.True(t, strings.Contains(output, "yes"))
+			assert.Contains(t, output, "yes")
 		})
 }
 
@@ -65,7 +64,7 @@ func TestCanIGetLogsDenySwitchOn(t *testing.T) {
 		CanIGetLogs().
 		Then().
 		AndCLIOutput(func(output string, err error) {
-			assert.True(t, strings.Contains(output, "no"))
+			assert.Contains(t, output, "no")
 		})
 }
 
@@ -93,7 +92,7 @@ func TestCanIGetLogsAllowSwitchOn(t *testing.T) {
 		CanIGetLogs().
 		Then().
 		AndCLIOutput(func(output string, err error) {
-			assert.True(t, strings.Contains(output, "yes"))
+			assert.Contains(t, output, "yes")
 		})
 }
 
@@ -108,7 +107,7 @@ func TestCanIGetLogsAllowSwitchOff(t *testing.T) {
 		CanIGetLogs().
 		Then().
 		AndCLIOutput(func(output string, err error) {
-			assert.True(t, strings.Contains(output, "yes"))
+			assert.Contains(t, output, "yes")
 		})
 }
 

test/e2e/app_management_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"reflect"
-	"regexp"
 	"testing"
 	"time"
 
@@ -491,7 +490,7 @@ func TestPatchValuesObject(t *testing.T) {
 		Expect(NoConditions()).
 		And(func(app *Application) {
 			// Check that the patch was a success.
-			assert.Equal(t, `{"some":{"foo":"bar","new":"field"}}`, string(app.Spec.Source.Helm.ValuesObject.Raw))
+			assert.JSONEq(t, `{"some":{"foo":"bar","new":"field"}}`, string(app.Spec.Source.Helm.ValuesObject.Raw))
 		})
 }
 
@@ -770,7 +769,7 @@ func assetSecretDataHidden(t *testing.T, manifest string) {
 	require.NoError(t, err)
 	assert.True(t, hasData)
 	for _, v := range secretData {
-		assert.Regexp(t, regexp.MustCompile(`[*]*`), v)
+		assert.Regexp(t, `[*]*`, v)
 	}
 	var lastAppliedConfigAnnotation string
 	annotations := secret.GetAnnotations()

test/e2e/cluster_test.go

@@ -3,7 +3,6 @@ package e2e
 import (
 	"fmt"
 	"net/url"
-	"strings"
 	"testing"
 	"time"
 
@@ -171,8 +170,8 @@ func TestClusterSet(t *testing.T) {
 		GetByName("in-cluster").
 		Then().
 		AndCLIOutput(func(output string, err error) {
-			assert.True(t, strings.Contains(output, "namespace-edit-1"))
-			assert.True(t, strings.Contains(output, "namespace-edit-2"))
+			assert.Contains(t, output, "namespace-edit-1")
+			assert.Contains(t, output, "namespace-edit-2")
 		})
 }
 

test/e2e/fixture/app/actions.go

@@ -78,6 +78,18 @@ func (a *Actions) AddTag(name string) *Actions {
 	return a
 }
 
+func (a *Actions) AddAnnotatedTag(name string, message string) *Actions {
+	a.context.t.Helper()
+	fixture.AddAnnotatedTag(a.context.t, name, message)
+	return a
+}
+
+func (a *Actions) AddTagWithForce(name string) *Actions {
+	a.context.t.Helper()
+	fixture.AddTagWithForce(a.context.t, name)
+	return a
+}
+
 func (a *Actions) RemoveSubmodule() *Actions {
 	a.context.t.Helper()
 	fixture.RemoveSubmodule()

test/e2e/fixture/app/consequences.go

@@ -42,6 +42,31 @@ func (c *Consequences) Expect(e Expectation) *Consequences {
 	return c
 }
 
+// ExpectConsistently will continuously evaluate a condition, and it must be true each time it is evaluated, otherwise the test is failed. The condition will be repeatedly evaluated until 'expirationDuration' is met, waiting 'waitDuration' after each success.
+func (c *Consequences) ExpectConsistently(e Expectation, waitDuration time.Duration, expirationDuration time.Duration) *Consequences {
+	// this invocation makes sure this func is not reported as the cause of the failure - we are a "test helper"
+	c.context.t.Helper()
+
+	expiration := time.Now().Add(expirationDuration)
+	for time.Now().Before(expiration) {
+		state, message := e(c)
+		switch state {
+		case succeeded:
+			log.Infof("expectation succeeded: %s", message)
+		case failed:
+			c.context.t.Fatalf("failed expectation: %s", message)
+			return c
+		}
+
+		// On condition success: wait, then retry
+		log.Infof("Expectation '%s' passes, repeating to ensure consistency", message)
+		time.Sleep(waitDuration)
+	}
+
+	// If the condition never failed before expiring, it is a pass.
+	return c
+}
+
 func (c *Consequences) And(block func(app *Application)) *Consequences {
 	c.context.t.Helper()
 	block(c.app())

test/e2e/fixture/applicationsets/utils/fixture.go

@@ -3,6 +3,7 @@ package utils
 import (
 	"context"
 	"encoding/json"
+	goerrors "errors"
 	"fmt"
 	"os"
 	"regexp"
@@ -268,7 +269,7 @@ func cleanUpNamespace(fixtureClient *E2EFixtureK8sClient, namespace string) erro
 		msg = err.Error()
 	}
 
-	return fmt.Errorf(msg)
+	return goerrors.New(msg)
 }
 
 // waitForSuccess waits for the condition to return a non-error value.