Merge pull request #819 from fluxcd/tidy-code

Remove obsolete code and tidy things

internal/action/diff.go

@@ -24,7 +24,7 @@ import (
 	helmaction "helm.sh/helm/v3/pkg/action"
 	helmrelease "helm.sh/helm/v3/pkg/release"
 	"k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 
@@ -42,7 +42,7 @@ func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmreleas
 	if err != nil {
 		return nil, err
 	}
-	c, err := client.New(cfg, client.Options{DryRun: pointer.Bool(true)})
+	c, err := client.New(cfg, client.Options{DryRun: ptr.To(true)})
 	if err != nil {
 		return nil, err
 	}

internal/action/verify.go

@@ -90,19 +90,6 @@ func LastRelease(config *helmaction.Configuration, releaseName string) (*helmrel
 	return rls, nil
 }
 
-// IsInstalled returns true if there is any release in the Helm storage with the
-// given name. It returns any error other than driver.ErrReleaseNotFound.
-func IsInstalled(config *helmaction.Configuration, releaseName string) (bool, error) {
-	_, err := config.Releases.Last(release.ShortenName(releaseName))
-	if err != nil {
-		if errors.Is(err, helmdriver.ErrReleaseNotFound) {
-			return false, nil
-		}
-		return false, err
-	}
-	return true, nil
-}
-
 // VerifySnapshot verifies the data of the given v2beta2.Snapshot
 // matches the release object in the Helm storage. It returns the verified
 // release, or an error of type ErrReleaseNotFound, ErrReleaseDisappeared,
@@ -127,30 +114,6 @@ func VerifySnapshot(config *helmaction.Configuration, snapshot *v2.Snapshot) (rl
 	return rls, nil
 }
 
-// VerifyLastStorageItem verifies the data of the given v2beta2.Snapshot
-// matches the last release object in the Helm storage. It returns the release
-// and any verification error of type ErrReleaseNotFound, ErrReleaseDisappeared,
-// ErrReleaseDigest or ErrReleaseNotObserved indicating the reason for the
-// verification failure.
-func VerifyLastStorageItem(config *helmaction.Configuration, snapshot *v2.Snapshot) (rls *helmrelease.Release, err error) {
-	if snapshot == nil {
-		return nil, ErrReleaseNotFound
-	}
-
-	rls, err = config.Releases.Last(snapshot.Name)
-	if err != nil {
-		if errors.Is(err, helmdriver.ErrReleaseNotFound) {
-			return nil, ErrReleaseDisappeared
-		}
-		return nil, err
-	}
-
-	if err = VerifyReleaseObject(snapshot, rls); err != nil {
-		return nil, err
-	}
-	return rls, nil
-}
-
 // VerifyReleaseObject verifies the data of the given v2beta2.Snapshot
 // matches the given Helm release object. It returns an error of type
 // ErrReleaseDigest or ErrReleaseNotObserved indicating the reason for the

internal/action/verify_test.go

@@ -225,70 +225,6 @@ func TestReleaseTargetChanged(t *testing.T) {
 	}
 }
 
-func TestIsInstalled(t *testing.T) {
-	var mockError = errors.New("query mock error")
-
-	tests := []struct {
-		name        string
-		releaseName string
-		releases    []*helmrelease.Release
-		queryError  error
-		want        bool
-		wantErr     error
-	}{
-		{
-			name:        "installed",
-			releaseName: "release",
-			releases: []*helmrelease.Release{
-				testutil.BuildRelease(&helmrelease.MockReleaseOptions{
-					Name:      "release",
-					Version:   1,
-					Status:    helmrelease.StatusDeployed,
-					Namespace: "default",
-				}),
-			},
-			want: true,
-		},
-		{
-			name:        "not installed",
-			releaseName: "release",
-			want:        false,
-		},
-		{
-			name:       "release list error",
-			queryError: mockError,
-			wantErr:    mockError,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			g := NewWithT(t)
-
-			s := helmstorage.Init(driver.NewMemory())
-			for _, v := range tt.releases {
-				g.Expect(s.Create(v)).To(Succeed())
-			}
-
-			s.Driver = &storage.Failing{
-				Driver:   s.Driver,
-				QueryErr: tt.queryError,
-			}
-
-			got, err := IsInstalled(&helmaction.Configuration{Releases: s}, tt.releaseName)
-
-			if tt.wantErr != nil {
-				g.Expect(err).To(HaveOccurred())
-				g.Expect(err).To(Equal(tt.wantErr))
-				g.Expect(got).To(BeFalse())
-				return
-			}
-
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(got).To(Equal(tt.want))
-		})
-	}
-}
-
 func TestVerifySnapshot(t *testing.T) {
 	mock := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
 		Name:      "release",
@@ -372,89 +308,6 @@ func TestVerifySnapshot(t *testing.T) {
 	}
 }
 
-func TestVerifyLastStorageItem(t *testing.T) {
-	mockOne := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
-		Name:      "release",
-		Version:   1,
-		Status:    helmrelease.StatusSuperseded,
-		Namespace: "default",
-	})
-	mockTwo := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
-		Name:      "release",
-		Version:   2,
-		Status:    helmrelease.StatusDeployed,
-		Namespace: "default",
-	})
-	mockInfo := release.ObservedToSnapshot(release.ObserveRelease(mockTwo))
-	mockQueryErr := errors.New("mock query error")
-
-	tests := []struct {
-		name       string
-		snapshot   *v2.Snapshot
-		releases   []*helmrelease.Release
-		queryError error
-		want       *helmrelease.Release
-		wantErr    error
-	}{
-		{
-			name:     "valid last release",
-			snapshot: mockInfo,
-			releases: []*helmrelease.Release{mockOne, mockTwo},
-			want:     mockTwo,
-		},
-		{
-			name:     "invalid last release",
-			snapshot: mockInfo,
-			releases: []*helmrelease.Release{mockOne},
-			wantErr:  ErrReleaseNotObserved,
-		},
-		{
-			name:     "no last release",
-			snapshot: mockInfo,
-			releases: []*helmrelease.Release{},
-			wantErr:  ErrReleaseDisappeared,
-		},
-		{
-			name:     "no release snapshot",
-			snapshot: nil,
-			releases: nil,
-			wantErr:  ErrReleaseNotFound,
-		},
-		{
-			name:       "driver query error",
-			snapshot:   mockInfo,
-			queryError: mockQueryErr,
-			wantErr:    mockQueryErr,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			g := NewWithT(t)
-
-			s := helmstorage.Init(driver.NewMemory())
-			for _, v := range tt.releases {
-				g.Expect(s.Create(v)).To(Succeed())
-			}
-
-			s.Driver = &storage.Failing{
-				Driver:   s.Driver,
-				QueryErr: tt.queryError,
-			}
-
-			rls, err := VerifyLastStorageItem(&helmaction.Configuration{Releases: s}, tt.snapshot)
-			if tt.wantErr != nil {
-				g.Expect(err).To(HaveOccurred())
-				g.Expect(err).To(Equal(tt.wantErr))
-				g.Expect(rls).To(BeNil())
-				return
-			}
-
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(rls).To(Equal(tt.want))
-		})
-	}
-}
-
 func TestVerifyReleaseObject(t *testing.T) {
 	mockRls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
 		Name:      "release",

internal/controller/helmrelease_controller.go

@@ -549,6 +549,9 @@ func (r *HelmReleaseReconciler) adoptLegacyRelease(ctx context.Context, getter g
 		action.WithStorage(action.DefaultStorageDriver, storageNamespace),
 		action.WithStorageLog(action.NewDebugLog(ctrl.LoggerFrom(ctx).V(logger.TraceLevel))),
 	)
+	if err != nil {
+		return err
+	}
 
 	// Get the last successful release based on the observation for the v2beta1
 	// object.

internal/controller/helmrelease_controller_test.go

@@ -512,7 +512,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			Status: v2.HelmReleaseStatus{
 				HelmChart:                   chart.Namespace + "/" + chart.Name,
 				LastReleaseRevision:         rls.Version,
-				LastAttemptedValuesChecksum: valChecksum.Hex(),
+				LastAttemptedValuesChecksum: valChecksum.Encoded(),
 			},
 		}
 

internal/features/features.go

@@ -31,11 +31,15 @@ const (
 	// DetectDrift configures the detection of cluster state drift compared to
 	// the desired state as described in the manifest of the Helm release
 	// storage object.
+	// Deprecated in v0.37.0, use the drift detection mode on the HelmRelease
+	// object instead.
 	DetectDrift = "DetectDrift"
 
 	// CorrectDrift configures the correction of cluster state drift compared to
 	// the desired state as described in the manifest of the Helm release. It
 	// is only effective when DetectDrift is enabled.
+	// Deprecated in v0.37.0, use the drift detection mode on the HelmRelease
+	// object instead.
 	CorrectDrift = "CorrectDrift"
 
 	// AllowDNSLookups allows the controller to perform DNS lookups when rendering Helm
@@ -61,11 +65,11 @@ var features = map[string]bool{
 	// opt-in from v0.28
 	CacheSecretsAndConfigMaps: false,
 	// DetectDrift
-	// opt-in from v0.31
+	// deprecated in v0.37.0
 	DetectDrift: false,
 	// CorrectDrift,
-	// opt-out from v0.31.2
-	CorrectDrift: true,
+	// deprecated in v0.37.0
+	CorrectDrift: false,
 	// AllowDNSLookups
 	// opt-in from v0.31
 	AllowDNSLookups: false,

internal/reconcile/atomic_release.go

@@ -321,7 +321,7 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 		log.Info(msgWithReason("detected changes in cluster state", diff.SummarizeDiffSetBrief(state.Diff)))
 		for _, change := range state.Diff {
 			if change.Type == jsondiff.DiffTypeCreate || change.Type == jsondiff.DiffTypeUpdate {
-				log.V(logger.DebugLevel).Info(fmt.Sprintf("observed change in cluster state"), "diff", change)
+				log.V(logger.DebugLevel).Info("observed change in cluster state", "diff", change)
 			}
 		}
 

internal/reconcile/atomic_release_test.go

@@ -33,7 +33,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/tools/record"
-	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/fluxcd/pkg/apis/meta"
@@ -616,7 +616,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 			spec: func(spec *v2.HelmReleaseSpec) {
 				spec.Install = &v2.Install{
 					Remediation: &v2.InstallRemediation{
-						RemediateLastFailure: pointer.Bool(true),
+						RemediateLastFailure: ptr.To(true),
 					},
 				}
 				spec.Uninstall = &v2.Uninstall{
@@ -635,7 +635,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 			spec: func(spec *v2.HelmReleaseSpec) {
 				spec.Install = &v2.Install{
 					Remediation: &v2.InstallRemediation{
-						RemediateLastFailure: pointer.Bool(true),
+						RemediateLastFailure: ptr.To(true),
 					},
 				}
 				spec.Test = &v2.Test{
@@ -740,7 +740,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 			spec: func(spec *v2.HelmReleaseSpec) {
 				spec.Upgrade = &v2.Upgrade{
 					Remediation: &v2.UpgradeRemediation{
-						RemediateLastFailure: pointer.Bool(true),
+						RemediateLastFailure: ptr.To(true),
 					},
 				}
 			},
@@ -778,7 +778,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 				spec.Upgrade = &v2.Upgrade{
 					Remediation: &v2.UpgradeRemediation{
 						Strategy:             &strategy,
-						RemediateLastFailure: pointer.Bool(true),
+						RemediateLastFailure: ptr.To(true),
 					},
 				}
 				spec.Uninstall = &v2.Uninstall{
@@ -816,7 +816,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 			spec: func(spec *v2.HelmReleaseSpec) {
 				spec.Upgrade = &v2.Upgrade{
 					Remediation: &v2.UpgradeRemediation{
-						RemediateLastFailure: pointer.Bool(true),
+						RemediateLastFailure: ptr.To(true),
 					},
 				}
 				spec.Test = &v2.Test{

internal/reconcile/state.go

@@ -129,7 +129,7 @@ func DetermineReleaseState(ctx context.Context, cfg *action.ConfigFactory, req *
 	case helmrelease.StatusFailed:
 		return ReleaseState{Status: ReleaseStatusFailed}, nil
 	case helmrelease.StatusUninstalled:
-		return ReleaseState{Status: ReleaseStatusAbsent, Reason: fmt.Sprintf("found uninstalled release in storage")}, nil
+		return ReleaseState{Status: ReleaseStatusAbsent, Reason: "found uninstalled release in storage"}, nil
 	case helmrelease.StatusDeployed:
 		// Verify the release is in sync with the desired configuration.
 		if err = action.VerifyRelease(rls, cur, req.Chart.Metadata, req.Values); err != nil {