Addressing comments on api comments and code duplication

Signed-off-by: Soule BA <soule@weave.works>

Author: souleb

api/v1beta1/helmrepository_types.go

@@ -30,6 +30,11 @@ const (
 	// HelmRepositoryURLIndexKey is the key to use for indexing HelmRepository
 	// resources by their HelmRepositorySpec.URL.
 	HelmRepositoryURLIndexKey = ".metadata.helmRepositoryURL"
+	// HelmRepositoryTypeDefault is the default HelmRepository type.
+	// It is used when no type is specified and corresponds to a Helm repository.
+	HelmRepositoryTypeDefault = "default"
+	// HelmRepositoryTypeOCI is the type for an OCI repository.
+	HelmRepositoryTypeOCI = "oci"
 )
 
 // HelmRepositorySpec defines the reference to a Helm repository.

api/v1beta2/helmrepository_types.go

@@ -31,9 +31,11 @@ const (
 	// HelmRepositoryURLIndexKey is the key used for indexing HelmRepository
 	// objects by their HelmRepositorySpec.URL.
 	HelmRepositoryURLIndexKey = ".metadata.helmRepositoryURL"
-
+	// HelmRepositoryTypeDefault is the default HelmRepository type.
+	// It is used when no type is specified and corresponds to a Helm repository.
 	HelmRepositoryTypeDefault = "default"
-	HelmRepositoryTypeOCI     = "oci"
+	// HelmRepositoryTypeOCI is the type for an OCI repository.
+	HelmRepositoryTypeOCI = "oci"
 )
 
 // HelmRepositorySpec specifies the required configuration to produce an

config/manager/kustomization.yaml

@@ -6,4 +6,4 @@ resources:
 images:
 - name: fluxcd/source-controller
   newName: fluxcd/source-controller
-  newTag: latest
+  newTag: v0.24.4

controllers/helmchart_controller.go

@@ -445,16 +445,18 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1
 // object, and returns early.
 func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *sourcev1.HelmChart,
 	repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) {
-	var tlsConfig *tls.Config
+	var (
+		tlsConfig *tls.Config
+		logOpts   []registry.LoginOption
+	)
 
 	// Construct the Getter options from the HelmRepository data
 	clientOpts := []helmgetter.Option{
 		helmgetter.WithURL(repo.Spec.URL),
 		helmgetter.WithTimeout(repo.Spec.Timeout.Duration),
 		helmgetter.WithPassCredentialsAll(repo.Spec.PassCredentials),
 	}
-	secret, err := r.getHelmRepositorySecret(ctx, repo)
-	if secret != nil || err != nil {
+	if secret, err := r.getHelmRepositorySecret(ctx, repo); secret != nil || err != nil {
 		if err != nil {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to get secret '%s': %w", repo.Spec.SecretRef.Name, err),
@@ -488,13 +490,27 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 			// Requeue as content of secret might change
 			return sreconcile.ResultEmpty, e
 		}
+
+		// Build registryClient options from secret
+		logOpt, err := loginOptionFromSecret(*secret)
+		if err != nil {
+			e := &serror.Event{
+				Err:    fmt.Errorf("failed to configure Helm client with secret data: %w", err),
+				Reason: sourcev1.AuthenticationFailedReason,
+			}
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
+			// Requeue as content of secret might change
+			return sreconcile.ResultEmpty, e
+		}
+
+		logOpts = append([]registry.LoginOption{}, logOpt)
 	}
 
 	// Initialize the chart repository
 	var chartRepo chart.Remote
 	if repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
 		if !registry.IsOCI(repo.Spec.URL) {
-			err = fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
+			err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
 			return chartRepoErrorReturn(err, obj)
 		}
 		// Tell the chart repository to use the OCI client with the configured getter
@@ -507,15 +523,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 
 		// If login options are configured, use them to login to the registry
 		// The OCIGetter will later retrieve the stored credentials to pull the chart
-		if secret != nil {
-			// Construct actual options
-			logOpt, err := loginOptionFromSecret(*secret)
-			if err != nil {
-				return chartRepoErrorReturn(err, obj)
-			}
-
-			logOpts := append([]registry.LoginOption{}, logOpt)
-
+		if logOpts != nil {
 			// create a temporary file to store the credentials
 			// this is needed because otherwise the credentials are stored in ~/.docker/config.json.
 			// TODO@souleb: remove this once the registry move to Oras v2
@@ -535,7 +543,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		}
 	} else {
 		var httpChartRepo *repository.ChartRepository
-		httpChartRepo, err = repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
+		httpChartRepo, err := repository.NewChartRepository(repo.Spec.URL, r.Storage.LocalPath(*repo.GetArtifact()), r.Getters, tlsConfig, clientOpts,
 			repository.WithMemoryCache(r.Storage.LocalPath(*repo.GetArtifact()), r.Cache, r.TTL, func(event string) {
 				r.IncCacheEvents(event, obj.Name, obj.Namespace)
 			}))

controllers/helmchart_controller_test.go

@@ -33,6 +33,7 @@ import (
 
 	"github.com/darkowlzz/controller-check/status"
 	. "github.com/onsi/gomega"
+	hchart "helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	"helm.sh/helm/v3/pkg/registry"
 	corev1 "k8s.io/api/core/v1"
@@ -54,7 +55,6 @@ import (
 	"github.com/fluxcd/source-controller/internal/helm/chart"
 	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
 	"github.com/fluxcd/source-controller/internal/reconcile/summarize"
-	hchart "helm.sh/helm/v3/pkg/chart"
 )
 
 func TestHelmChartReconciler_Reconcile(t *testing.T) {

controllers/helmrepository_controller_oci.go

@@ -83,8 +83,7 @@ type HelmRepositoryOCIReconciler struct {
 	client.Client
 	kuberecorder.EventRecorder
 	helper.Metrics
-	Getters helmgetter.Providers
-	// Storage        *Storage
+	Getters        helmgetter.Providers
 	ControllerName string
 	RegistryClient *registry.Client
 }

internal/helm/repository/oci_chart_repository.go

@@ -93,14 +93,6 @@ func WithOCIGetterOptions(getterOpts []getter.Option) OCIChartRepositoryOption {
 	}
 }
 
-// WithOCITlsConfig returns a ChartRepositoryOption that will set the tls.Config
-func WithTlsConfig(tlsConfig *tls.Config) OCIChartRepositoryOption {
-	return func(r *OCIChartRepository) error {
-		r.tlsConfig = tlsConfig
-		return nil
-	}
-}
-
 // NewOCIChartRepository constructs and returns a new ChartRepository with
 // the ChartRepository.Client configured to the getter.Getter for the
 // repository URL scheme. It returns an error on URL parsing failures.
@@ -111,7 +103,7 @@ func NewOCIChartRepository(repositoryURL string, chartRepoOpts ...OCIChartReposi
 		return nil, err
 	}
 
-	r := newOCIChartRepository()
+	r := &OCIChartRepository{}
 	r.URL = *u
 	for _, opt := range chartRepoOpts {
 		if err := opt(r); err != nil {
@@ -122,10 +114,6 @@ func NewOCIChartRepository(repositoryURL string, chartRepoOpts ...OCIChartReposi
 	return r, nil
 }
 
-func newOCIChartRepository() *OCIChartRepository {
-	return &OCIChartRepository{}
-}
-
 // Get returns the repo.ChartVersion for the given name, the version is expected
 // to be a semver.Constraints compatible string. If version is empty, the latest
 // stable version will be returned and prerelease versions will be ignored.