From 0538b2bea4716c9c75d15ee1e584b9ecde515a84 Mon Sep 17 00:00:00 2001 From: Atif Ali <56743004+aali309@users.noreply.github.com> Date: Tue, 18 Mar 2025 11:46:19 -0400 Subject: [PATCH 1/2] fix: handle annotated git tags correctly in repo server cache (#21771) Signed-off-by: Atif Ali --- test/e2e/fixture/app/actions.go | 12 +++ test/e2e/fixture/fixture.go | 19 ++++ test/e2e/git_test.go | 151 +++++++++++++++++++++++++++++++- util/git/client_test.go | 14 +++ util/git/git_test.go | 24 +++++ util/git/workaround.go | 6 ++ 6 files changed, 225 insertions(+), 1 deletion(-) diff --git a/test/e2e/fixture/app/actions.go b/test/e2e/fixture/app/actions.go index 7fc4489a620de..5377591f9e38c 100644 --- a/test/e2e/fixture/app/actions.go +++ b/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() diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go index 80dcd36c1bb9f..af8603b235aa4 100644 --- a/test/e2e/fixture/fixture.go +++ b/test/e2e/fixture/fixture.go @@ -861,6 +861,25 @@ func AddTag(name string) { } } +func AddTagWithForce(t *testing.T, name string) { + t.Helper() + prevGnuPGHome := os.Getenv("GNUPGHOME") + t.Setenv("GNUPGHOME", TmpDir+"/gpg") + defer t.Setenv("GNUPGHOME", prevGnuPGHome) + errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "tag", "-f", name)) + if IsRemote() { + errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master")) + } +} + +func AddAnnotatedTag(t *testing.T, name string, message string) { + t.Helper() + errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "tag", "-f", "-a", name, "-m", message)) + if IsRemote() { + errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master")) + } +} + // create the resource by creating using "kubectl apply", with bonus templating func Declarative(filename string, values interface{}) (string, error) { bytes, err := os.ReadFile(path.Join("testdata", filename)) diff --git a/test/e2e/git_test.go b/test/e2e/git_test.go index d231ab2034311..d38cfc83338b4 100644 --- a/test/e2e/git_test.go +++ b/test/e2e/git_test.go @@ -1,15 +1,22 @@ package e2e import ( + "context" "strings" "testing" + "time" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" - "github.com/argoproj/argo-cd/v2/test/e2e/fixture" + "github.com/stretchr/testify/require" . "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/v2/test/e2e/fixture" . "github.com/argoproj/argo-cd/v2/test/e2e/fixture/app" + "github.com/argoproj/argo-cd/v2/util/errors" ) func TestGitSemverResolutionNotUsingConstraint(t *testing.T) { @@ -51,3 +58,145 @@ func TestGitSemverResolutionUsingConstraint(t *testing.T) { Expect(SyncStatusIs(SyncStatusCodeSynced)). Expect(Pod(func(p v1.Pod) bool { return strings.HasPrefix(p.Name, "new-app") })) } + +func TestAnnotatedTagInStatusSyncRevision(t *testing.T) { + Given(t). + Path(guestbookPath). + When(). + // Create annotated tag name 'annotated-tag' + AddAnnotatedTag("annotated-tag", "my-generic-tag-message"). + // Create Application targeting annotated-tag, with automatedSync: true + CreateFromFile(func(app *Application) { + app.Spec.Source.TargetRevision = "annotated-tag" + app.Spec.SyncPolicy = &SyncPolicy{Automated: &SyncPolicyAutomated{Prune: true, SelfHeal: false}} + }). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + And(func(app *Application) { + annotatedTagIDOutput, err := fixture.Run(fixture.TmpDir+"/testdata.git", "git", "show-ref", "annotated-tag") + require.NoError(t, err) + require.NotEmpty(t, annotatedTagIDOutput) + // example command output: + // "569798c430515ffe170bdb23e3aafaf8ae24b9ff refs/tags/annotated-tag" + annotatedTagIDFields := strings.Fields(string(annotatedTagIDOutput)) + require.Len(t, annotatedTagIDFields, 2) + + targetCommitID, err := fixture.Run(fixture.TmpDir+"/testdata.git", "git", "rev-parse", "--verify", "annotated-tag^{commit}") + // example command output: + // "bcd35965e494273355265b9f0bf85075b6bc5163" + require.NoError(t, err) + require.NotEmpty(t, targetCommitID) + + require.NotEmpty(t, app.Status.Sync.Revision, "revision in sync status should be set by sync operation") + + require.NotEqual(t, app.Status.Sync.Revision, annotatedTagIDFields[0], "revision should not match the annotated tag id") + require.Equal(t, app.Status.Sync.Revision, strings.TrimSpace(string(targetCommitID)), "revision SHOULD match the target commit SHA") + }) +} + +// Test updates to K8s resources should not trigger a self-heal when self-heal is false. +func TestAutomatedSelfHealingAgainstAnnotatedTag(t *testing.T) { + Given(t). + Path(guestbookPath). + When(). + AddAnnotatedTag("annotated-tag", "my-generic-tag-message"). + // App should be auto-synced once created + CreateFromFile(func(app *Application) { + app.Spec.Source.TargetRevision = "annotated-tag" + app.Spec.SyncPolicy = &SyncPolicy{Automated: &SyncPolicyAutomated{Prune: true, SelfHeal: false}} + }). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + ExpectConsistently(SyncStatusIs(SyncStatusCodeSynced), WaitDuration, time.Second*10). + When(). + // Update the annotated tag to a new git commit, that has a new revisionHistoryLimit. + PatchFile("guestbook-ui-deployment.yaml", `[{"op": "replace", "path": "/spec/revisionHistoryLimit", "value": 10}]`). + AddAnnotatedTag("annotated-tag", "my-generic-tag-message"). + Refresh(RefreshTypeHard). + // The Application should update to the new annotated tag value within 10 seconds. + And(func() { + // Deployment revisionHistoryLimit should switch to 10 + timeoutErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + if err != nil { + return false, nil + } + + revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit + return revisionHistoryLimit != nil && *revisionHistoryLimit == 10, nil + }) + require.NoError(t, timeoutErr) + }). + // Update the Deployment to a different revisionHistoryLimit + And(func() { + errors.NewHandler(t).FailOnErr(fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(t.Context(), + "guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{})) + }). + // The revisionHistoryLimit should NOT be self-healed, because selfHealing: false. It should remain at 9. + And(func() { + // Wait up to 10 seconds to ensure that deployment revisionHistoryLimit does NOT should switch to 10, it should remain at 9. + waitErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + if err != nil { + return false, nil + } + + revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit + return revisionHistoryLimit != nil && *revisionHistoryLimit != 9, nil + }) + require.Error(t, waitErr, "A timeout error should occur, indicating that revisionHistoryLimit never changed from 9") + }) +} + +func TestAutomatedSelfHealingAgainstLightweightTag(t *testing.T) { + Given(t). + Path(guestbookPath). + When(). + AddTag("annotated-tag"). + // App should be auto-synced once created + CreateFromFile(func(app *Application) { + app.Spec.Source.TargetRevision = "annotated-tag" + app.Spec.SyncPolicy = &SyncPolicy{Automated: &SyncPolicyAutomated{Prune: true, SelfHeal: false}} + }). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + ExpectConsistently(SyncStatusIs(SyncStatusCodeSynced), WaitDuration, time.Second*10). + When(). + // Update the annotated tag to a new git commit, that has a new revisionHistoryLimit. + PatchFile("guestbook-ui-deployment.yaml", `[{"op": "replace", "path": "/spec/revisionHistoryLimit", "value": 10}]`). + AddTagWithForce("annotated-tag"). + Refresh(RefreshTypeHard). + // The Application should update to the new annotated tag value within 10 seconds. + And(func() { + // Deployment revisionHistoryLimit should switch to 10 + timeoutErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + if err != nil { + return false, nil + } + + revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit + return revisionHistoryLimit != nil && *revisionHistoryLimit == 10, nil + }) + require.NoError(t, timeoutErr) + }). + // Update the Deployment to a different revisionHistoryLimit + And(func() { + errors.NewHandler(t).FailOnErr(fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(t.Context(), + "guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{})) + }). + // The revisionHistoryLimit should NOT be self-healed, because selfHealing: false + And(func() { + // Wait up to 10 seconds to ensure that deployment revisionHistoryLimit does NOT should switch to 10, it should remain at 9. + waitErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + if err != nil { + return false, nil + } + + revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit + return revisionHistoryLimit != nil && *revisionHistoryLimit != 9, nil + }) + require.Error(t, waitErr, "A timeout error should occur, indicating that revisionHistoryLimit never changed from 9") + }) +} diff --git a/util/git/client_test.go b/util/git/client_test.go index 2688b44792cea..d2526d427bb7f 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "testing" + "github.com/go-git/go-git/v5/plumbing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -118,6 +119,19 @@ func Test_IsAnnotatedTag(t *testing.T) { assert.False(t, atag) } +func Test_resolveTagReference(t *testing.T) { + // Setup + commitHash := plumbing.NewHash("0123456789abcdef0123456789abcdef01234567") + tagRef := plumbing.NewReferenceFromStrings("refs/tags/v1.0.0", "sometaghash") + + // Test single function + resolvedRef := plumbing.NewHashReference(tagRef.Name(), commitHash) + + // Verify + assert.Equal(t, commitHash, resolvedRef.Hash()) + assert.Equal(t, tagRef.Name(), resolvedRef.Name()) +} + func Test_ChangedFiles(t *testing.T) { tempDir := t.TempDir() diff --git a/util/git/git_test.go b/util/git/git_test.go index 77c57cd61122e..7ef2ecde46e11 100644 --- a/util/git/git_test.go +++ b/util/git/git_test.go @@ -501,3 +501,27 @@ func TestLsFiles(t *testing.T) { require.NoError(t, err) assert.Equal(t, nilResult, lsResult) } + +func TestAnnotatedTagHandling(t *testing.T) { + dir := t.TempDir() + + client, err := NewClientExt("https://github.com/argoproj/argo-cd.git", dir, NopCreds{}, false, false, "", "") + require.NoError(t, err) + + err = client.Init() + require.NoError(t, err) + + // Test annotated tag resolution + commitSHA, err := client.LsRemote("v1.0.0") // Known annotated tag + require.NoError(t, err) + + // Verify we get commit SHA, not tag SHA + assert.True(t, IsCommitSHA(commitSHA)) + + // Test tag reference handling + refs, err := client.LsRefs() + require.NoError(t, err) + + // Verify tag exists in the list and points to a valid commit SHA + assert.Contains(t, refs.Tags, "v1.0.0", "Tag v1.0.0 should exist in refs") +} diff --git a/util/git/workaround.go b/util/git/workaround.go index 47636125cf349..e6806ce3d561c 100644 --- a/util/git/workaround.go +++ b/util/git/workaround.go @@ -85,6 +85,12 @@ func listRemote(r *git.Remote, o *git.ListOptions, insecure bool, creds Creds, p var resultRefs []*plumbing.Reference _ = refs.ForEach(func(ref *plumbing.Reference) error { + if ref.Name().IsTag() { + if peeled, ok := ar.Peeled[ref.Name().String()]; ok { + resultRefs = append(resultRefs, plumbing.NewHashReference(ref.Name(), peeled)) + return nil + } + } resultRefs = append(resultRefs, ref) return nil }) From 6631de7e28c90f00437cff20778791573d2aed4b Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Mon, 24 Mar 2025 17:02:14 -0400 Subject: [PATCH 2/2] resolve linter issues Signed-off-by: Atif Ali --- .../controllers/applicationset_controller.go | 2 +- applicationset/generators/git.go | 2 +- applicationset/generators/plugin_test.go | 2 +- applicationset/utils/createOrUpdate_test.go | 2 +- applicationset/utils/utils.go | 2 +- .../commands/admin/project_allowlist_test.go | 2 +- cmd/argocd/commands/app.go | 2 +- cmd/argocd/commands/common_test.go | 12 +++---- cmd/util/cluster_test.go | 5 ++- controller/appcontroller_test.go | 2 +- controller/metrics/metrics_test.go | 2 +- controller/state.go | 2 +- pkg/apis/application/v1alpha1/types.go | 2 +- pkg/apis/application/v1alpha1/types_test.go | 3 +- reposerver/cache/cache_test.go | 3 +- reposerver/repository/repository_test.go | 2 +- server/application/application_test.go | 2 +- server/logout/logout_test.go | 2 +- test/e2e/accounts_test.go | 9 +++-- test/e2e/app_management_test.go | 5 ++- test/e2e/cluster_test.go | 5 ++- test/e2e/fixture/app/consequences.go | 25 ++++++++++++++ .../fixture/applicationsets/utils/fixture.go | 3 +- test/e2e/fixture/cluster/actions.go | 3 +- test/e2e/fixture/fixture.go | 12 ++++--- test/e2e/fixture/project/actions.go | 2 +- test/e2e/git_test.go | 33 +++++++++++-------- test/e2e/project_management_test.go | 10 +++--- test/e2e/scoped_repository_test.go | 9 +++-- util/argo/argo.go | 2 +- util/exec/exec_test.go | 4 +-- util/git/git_test.go | 2 +- util/kube/portforwarder.go | 3 +- util/oidc/oidc_test.go | 2 +- util/settings/accounts_test.go | 4 +-- util/test/testutil.go | 2 +- 36 files changed, 107 insertions(+), 79 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index c7585e8153825..98b2807f372ab 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/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() } } diff --git a/applicationset/generators/git.go b/applicationset/generators/git.go index 312924bbb415c..74fe02044b473 100644 --- a/applicationset/generators/git.go +++ b/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 diff --git a/applicationset/generators/plugin_test.go b/applicationset/generators/plugin_test.go index 55ebcfd5c7820..0ade708ee569b 100644 --- a/applicationset/generators/plugin_test.go +++ b/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)) } }) } diff --git a/applicationset/utils/createOrUpdate_test.go b/applicationset/utils/createOrUpdate_test.go index 2dc5945d2d2cc..de64541337178 100644 --- a/applicationset/utils/createOrUpdate_test.go +++ b/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)) }) } } diff --git a/applicationset/utils/utils.go b/applicationset/utils/utils.go index dfcc11cbdd35a..10aac884107e5 100644 --- a/applicationset/utils/utils.go +++ b/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"} } diff --git a/cmd/argocd/commands/admin/project_allowlist_test.go b/cmd/argocd/commands/admin/project_allowlist_test.go index eeec46b9be231..7c22fd1c0ee75 100644 --- a/cmd/argocd/commands/admin/project_allowlist_test.go +++ b/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) } diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 464a7cfc308e3..0a00d69df1f4a 100644 --- a/cmd/argocd/commands/app.go +++ b/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) diff --git a/cmd/argocd/commands/common_test.go b/cmd/argocd/commands/common_test.go index 24ab6ebcf7fd9..8a3a2446a6989 100644 --- a/cmd/argocd/commands/common_test.go +++ b/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) diff --git a/cmd/util/cluster_test.go b/cmd/util/cluster_test.go index f30a4aed51abd..24b46765ca686 100644 --- a/cmd/util/cluster_test.go +++ b/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) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index ee92360aace26..94f8272309109 100644 --- a/controller/appcontroller_test.go +++ b/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) { diff --git a/controller/metrics/metrics_test.go b/controller/metrics/metrics_test.go index 28422be55f653..e0a2185b9a5c3 100644 --- a/controller/metrics/metrics_test.go +++ b/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) } } diff --git a/controller/state.go b/controller/state.go index 4e9b383a2e0cb..76b59372c1c15 100644 --- a/controller/state.go +++ b/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 } diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index a5d52f3d3f75a..d5dacb1e0a474 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/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 { diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 22ef37badb24f..825a0295cfa8a 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/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) { diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index dd89a10b7b90c..07439610b5b48 100644 --- a/reposerver/cache/cache_test.go +++ b/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) } } diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 8b35c754e69ea..7087405820c35 100644 --- a/reposerver/repository/repository_test.go +++ b/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, diff --git a/server/application/application_test.go b/server/application/application_test.go index b8c80d91d57e6..f2b0d4e324674 100644 --- a/server/application/application_test.go +++ b/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)) } } diff --git a/server/logout/logout_test.go b/server/logout/logout_test.go index 78a735c528beb..08c9206463b9d 100644 --- a/server/logout/logout_test.go +++ b/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 { diff --git a/test/e2e/accounts_test.go b/test/e2e/accounts_test.go index c238aacb728b5..7f3f056a952c9 100644 --- a/test/e2e/accounts_test.go +++ b/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") }) } diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index a500b229b56ea..2e1ceda009586 100644 --- a/test/e2e/app_management_test.go +++ b/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() diff --git a/test/e2e/cluster_test.go b/test/e2e/cluster_test.go index 81399c0a01022..d09df7d0cc812 100644 --- a/test/e2e/cluster_test.go +++ b/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") }) } diff --git a/test/e2e/fixture/app/consequences.go b/test/e2e/fixture/app/consequences.go index 9ee99fec6ca6d..ff64dee0de4b8 100644 --- a/test/e2e/fixture/app/consequences.go +++ b/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()) diff --git a/test/e2e/fixture/applicationsets/utils/fixture.go b/test/e2e/fixture/applicationsets/utils/fixture.go index e447f9d455433..1280570e4c190 100644 --- a/test/e2e/fixture/applicationsets/utils/fixture.go +++ b/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. diff --git a/test/e2e/fixture/cluster/actions.go b/test/e2e/fixture/cluster/actions.go index bd8fb33184379..ac114bad0cdf9 100644 --- a/test/e2e/fixture/cluster/actions.go +++ b/test/e2e/fixture/cluster/actions.go @@ -3,7 +3,6 @@ package cluster import ( "context" "errors" - "fmt" "log" "strings" @@ -60,7 +59,7 @@ func (a *Actions) Create(args ...string) *Actions { }) if err != nil { if !a.ignoreErrors { - log.Fatalf(fmt.Sprintf("Failed to upsert cluster %v", err.Error())) + log.Fatalf("Failed to upsert cluster %v", err.Error()) } a.lastError = errors.New(err.Error()) } diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go index af8603b235aa4..418492e99c4be 100644 --- a/test/e2e/fixture/fixture.go +++ b/test/e2e/fixture/fixture.go @@ -866,17 +866,21 @@ func AddTagWithForce(t *testing.T, name string) { prevGnuPGHome := os.Getenv("GNUPGHOME") t.Setenv("GNUPGHOME", TmpDir+"/gpg") defer t.Setenv("GNUPGHOME", prevGnuPGHome) - errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "tag", "-f", name)) + _, err := Run(repoDirectory(), "git", "tag", "-f", name) + CheckError(err) if IsRemote() { - errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master")) + _, err = Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master") + CheckError(err) } } func AddAnnotatedTag(t *testing.T, name string, message string) { t.Helper() - errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "tag", "-f", "-a", name, "-m", message)) + _, err := Run(repoDirectory(), "git", "tag", "-f", "-a", name, "-m", message) + CheckError(err) if IsRemote() { - errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master")) + _, err = Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master") + CheckError(err) } } diff --git a/test/e2e/fixture/project/actions.go b/test/e2e/fixture/project/actions.go index 61a2ad90615fb..f4358366c847d 100644 --- a/test/e2e/fixture/project/actions.go +++ b/test/e2e/fixture/project/actions.go @@ -102,6 +102,6 @@ func (a *Actions) runCli(args ...string) { a.context.t.Helper() a.lastOutput, a.lastError = fixture.RunCli(args...) if !a.ignoreErrors { - require.Empty(a.context.t, a.lastError) + require.NoError(a.context.t, a.lastError) } } diff --git a/test/e2e/git_test.go b/test/e2e/git_test.go index d38cfc83338b4..a2117e19222db 100644 --- a/test/e2e/git_test.go +++ b/test/e2e/git_test.go @@ -16,7 +16,10 @@ import ( . "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/test/e2e/fixture" . "github.com/argoproj/argo-cd/v2/test/e2e/fixture/app" - "github.com/argoproj/argo-cd/v2/util/errors" +) + +const ( + WaitDuration = 10 * time.Second ) func TestGitSemverResolutionNotUsingConstraint(t *testing.T) { @@ -116,8 +119,8 @@ func TestAutomatedSelfHealingAgainstAnnotatedTag(t *testing.T) { // The Application should update to the new annotated tag value within 10 seconds. And(func() { // Deployment revisionHistoryLimit should switch to 10 - timeoutErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { - deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + timeoutErr := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(context.Background(), "guestbook-ui", metav1.GetOptions{}) if err != nil { return false, nil } @@ -129,14 +132,15 @@ func TestAutomatedSelfHealingAgainstAnnotatedTag(t *testing.T) { }). // Update the Deployment to a different revisionHistoryLimit And(func() { - errors.NewHandler(t).FailOnErr(fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(t.Context(), - "guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{})) + _, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(context.Background(), + "guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{}) + require.NoError(t, err) }). // The revisionHistoryLimit should NOT be self-healed, because selfHealing: false. It should remain at 9. And(func() { // Wait up to 10 seconds to ensure that deployment revisionHistoryLimit does NOT should switch to 10, it should remain at 9. - waitErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { - deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + waitErr := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(context.Background(), "guestbook-ui", metav1.GetOptions{}) if err != nil { return false, nil } @@ -160,7 +164,7 @@ func TestAutomatedSelfHealingAgainstLightweightTag(t *testing.T) { }). Then(). Expect(SyncStatusIs(SyncStatusCodeSynced)). - ExpectConsistently(SyncStatusIs(SyncStatusCodeSynced), WaitDuration, time.Second*10). + ExpectConsistently(SyncStatusIs(SyncStatusCodeSynced), 10*time.Second, 10*time.Second). When(). // Update the annotated tag to a new git commit, that has a new revisionHistoryLimit. PatchFile("guestbook-ui-deployment.yaml", `[{"op": "replace", "path": "/spec/revisionHistoryLimit", "value": 10}]`). @@ -169,8 +173,8 @@ func TestAutomatedSelfHealingAgainstLightweightTag(t *testing.T) { // The Application should update to the new annotated tag value within 10 seconds. And(func() { // Deployment revisionHistoryLimit should switch to 10 - timeoutErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { - deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + timeoutErr := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(context.Background(), "guestbook-ui", metav1.GetOptions{}) if err != nil { return false, nil } @@ -182,14 +186,15 @@ func TestAutomatedSelfHealingAgainstLightweightTag(t *testing.T) { }). // Update the Deployment to a different revisionHistoryLimit And(func() { - errors.NewHandler(t).FailOnErr(fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(t.Context(), - "guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{})) + _, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(context.Background(), + "guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{}) + require.NoError(t, err) }). // The revisionHistoryLimit should NOT be self-healed, because selfHealing: false And(func() { // Wait up to 10 seconds to ensure that deployment revisionHistoryLimit does NOT should switch to 10, it should remain at 9. - waitErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { - deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{}) + waitErr := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) { + deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(context.Background(), "guestbook-ui", metav1.GetOptions{}) if err != nil { return false, nil } diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index 5e8b42a94442d..3983cedcd75c8 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -164,21 +164,21 @@ func TestAddProjectDestination(t *testing.T) { "test1", ) require.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "already defined")) + assert.Contains(t, err.Error(), "already defined") _, err = fixture.RunCli("proj", "add-destination", projectName, "!*", "test1", ) require.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "server has an invalid format, '!*'")) + assert.Contains(t, err.Error(), "server has an invalid format, '!*'") _, err = fixture.RunCli("proj", "add-destination", projectName, "https://192.168.99.100:8443", "!*", ) require.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "namespace has an invalid format, '!*'")) + assert.Contains(t, err.Error(), "namespace has an invalid format, '!*'") proj, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Get(context.Background(), projectName, metav1.GetOptions{}) require.NoError(t, err) @@ -383,7 +383,7 @@ func TestUseJWTToken(t *testing.T) { roleGetResult, err = fixture.RunCli("proj", "role", "get", projectName, roleName) require.NoError(t, err) - assert.True(t, strings.Contains(roleGetResult, strconv.FormatInt(newProj.Status.JWTTokensByRole[roleName].Items[0].IssuedAt, 10))) + assert.Contains(t, roleGetResult, strconv.FormatInt(newProj.Status.JWTTokensByRole[roleName].Items[0].IssuedAt, 10)) _, err = fixture.RunCli("proj", "role", "delete-token", projectName, roleName, strconv.FormatInt(newProj.Status.JWTTokensByRole[roleName].Items[0].IssuedAt, 10)) require.NoError(t, err) @@ -420,7 +420,7 @@ func TestAddOrphanedIgnore(t *testing.T) { "name", ) require.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "already defined")) + assert.Contains(t, err.Error(), "already defined") proj, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Get(context.Background(), projectName, metav1.GetOptions{}) require.NoError(t, err) diff --git a/test/e2e/scoped_repository_test.go b/test/e2e/scoped_repository_test.go index 1a459af06736d..d1da08b0a434d 100644 --- a/test/e2e/scoped_repository_test.go +++ b/test/e2e/scoped_repository_test.go @@ -1,7 +1,6 @@ package e2e import ( - "strings" "testing" "github.com/argoproj/argo-cd/v2/test/e2e/fixture" @@ -57,7 +56,7 @@ func TestCreateRepositoryNonAdminUserPermissionDenied(t *testing.T) { Create(). Then(). AndCLIOutput(func(output string, err error) { - assert.True(t, strings.Contains(err.Error(), "PermissionDenied desc = permission denied: repositories, create")) + assert.Contains(t, err.Error(), "PermissionDenied desc = permission denied: repositories, create") }) } @@ -84,7 +83,7 @@ func TestCreateRepositoryNonAdminUserWithWrongProject(t *testing.T) { Create(). Then(). AndCLIOutput(func(output string, err error) { - assert.True(t, strings.Contains(err.Error(), "PermissionDenied desc = permission denied: repositories, create")) + assert.Contains(t, err.Error(), "PermissionDenied desc = permission denied: repositories, create") }) } @@ -127,7 +126,7 @@ func TestDeleteRepositoryRbacAllowed(t *testing.T) { Delete(). Then(). AndCLIOutput(func(output string, err error) { - assert.True(t, strings.Contains(output, "Repository 'https://github.com/argoproj/argo-cd.git' removed")) + assert.Contains(t, output, "Repository 'https://github.com/argoproj/argo-cd.git' removed") }) } @@ -171,7 +170,7 @@ func TestDeleteRepositoryRbacDenied(t *testing.T) { Delete(). Then(). AndCLIOutput(func(output string, err error) { - assert.True(t, strings.Contains(err.Error(), "PermissionDenied desc = permission denied: repositories, delete")) + assert.Contains(t, err.Error(), "PermissionDenied desc = permission denied: repositories, delete") }) } diff --git a/util/argo/argo.go b/util/argo/argo.go index f90b4f4dbed23..bf98fbd963a6a 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -866,7 +866,7 @@ func NormalizeApplicationSpec(spec *argoappv1.ApplicationSpec) *argoappv1.Applic if spec.SyncPolicy.IsZero() { spec.SyncPolicy = nil } - if spec.Sources != nil && len(spec.Sources) > 0 { + if len(spec.Sources) > 0 { for _, source := range spec.Sources { NormalizeSource(&source) } diff --git a/util/exec/exec_test.go b/util/exec/exec_test.go index d7dd75c61a1bd..e30fdff4c7c24 100644 --- a/util/exec/exec_test.go +++ b/util/exec/exec_test.go @@ -32,13 +32,13 @@ func TestRun(t *testing.T) { func TestHideUsernamePassword(t *testing.T) { _, err := RunWithRedactor(exec.Command("helm registry login https://charts.bitnami.com/bitnami", "--username", "foo", "--password", "bar"), nil) - assert.NotEmpty(t, err) + require.Error(t, err) redactor := func(text string) string { return regexp.MustCompile("(--username|--password) [^ ]*").ReplaceAllString(text, "$1 ******") } _, err = RunWithRedactor(exec.Command("helm registry login https://charts.bitnami.com/bitnami", "--username", "foo", "--password", "bar"), redactor) - assert.NotEmpty(t, err) + require.Error(t, err) } func TestRunWithExecRunOpts(t *testing.T) { diff --git a/util/git/git_test.go b/util/git/git_test.go index 7ef2ecde46e11..22ca5fba74d37 100644 --- a/util/git/git_test.go +++ b/util/git/git_test.go @@ -505,7 +505,7 @@ func TestLsFiles(t *testing.T) { func TestAnnotatedTagHandling(t *testing.T) { dir := t.TempDir() - client, err := NewClientExt("https://github.com/argoproj/argo-cd.git", dir, NopCreds{}, false, false, "", "") + client, err := NewClientExt("https://github.com/argoproj/argo-cd.git", dir, NopCreds{}, false, false, "") require.NoError(t, err) err = client.Init() diff --git a/util/kube/portforwarder.go b/util/kube/portforwarder.go index f08f783208289..66b3cb4733447 100644 --- a/util/kube/portforwarder.go +++ b/util/kube/portforwarder.go @@ -3,6 +3,7 @@ package kube import ( "bytes" "context" + "errors" "fmt" "net" "net/http" @@ -100,7 +101,7 @@ func PortForward(targetPort int, namespace string, overrides *clientcmd.ConfigOv case <-readyChan: } if len(errOut.String()) != 0 { - return -1, fmt.Errorf(errOut.String()) + return -1, errors.New(errOut.String()) } return port, nil } diff --git a/util/oidc/oidc_test.go b/util/oidc/oidc_test.go index 12a715f6a3e9a..ba9a187c4ba93 100644 --- a/util/oidc/oidc_test.go +++ b/util/oidc/oidc_test.go @@ -79,7 +79,7 @@ func TestIDTokenClaims(t *testing.T) { values, err := url.ParseQuery(authCodeURL.RawQuery) require.NoError(t, err) - assert.Equal(t, "{\"id_token\":{\"groups\":{\"essential\":true}}}", values.Get("claims")) + assert.JSONEq(t, "{\"id_token\":{\"groups\":{\"essential\":true}}}", values.Get("claims")) } type fakeProvider struct{} diff --git a/util/settings/accounts_test.go b/util/settings/accounts_test.go index 1415ce226de3d..2ab477ba6a963 100644 --- a/util/settings/accounts_test.go +++ b/util/settings/accounts_test.go @@ -164,7 +164,7 @@ func TestAddAccount_AccountAdded(t *testing.T) { assert.Equal(t, "hash", string(secret.Data["accounts.test.password"])) assert.Equal(t, mTime.Format(time.RFC3339), string(secret.Data["accounts.test.passwordMtime"])) - assert.Equal(t, `[{"id":"123","iat":0}]`, string(secret.Data["accounts.test.tokens"])) + assert.JSONEq(t, `[{"id":"123","iat":0}]`, string(secret.Data["accounts.test.tokens"])) } func TestAddAccount_AlreadyExists(t *testing.T) { @@ -204,7 +204,7 @@ func TestUpdateAccount_SuccessfullyUpdated(t *testing.T) { assert.Equal(t, "hash", string(secret.Data["accounts.test.password"])) assert.Equal(t, mTime.Format(time.RFC3339), string(secret.Data["accounts.test.passwordMtime"])) - assert.Equal(t, `[{"id":"123","iat":0}]`, string(secret.Data["accounts.test.tokens"])) + assert.JSONEq(t, `[{"id":"123","iat":0}]`, string(secret.Data["accounts.test.tokens"])) } func TestUpdateAccount_UpdateAdminPassword(t *testing.T) { diff --git a/util/test/testutil.go b/util/test/testutil.go index bb6c43313358c..4974325b43933 100644 --- a/util/test/testutil.go +++ b/util/test/testutil.go @@ -191,7 +191,7 @@ func oidcMockHandler(t *testing.T, url string) func(http.ResponseWriter, *http.R } out, err := json.Marshal(jwks) require.NoError(t, err) - _, err = io.WriteString(w, string(out)) + _, err = io.Writer.Write(w, out) require.NoError(t, err) default: w.WriteHeader(http.StatusNotFound)