Add test case for RemoteBuilder

Refactor the downloadChartFrom methods to reduce code duplication.

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

Author: souleb

internal/helm/chart/builder_remote.go

@@ -37,8 +37,8 @@ import (
 	"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
 )
 
-// Remote is a repository.ChartRepository or a repository.OCIChartRegistry.
-// It is used to download a chart from a remote repository or registry.
+// Remote is a repository.ChartRepository or a repository.OCIChartRepository.
+// It is used to download a chart from a remote Helm repository or OCI registry.
 type Remote interface {
 	// GetChart returns a chart.Chart from the remote repository.
 	Get(name, version string) (*repo.ChartVersion, error)
@@ -160,39 +160,14 @@ func (b *remoteChartBuilder) downloadFromOCIRepository(remote *repository.OCICha
 		return nil, &BuildError{Reason: ErrChartPull, Err: err}
 	}
 
-	result := &Build{}
-	result.Version = cv.Version
-	result.Name = cv.Name
-
-	// Set build specific metadata if instructed
-	if opts.VersionMetadata != "" {
-		ver, err := setBuildMetaData(result.Version, opts.VersionMetadata)
-		if err != nil {
-			return nil, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
-		}
-		result.Version = ver.String()
+	result, shouldReturn, err := generateBuildResult(cv, opts)
+	if err != nil {
+		return nil, err
 	}
 
-	requiresPackaging := len(opts.GetValuesFiles()) != 0 || opts.VersionMetadata != ""
-
-	// If all the following is true, we do not need to download and/or build the chart:
-	// - Chart name from cached chart matches resolved name
-	// - Chart version from cached chart matches calculated version
-	// - BuildOptions.Force is False
-	if opts.CachedChart != "" && !opts.Force {
-		if curMeta, err := LoadChartMetadataFromArchive(opts.CachedChart); err == nil {
-			// If the cached metadata is corrupt, we ignore its existence
-			// and continue the build
-			if err = curMeta.Validate(); err == nil {
-				if result.Name == curMeta.Name && result.Version == curMeta.Version {
-					result.Path = opts.CachedChart
-					result.ValuesFiles = opts.GetValuesFiles()
-					result.Packaged = requiresPackaging
-					*buildResult = *result
-					return nil, nil
-				}
-			}
-		}
+	if shouldReturn {
+		*buildResult = *result
+		return nil, nil
 	}
 
 	// Download the package for the resolved version
@@ -221,15 +196,38 @@ func (b *remoteChartBuilder) downloadFromRepository(remote *repository.ChartRepo
 		return nil, &BuildError{Reason: ErrChartReference, Err: err}
 	}
 
+	result, shouldReturn, err := generateBuildResult(cv, opts)
+	if err != nil {
+		return nil, err
+	}
+
+	if shouldReturn {
+		*buildResult = *result
+		return nil, nil
+	}
+
+	// Download the package for the resolved version
+	res, err := remote.DownloadChart(cv)
+	if err != nil {
+		err = fmt.Errorf("failed to download chart for remote reference: %w", err)
+		return nil, &BuildError{Reason: ErrChartPull, Err: err}
+	}
+
+	*buildResult = *result
+
+	return res, nil
+}
+
+func generateBuildResult(cv *repo.ChartVersion, opts BuildOptions) (*Build, bool, error) {
 	result := &Build{}
-	result.Name = cv.Name
 	result.Version = cv.Version
+	result.Name = cv.Name
 
 	// Set build specific metadata if instructed
 	if opts.VersionMetadata != "" {
 		ver, err := setBuildMetaData(result.Version, opts.VersionMetadata)
 		if err != nil {
-			return nil, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
+			return nil, false, &BuildError{Reason: ErrChartMetadataPatch, Err: err}
 		}
 		result.Version = ver.String()
 	}
@@ -249,23 +247,13 @@ func (b *remoteChartBuilder) downloadFromRepository(remote *repository.ChartRepo
 					result.Path = opts.CachedChart
 					result.ValuesFiles = opts.GetValuesFiles()
 					result.Packaged = requiresPackaging
-					*buildResult = *result
-					return nil, nil
+					return result, true, nil
 				}
 			}
 		}
 	}
 
-	// Download the package for the resolved version
-	res, err := remote.DownloadChart(cv)
-	if err != nil {
-		err = fmt.Errorf("failed to download chart for remote reference: %w", err)
-		return nil, &BuildError{Reason: ErrChartPull, Err: err}
-	}
-
-	*buildResult = *result
-
-	return res, nil
+	return result, false, nil
 }
 
 func setBuildMetaData(version, versionMetadata string) (*semver.Version, error) {

internal/helm/chart/builder_remote_test.go

@@ -19,6 +19,8 @@ package chart
 import (
 	"bytes"
 	"context"
+	"fmt"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -29,11 +31,35 @@ import (
 	helmchart "helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chartutil"
 	helmgetter "helm.sh/helm/v3/pkg/getter"
+	"helm.sh/helm/v3/pkg/registry"
 
 	"github.com/fluxcd/source-controller/internal/helm/chart/secureloader"
 	"github.com/fluxcd/source-controller/internal/helm/repository"
 )
 
+type mockRegistryClient struct {
+	tags         map[string][]string
+	requestedURL string
+}
+
+func (m *mockRegistryClient) Tags(url string) ([]string, error) {
+	m.requestedURL = url
+	if tags, ok := m.tags[url]; ok {
+		return tags, nil
+	}
+	return nil, fmt.Errorf("no tags found for %s", url)
+}
+
+func (m *mockRegistryClient) Login(url string, opts ...registry.LoginOption) error {
+	m.requestedURL = url
+	return nil
+}
+
+func (m *mockRegistryClient) Logout(url string, opts ...registry.LogoutOption) error {
+	m.requestedURL = url
+	return nil
+}
+
 // mockIndexChartGetter returns specific response for index and chart queries.
 type mockIndexChartGetter struct {
 	IndexResponse []byte
@@ -54,7 +80,7 @@ func (g *mockIndexChartGetter) LastGet() string {
 	return g.requestedURL
 }
 
-func TestRemoteBuilder_Build(t *testing.T) {
+func TestRemoteBuilder__BuildFromChartRepository(t *testing.T) {
 	g := NewWithT(t)
 
 	chartGrafana, err := os.ReadFile("./../testdata/charts/helmchart-0.1.0.tgz")
@@ -195,6 +221,140 @@ entries:
 	}
 }
 
+func TestRemoteBuilder_BuildFromOCIChatRepository(t *testing.T) {
+	g := NewWithT(t)
+
+	chartGrafana, err := os.ReadFile("./../testdata/charts/helmchart-0.1.0.tgz")
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(chartGrafana).ToNot(BeEmpty())
+
+	registryClient := &mockRegistryClient{
+		tags: map[string][]string{
+			"localhost:5000/my_repo/grafana": {"6.17.4"},
+		},
+	}
+
+	mockGetter := &mockIndexChartGetter{
+		ChartResponse: chartGrafana,
+	}
+
+	u, err := url.Parse("oci://localhost:5000/my_repo")
+	g.Expect(err).ToNot(HaveOccurred())
+
+	mockRepo := func() *repository.OCIChartRepository {
+		return &repository.OCIChartRepository{
+			URL:            *u,
+			Client:         mockGetter,
+			RegistryClient: registryClient,
+		}
+	}
+
+	tests := []struct {
+		name         string
+		reference    Reference
+		buildOpts    BuildOptions
+		repository   *repository.OCIChartRepository
+		wantValues   chartutil.Values
+		wantVersion  string
+		wantPackaged bool
+		wantErr      string
+	}{
+		{
+			name:      "invalid reference",
+			reference: LocalReference{},
+			wantErr:   "expected remote chart reference",
+		},
+		{
+			name:      "invalid reference - no name",
+			reference: RemoteReference{},
+			wantErr:   "no name set for remote chart reference",
+		},
+		{
+			name:       "chart not in repository",
+			reference:  RemoteReference{Name: "foo"},
+			repository: mockRepo(),
+			wantErr:    "failed to get chart version for remote reference",
+		},
+		{
+			name:       "chart version not in repository",
+			reference:  RemoteReference{Name: "grafana", Version: "1.1.1"},
+			repository: mockRepo(),
+			wantErr:    "failed to get chart version for remote reference",
+		},
+		{
+			name:       "invalid version metadata",
+			reference:  RemoteReference{Name: "grafana"},
+			repository: mockRepo(),
+			buildOpts:  BuildOptions{VersionMetadata: "^"},
+			wantErr:    "Invalid Metadata string",
+		},
+		{
+			name:         "with version metadata",
+			reference:    RemoteReference{Name: "grafana"},
+			repository:   mockRepo(),
+			buildOpts:    BuildOptions{VersionMetadata: "foo"},
+			wantVersion:  "6.17.4+foo",
+			wantPackaged: true,
+		},
+		{
+			name:        "default values",
+			reference:   RemoteReference{Name: "grafana"},
+			repository:  mockRepo(),
+			wantVersion: "0.1.0",
+			wantValues: chartutil.Values{
+				"replicaCount": float64(1),
+			},
+		},
+		{
+			name:      "merge values",
+			reference: RemoteReference{Name: "grafana"},
+			buildOpts: BuildOptions{
+				ValuesFiles: []string{"a.yaml", "b.yaml", "c.yaml"},
+			},
+			repository:  mockRepo(),
+			wantVersion: "6.17.4",
+			wantValues: chartutil.Values{
+				"a": "b",
+				"b": "d",
+			},
+			wantPackaged: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			tmpDir, err := os.MkdirTemp("", "remote-chart-builder-")
+			g.Expect(err).ToNot(HaveOccurred())
+			defer os.RemoveAll(tmpDir)
+			targetPath := filepath.Join(tmpDir, "chart.tgz")
+
+			b := NewRemoteBuilder(tt.repository)
+
+			cb, err := b.Build(context.TODO(), tt.reference, targetPath, tt.buildOpts)
+
+			if tt.wantErr != "" {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring(tt.wantErr))
+				g.Expect(cb).To(BeZero())
+				return
+			}
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(cb.Packaged).To(Equal(tt.wantPackaged), "unexpected Build.Packaged value")
+			g.Expect(cb.Path).ToNot(BeEmpty(), "empty Build.Path")
+
+			// Load the resulting chart and verify the values.
+			resultChart, err := secureloader.LoadFile(cb.Path)
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(resultChart.Metadata.Version).To(Equal(tt.wantVersion))
+
+			for k, v := range tt.wantValues {
+				g.Expect(v).To(Equal(resultChart.Values[k]))
+			}
+		})
+	}
+}
+
 func TestRemoteBuilder_Build_CachedChart(t *testing.T) {
 	g := NewWithT(t)
 

internal/helm/repository/oci_chart_repository.go

@@ -100,18 +100,14 @@ func WithTlsConfig(tlsConfig *tls.Config) OCIChartRepositoryOption {
 
 // 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,
-// or if there is no getter available for the scheme.
+// repository URL scheme. It returns an error on URL parsing failures.
+// It assumes that the url scheme has been validated to be an OCI scheme.
 func NewOCIChartRepository(repositoryURL string, chartRepoOpts ...OCIChartRepositoryOption) (*OCIChartRepository, error) {
 	u, err := url.Parse(repositoryURL)
 	if err != nil {
 		return nil, err
 	}
 
-	if !registry.IsOCI(repositoryURL) {
-		return nil, fmt.Errorf("the url scheme is not supported: %s", u.Scheme)
-	}
-
 	r := newOCIChartRepository()
 	r.URL = *u
 	for _, opt := range chartRepoOpts {
@@ -157,7 +153,7 @@ func (r *OCIChartRepository) Get(name, ver string) (*repo.ChartVersion, error) {
 	}, err
 }
 
-// this function shall be called for OCI registries only
+// This function shall be called for OCI registries only
 // It assumes that the ref has been validated to be an OCI reference.
 func (r *OCIChartRepository) getTags(ref string) ([]string, error) {
 	// Retrieve list of repository tags
@@ -182,10 +178,6 @@ func (r *OCIChartRepository) DownloadChart(chart *repo.ChartVersion) (*bytes.Buf
 	}
 
 	ref := chart.URLs[0]
-	if !registry.IsOCI(ref) {
-		return nil, fmt.Errorf("chart '%s' is not an OCI chart", chart.Name)
-	}
-
 	u, err := url.Parse(ref)
 	if err != nil {
 		err = fmt.Errorf("invalid chart URL format '%s': %w", ref, err)
@@ -201,6 +193,7 @@ func (r *OCIChartRepository) DownloadChart(chart *repo.ChartVersion) (*bytes.Buf
 }
 
 // Login attempts to login to the OCI registry.
+// It returns an error on failure.
 func (r *OCIChartRepository) Login(opts ...registry.LoginOption) error {
 	err := r.RegistryClient.Login(r.URL.Host, opts...)
 	if err != nil {
@@ -210,6 +203,7 @@ func (r *OCIChartRepository) Login(opts ...registry.LoginOption) error {
 }
 
 // Logout attempts to logout from the OCI registry.
+// It returns an error on failure.
 func (r *OCIChartRepository) Logout() error {
 	err := r.RegistryClient.Logout(r.URL.Host)
 	if err != nil {
@@ -218,6 +212,8 @@ func (r *OCIChartRepository) Logout() error {
 	return nil
 }
 
+// getLastMatchingVersionOrConstraint returns the last version that matches the given version string.
+// If the version string is empty, the highest available version is returned.
 func getLastMatchingVersionOrConstraint(cvs []string, ver string) (string, error) {
 	// Check for exact matches first
 	if ver != "" {