Merge pull request #1257 from fluxcd/tidy-nits

Address miscellaneous issues throughout code base

go.mod

@@ -68,7 +68,7 @@ require (
 	k8s.io/api v0.27.4
 	k8s.io/apimachinery v0.27.4
 	k8s.io/client-go v0.27.4
-	k8s.io/utils v0.0.0-20230505201702-9f6742963106
+	k8s.io/utils v0.0.0-20230726121419-3b25d923346b
 	sigs.k8s.io/cli-utils v0.35.0
 	sigs.k8s.io/controller-runtime v0.15.1
 	sigs.k8s.io/yaml v1.3.0

go.sum

@@ -1808,8 +1808,8 @@ k8s.io/kube-openapi v0.0.0-20230515203736-54b630e78af5 h1:azYPdzztXxPSa8wb+hksEK
 k8s.io/kube-openapi v0.0.0-20230515203736-54b630e78af5/go.mod h1:kzo02I3kQ4BTtEfVLaPbjvCkX97YqGve33wzlb3fofQ=
 k8s.io/kubectl v0.27.3 h1:HyC4o+8rCYheGDWrkcOQHGwDmyLKR5bxXFgpvF82BOw=
 k8s.io/kubectl v0.27.3/go.mod h1:g9OQNCC2zxT+LT3FS09ZYqnDhlvsKAfFq76oyarBcq4=
-k8s.io/utils v0.0.0-20230505201702-9f6742963106 h1:EObNQ3TW2D+WptiYXlApGNLVy0zm/JIBVY9i+M4wpAU=
-k8s.io/utils v0.0.0-20230505201702-9f6742963106/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
+k8s.io/utils v0.0.0-20230726121419-3b25d923346b h1:sgn3ZU783SCgtaSJjpcVVlRqd6GSnlTLKgpAAttJvpI=
+k8s.io/utils v0.0.0-20230726121419-3b25d923346b/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 oras.land/oras-go v1.2.3 h1:v8PJl+gEAntI1pJ/LCrDgsuk+1PKVavVEPsYIHFE5uY=
 oras.land/oras-go v1.2.3/go.mod h1:M/uaPdYklze0Vf3AakfarnpoEckvw0ESbRdN8Z1vdJg=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=

internal/controller/bucket_controller_fetch_test.go

@@ -41,7 +41,7 @@ type mockBucketClient struct {
 	objects    map[string]mockBucketObject
 }
 
-var mockNotFound = fmt.Errorf("not found")
+var errMockNotFound = fmt.Errorf("not found")
 
 func (m mockBucketClient) BucketExists(_ context.Context, name string) (bool, error) {
 	return name == m.bucketName, nil
@@ -57,7 +57,7 @@ func (m mockBucketClient) FGetObject(_ context.Context, bucket, obj, path string
 	}
 	object, ok := m.objects[obj]
 	if !ok {
-		return "", mockNotFound
+		return "", errMockNotFound
 	}
 	if err := os.WriteFile(path, []byte(object.data), os.FileMode(0660)); err != nil {
 		return "", err
@@ -66,7 +66,7 @@ func (m mockBucketClient) FGetObject(_ context.Context, bucket, obj, path string
 }
 
 func (m mockBucketClient) ObjectIsNotFound(e error) bool {
-	return e == mockNotFound
+	return e == errMockNotFound
 }
 
 func (m mockBucketClient) VisitObjects(_ context.Context, _ string, f func(key, etag string) error) error {
@@ -78,9 +78,7 @@ func (m mockBucketClient) VisitObjects(_ context.Context, _ string, f func(key,
 	return nil
 }
 
-func (m mockBucketClient) Close(_ context.Context) {
-	return
-}
+func (m mockBucketClient) Close(_ context.Context) {}
 
 func (m *mockBucketClient) addObject(key string, object mockBucketObject) {
 	if m.objects == nil {

internal/controller/bucket_controller_test.go

@@ -258,7 +258,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 			name: "notices missing artifact in storage",
 			beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error {
 				obj.Status.Artifact = &sourcev1.Artifact{
-					Path:     fmt.Sprintf("/reconcile-storage/invalid.txt"),
+					Path:     "/reconcile-storage/invalid.txt",
 					Revision: "d",
 				}
 				storage.SetArtifactURL(obj.Status.Artifact)
@@ -339,7 +339,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 			name: "updates hostname on diff from current",
 			beforeFunc: func(obj *bucketv1.Bucket, storage *Storage) error {
 				obj.Status.Artifact = &sourcev1.Artifact{
-					Path:     fmt.Sprintf("/reconcile-storage/hostname.txt"),
+					Path:     "/reconcile-storage/hostname.txt",
 					Revision: "f",
 					Digest:   "sha256:3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
 					URL:      "http://outdated.com/reconcile-storage/hostname.txt",
@@ -1211,8 +1211,8 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) {
 				// path.
 				t.Expect(os.RemoveAll(dir)).ToNot(HaveOccurred())
 				f, err := os.Create(dir)
-				defer f.Close()
 				t.Expect(err).ToNot(HaveOccurred())
+				t.Expect(f.Close()).ToNot(HaveOccurred())
 				conditions.MarkReconciling(obj, meta.ProgressingReason, "foo")
 				conditions.MarkUnknown(obj, meta.ReadyCondition, "foo", "bar")
 			},
@@ -1293,6 +1293,7 @@ func TestBucketReconciler_statusConditions(t *testing.T) {
 		name             string
 		beforeFunc       func(obj *bucketv1.Bucket)
 		assertConditions []metav1.Condition
+		wantErr          bool
 	}{
 		{
 			name: "positive conditions only",
@@ -1317,6 +1318,7 @@ func TestBucketReconciler_statusConditions(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory"),
 				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"),
 			},
+			wantErr: true,
 		},
 		{
 			name: "mixed positive and negative conditions",
@@ -1329,6 +1331,7 @@ func TestBucketReconciler_statusConditions(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"),
 				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"),
 			},
+			wantErr: true,
 		},
 	}
 
@@ -1360,21 +1363,18 @@ func TestBucketReconciler_statusConditions(t *testing.T) {
 			}
 
 			ctx := context.TODO()
-			recResult := sreconcile.ResultSuccess
-			var retErr error
-
 			summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher)
 			summarizeOpts := []summarize.Option{
 				summarize.WithConditions(bucketReadyCondition),
-				summarize.WithReconcileResult(recResult),
-				summarize.WithReconcileError(retErr),
+				summarize.WithReconcileResult(sreconcile.ResultSuccess),
 				summarize.WithIgnoreNotFound(),
 				summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{
 					RequeueAfter: jitter.JitteredIntervalDuration(obj.GetRequeueAfter()),
 				}),
 				summarize.WithPatchFieldOwner("source-controller"),
 			}
-			_, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
+			_, err := summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
+			g.Expect(err != nil).To(Equal(tt.wantErr))
 
 			key := client.ObjectKeyFromObject(obj)
 			g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred())

internal/controller/gitrepository_controller.go

@@ -33,7 +33,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kuberecorder "k8s.io/client-go/tools/record"
-	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -366,7 +366,7 @@ func (r *GitRepositoryReconciler) shouldNotify(oldObj, newObj *sourcev1.GitRepos
 	if resErr != nil && res == sreconcile.ResultEmpty && newObj.Status.Artifact != nil {
 		// Convert to Generic error and check for ignore.
 		if ge, ok := resErr.(*serror.Generic); ok {
-			return ge.Ignore == true
+			return ge.Ignore
 		}
 	}
 	return false
@@ -521,7 +521,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
 
 	// Observe if the artifacts still match the previous included ones
 	if artifacts.Diff(obj.Status.IncludedArtifacts) {
-		message := fmt.Sprintf("included artifacts differ from last observed includes")
+		message := "included artifacts differ from last observed includes"
 		if obj.Status.IncludedArtifacts != nil {
 			conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message)
 		}
@@ -1106,7 +1106,7 @@ func (r *GitRepositoryReconciler) eventLogf(ctx context.Context, obj runtime.Obj
 // changed and requires rebuilding the artifact. Rebuilding the artifact is also
 // required if the object needs to be (re)verified.
 func gitContentConfigChanged(obj *sourcev1.GitRepository, includes *artifactSet) bool {
-	if !pointer.StringEqual(obj.Spec.Ignore, obj.Status.ObservedIgnore) {
+	if !ptr.Equal(obj.Spec.Ignore, obj.Status.ObservedIgnore) {
 		return true
 	}
 	if obj.Spec.RecurseSubmodules != obj.Status.ObservedRecurseSubmodules {

internal/controller/gitrepository_controller_test.go

@@ -40,7 +40,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
-	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 	kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -827,7 +827,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 			},
 			beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) {
 				// Set new ignore value.
-				obj.Spec.Ignore = pointer.StringPtr("foo")
+				obj.Spec.Ignore = ptr.To("foo")
 				// Add existing artifact on the object and storage.
 				obj.Status = sourcev1.GitRepositoryStatus{
 					Artifact: &sourcev1.Artifact{
@@ -1001,7 +1001,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) {
 			dir:  "testdata/git/repository",
 			beforeFunc: func(obj *sourcev1.GitRepository) {
 				obj.Spec.Interval = metav1.Duration{Duration: interval}
-				obj.Spec.Ignore = pointer.StringPtr("!**.txt\n")
+				obj.Spec.Ignore = ptr.To("!**.txt\n")
 			},
 			afterFunc: func(t *WithT, obj *sourcev1.GitRepository) {
 				t.Expect(obj.GetArtifact()).ToNot(BeNil())
@@ -1849,6 +1849,41 @@ func TestGitRepositoryReconciler_verifySignature(t *testing.T) {
 				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidCommitSignature", "signature verification of commit 'shasum' failed: unable to verify Git commit: unable to verify payload with any of the given key rings"),
 			},
 		},
+		{
+			name: "Invalid tag signature with mode=tag makes SourceVerifiedCondition=False",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "existing",
+				},
+				Data: map[string][]byte{
+					"foo": []byte(armoredKeyRingFixture),
+				},
+			},
+			commit: git.Commit{
+				ReferencingTag: &git.Tag{
+					Name:      "v0.1.0",
+					Hash:      []byte("shasum"),
+					Encoded:   []byte(malformedEncodedTagFixture),
+					Signature: signatureTagFixture,
+				},
+			},
+			beforeFunc: func(obj *sourcev1.GitRepository) {
+				obj.Spec.Reference = &sourcev1.GitRepositoryRef{
+					Tag: "v0.1.0",
+				}
+				obj.Spec.Interval = metav1.Duration{Duration: interval}
+				obj.Spec.Verification = &sourcev1.GitRepositoryVerification{
+					Mode: sourcev1.ModeGitTag,
+					SecretRef: meta.LocalObjectReference{
+						Name: "existing",
+					},
+				}
+			},
+			wantErr: true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidTagSignature", "signature verification of tag 'v0.1.0@shasum' failed: unable to verify Git tag: unable to verify payload with any of the given key rings"),
+			},
+		},
 		{
 			name: "Invalid PGP key makes SourceVerifiedCondition=False and returns error",
 			secret: &corev1.Secret{
@@ -2328,6 +2363,7 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) {
 		name             string
 		beforeFunc       func(obj *sourcev1.GitRepository)
 		assertConditions []metav1.Condition
+		wantErr          bool
 	}{
 		{
 			name: "multiple positive conditions",
@@ -2356,6 +2392,7 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.DirCreationFailedReason, "failed to create directory"),
 				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"),
 			},
+			wantErr: true,
 		},
 		{
 			name: "mixed positive and negative conditions",
@@ -2368,6 +2405,7 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"),
 				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"),
 			},
+			wantErr: true,
 		},
 	}
 
@@ -2400,22 +2438,19 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) {
 			}
 
 			ctx := context.TODO()
-			recResult := sreconcile.ResultSuccess
-			var retErr error
-
 			summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher)
 			summarizeOpts := []summarize.Option{
 				summarize.WithConditions(gitRepositoryReadyCondition),
 				summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition),
-				summarize.WithReconcileResult(recResult),
-				summarize.WithReconcileError(retErr),
+				summarize.WithReconcileResult(sreconcile.ResultSuccess),
 				summarize.WithIgnoreNotFound(),
 				summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{
 					RequeueAfter: jitter.JitteredIntervalDuration(obj.GetRequeueAfter()),
 				}),
 				summarize.WithPatchFieldOwner("source-controller"),
 			}
-			_, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
+			_, err := summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
+			g.Expect(err != nil).To(Equal(tt.wantErr))
 
 			key := client.ObjectKeyFromObject(obj)
 			g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred())
@@ -2833,15 +2868,15 @@ func TestGitContentConfigChanged(t *testing.T) {
 		{
 			name: "unobserved ignore",
 			obj: sourcev1.GitRepository{
-				Spec: sourcev1.GitRepositorySpec{Ignore: pointer.String("foo")},
+				Spec: sourcev1.GitRepositorySpec{Ignore: ptr.To("foo")},
 			},
 			want: true,
 		},
 		{
 			name: "observed ignore",
 			obj: sourcev1.GitRepository{
-				Spec:   sourcev1.GitRepositorySpec{Ignore: pointer.String("foo")},
-				Status: sourcev1.GitRepositoryStatus{ObservedIgnore: pointer.String("foo")},
+				Spec:   sourcev1.GitRepositorySpec{Ignore: ptr.To("foo")},
+				Status: sourcev1.GitRepositoryStatus{ObservedIgnore: ptr.To("foo")},
 			},
 			want: false,
 		},

internal/controller/helmchart_controller.go

@@ -1129,9 +1129,9 @@ func (r *HelmChartReconciler) requestsForHelmRepositoryChange(ctx context.Contex
 	}
 
 	var reqs []reconcile.Request
-	for _, i := range list.Items {
-		if i.Status.ObservedSourceArtifactRevision != repo.GetArtifact().Revision {
-			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)})
+	for i, v := range list.Items {
+		if v.Status.ObservedSourceArtifactRevision != repo.GetArtifact().Revision {
+			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])})
 		}
 	}
 	return reqs
@@ -1159,9 +1159,9 @@ func (r *HelmChartReconciler) requestsForGitRepositoryChange(ctx context.Context
 	}
 
 	var reqs []reconcile.Request
-	for _, i := range list.Items {
-		if !repo.GetArtifact().HasRevision(i.Status.ObservedSourceArtifactRevision) {
-			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)})
+	for i, v := range list.Items {
+		if !repo.GetArtifact().HasRevision(v.Status.ObservedSourceArtifactRevision) {
+			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])})
 		}
 	}
 	return reqs
@@ -1189,9 +1189,9 @@ func (r *HelmChartReconciler) requestsForBucketChange(ctx context.Context, o cli
 	}
 
 	var reqs []reconcile.Request
-	for _, i := range list.Items {
-		if !bucket.GetArtifact().HasRevision(i.Status.ObservedSourceArtifactRevision) {
-			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&i)})
+	for i, v := range list.Items {
+		if !bucket.GetArtifact().HasRevision(v.Status.ObservedSourceArtifactRevision) {
+			reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&list.Items[i])})
 		}
 	}
 	return reqs

internal/controller/helmchart_controller_test.go

@@ -2029,6 +2029,7 @@ func TestHelmChartReconciler_statusConditions(t *testing.T) {
 		name             string
 		beforeFunc       func(obj *helmv1.HelmChart)
 		assertConditions []metav1.Condition
+		wantErr          bool
 	}{
 		{
 			name: "positive conditions only",
@@ -2055,6 +2056,7 @@ func TestHelmChartReconciler_statusConditions(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartPackageError", "some error"),
 				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "some error"),
 			},
+			wantErr: true,
 		},
 		{
 			name: "mixed positive and negative conditions",
@@ -2067,6 +2069,7 @@ func TestHelmChartReconciler_statusConditions(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "failed to get secret"),
 				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision"),
 			},
+			wantErr: true,
 		},
 	}
 
@@ -2098,22 +2101,19 @@ func TestHelmChartReconciler_statusConditions(t *testing.T) {
 			}
 
 			ctx := context.TODO()
-			recResult := sreconcile.ResultSuccess
-			var retErr error
-
 			summarizeHelper := summarize.NewHelper(record.NewFakeRecorder(32), serialPatcher)
 			summarizeOpts := []summarize.Option{
 				summarize.WithConditions(helmChartReadyCondition),
 				summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition),
-				summarize.WithReconcileResult(recResult),
-				summarize.WithReconcileError(retErr),
+				summarize.WithReconcileResult(sreconcile.ResultSuccess),
 				summarize.WithIgnoreNotFound(),
 				summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{
 					RequeueAfter: jitter.JitteredIntervalDuration(obj.GetRequeueAfter()),
 				}),
 				summarize.WithPatchFieldOwner("source-controller"),
 			}
-			_, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
+			_, err := summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...)
+			g.Expect(err != nil).To(Equal(tt.wantErr))
 
 			key := client.ObjectKeyFromObject(obj)
 			g.Expect(c.Get(ctx, key, obj)).ToNot(HaveOccurred())