From 2e0b6cb6c92a64084f314b0ec4fd4e640f6ec3e3 Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Fri, 11 Sep 2020 00:25:14 +0200
Subject: [PATCH 1/4] Allow overwriting chart values from GitRepository

---
 api/v1alpha1/helmchart_types.go               |   5 +
 .../source.toolkit.fluxcd.io_helmcharts.yaml  |   4 +
 controllers/helmchart_controller.go           | 109 ++++++++++++------
 docs/api/source.md                            |  26 +++++
 4 files changed, 107 insertions(+), 37 deletions(-)

diff --git a/api/v1alpha1/helmchart_types.go b/api/v1alpha1/helmchart_types.go
index 3d48f643..c5e731fd 100644
--- a/api/v1alpha1/helmchart_types.go
+++ b/api/v1alpha1/helmchart_types.go
@@ -41,6 +41,11 @@ type HelmChartSpec struct {
 	// The interval at which to check the Source for updates.
 	// +required
 	Interval metav1.Duration `json:"interval"`
+
+	// Alternative values file to use as the default chart values, expected to be
+	// a relative path in the SourceRef. Ignored when omitted.
+	// +optional
+	ValuesFile string `json:"valuesFile,omitempty"`
 }
 
 // LocalHelmChartSourceReference contains enough information to let you locate the
diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml
index b0a22796..9bae385d 100644
--- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml
+++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml
@@ -85,6 +85,10 @@ spec:
                 - kind
                 - name
                 type: object
+              valuesFile:
+                description: Alternative values file to use as the default chart values,
+                  expected to be a relative path in the SourceRef. Ignored when omitted.
+                type: string
               version:
                 description: The chart version semver expression, ignored for charts
                   from GitRepository sources. Defaults to latest when omitted.
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
index d733a1b7..8a1d8f6d 100644
--- a/controllers/helmchart_controller.go
+++ b/controllers/helmchart_controller.go
@@ -19,6 +19,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -104,7 +105,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 		}
 	}
 
-	// set initial status
+	// Conditionally set progressing condition in status
 	if resetChart, ok := r.resetStatus(chart); ok {
 		chart = resetChart
 		if err := r.Status().Update(ctx, &chart); err != nil {
@@ -113,11 +114,12 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 		}
 	}
 
-	// purge old artifacts from storage
+	// Purge all but current artifact from storage
 	if err := r.gc(chart, false); err != nil {
 		log.Error(err, "unable to purge old artifacts")
 	}
 
+	// Perform the reconciliation for the chart source type
 	var reconciledChart sourcev1.HelmChart
 	var reconcileErr error
 	switch chart.Spec.SourceRef.Kind {
@@ -146,19 +148,19 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 		return ctrl.Result{}, err
 	}
 
-	// update status with the reconciliation result
+	// Update status with the reconciliation result
 	if err := r.Status().Update(ctx, &reconciledChart); err != nil {
 		log.Error(err, "unable to update status")
 		return ctrl.Result{Requeue: true}, err
 	}
 
-	// if reconciliation failed, record the failure and requeue immediately
+	// If reconciliation failed, record the failure and requeue immediately
 	if reconcileErr != nil {
 		r.event(reconciledChart, recorder.EventSeverityError, reconcileErr.Error())
 		return ctrl.Result{Requeue: true}, reconcileErr
 	}
 
-	// emit revision change event
+	// Emit an event if we did not have an artifact before, or the revision has changed
 	if chart.Status.Artifact == nil || reconciledChart.Status.Artifact.Revision != chart.Status.Artifact.Revision {
 		r.event(reconciledChart, recorder.EventSeverityInfo, sourcev1.HelmChartReadyMessage(reconciledChart))
 	}
@@ -167,7 +169,6 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 		time.Now().Sub(start).String(),
 		chart.GetInterval().Duration.String(),
 	))
-
 	return ctrl.Result{RequeueAfter: chart.GetInterval().Duration}, nil
 }
 
@@ -195,8 +196,9 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
 	}
 
-	// return early on unchanged chart version
-	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), cv.Version, fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version))
+	// Return early if the revision is still the same as the current artifact
+	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), cv.Version,
+		fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version))
 	if repository.GetArtifact() != nil && repository.GetArtifact().Revision == cv.Version {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
@@ -228,6 +230,7 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		u.RawQuery = q.Encode()
 	}
 
+	// Get the getter for the protocol
 	c, err := r.Getters.ByScheme(u.Scheme)
 	if err != nil {
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
@@ -258,21 +261,14 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		clientOpts = opts
 	}
 
-	// TODO(hidde): implement timeout from the HelmRepository
-	//  https://github.com/helm/helm/pull/7950
-	res, err := c.Get(u.String(), clientOpts...)
-	if err != nil {
-		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
-	}
-
-	// create artifact dir
+	// Ensure artifact directory exists
 	err = r.Storage.MkdirAll(artifact)
 	if err != nil {
 		err = fmt.Errorf("unable to create chart directory: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// acquire lock
+	// Acquire a lock for the artifact
 	unlock, err := r.Storage.Lock(artifact)
 	if err != nil {
 		err = fmt.Errorf("unable to acquire lock: %w", err)
@@ -280,13 +276,20 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 	}
 	defer unlock()
 
-	// save artifact to storage
+	// TODO(hidde): implement timeout from the HelmRepository
+	//  https://github.com/helm/helm/pull/7950
+	res, err := c.Get(u.String(), clientOpts...)
+	if err != nil {
+		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
+	}
+
+	// Write the artifact to storage
 	if err := r.Storage.AtomicWriteFile(&artifact, res, 0644); err != nil {
 		err = fmt.Errorf("unable to write chart file: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// update symlink
+	// Update symlink
 	chartUrl, err := r.Storage.Symlink(artifact, fmt.Sprintf("%s-latest.tgz", cv.Name))
 	if err != nil {
 		err = fmt.Errorf("storage error: %w", err)
@@ -297,7 +300,7 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 	return sourcev1.HelmChartReady(chart, artifact, chartUrl, sourcev1.ChartPullSucceededReason, message), nil
 }
 
-// getChartRepositoryWithArtifact attempts to get the ChartRepository
+// getChartRepositoryWithArtifact attempts to get the v1alpha1.HelmRepository
 // for the given chart. It returns an error if the HelmRepository could
 // not be retrieved or if does not have an artifact.
 func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.HelmRepository, error) {
@@ -318,7 +321,7 @@ func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context
 	}
 
 	if repository.GetArtifact() == nil {
-		err = fmt.Errorf("no repository index artifact found in HelmRepository '%s'", repository.Name)
+		err = fmt.Errorf("no repository index artifact found in HelmRepository '%s'", name)
 	}
 
 	return repository, err
@@ -326,7 +329,7 @@ func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context
 
 func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	repository sourcev1.GitRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
-	// create tmp dir
+	// Create temporary working directory
 	tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s", chart.Namespace, chart.Name))
 	if err != nil {
 		err = fmt.Errorf("tmp dir error: %w", err)
@@ -334,34 +337,34 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	}
 	defer os.RemoveAll(tmpDir)
 
-	// open file
+	// Open GitRepository artifact file and untar files into working directory
 	f, err := os.Open(r.Storage.LocalPath(*repository.GetArtifact()))
 	if err != nil {
 		err = fmt.Errorf("artifact open error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
-
-	// extract artifact files
 	if _, err = untar.Untar(f, tmpDir); err != nil {
+		f.Close()
 		err = fmt.Errorf("artifact untar error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
+	f.Close()
 
-	// ensure configured path is a chart directory
+	// Ensure configured path is a chart directory
 	chartPath := path.Join(tmpDir, chart.Spec.Chart)
 	if _, err := chartutil.IsChartDir(chartPath); err != nil {
 		err = fmt.Errorf("chart path error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// read chart metadata
+	// Read the chart metadata
 	chartMetadata, err := chartutil.LoadChartfile(path.Join(chartPath, chartutil.ChartfileName))
 	if err != nil {
 		err = fmt.Errorf("load chart metadata error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// return early on unchanged chart version
+	// Return early if the revision is still the same as the current chart artifact
 	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version, fmt.Sprintf("%s-%s.tgz", chartMetadata.Name, chartMetadata.Version))
 	if chart.GetArtifact() != nil && chart.GetArtifact().Revision == chartMetadata.Version {
 		if artifact.URL != repository.GetArtifact().URL {
@@ -371,14 +374,42 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 		return chart, nil
 	}
 
-	// create artifact dir
+	// Overwrite default values if instructed to
+	if chart.Spec.ValuesFile != "" {
+		srcPath := path.Join(tmpDir, chart.Spec.ValuesFile)
+		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
+			err = fmt.Errorf("invalid values file path: %s", chart.Spec.ValuesFile)
+			return chart, err
+		}
+		src, err := os.Open(srcPath)
+		if err != nil {
+			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
+			return chart, err
+		}
+		t, err := os.OpenFile(path.Join(tmpDir, chartutil.ValuesfileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
+		if err != nil {
+			src.Close()
+			err = fmt.Errorf("failed to open values file '%s': %w", chartutil.ValuesfileName, err)
+			return chart, err
+		}
+		if _, err := io.Copy(t, src); err != nil {
+			t.Close()
+			src.Close()
+			err = fmt.Errorf("failed to copy values file '%s' to '%s: %w", chart.Spec.ValuesFile, chartutil.ValuesfileName, err)
+			return chart, err
+		}
+		t.Close()
+		src.Close()
+	}
+
+	// Ensure artifact directory exists
 	err = r.Storage.MkdirAll(artifact)
 	if err != nil {
 		err = fmt.Errorf("unable to create artifact directory: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	// acquire lock
+	// Acquire a lock for the artifact
 	unlock, err := r.Storage.Lock(artifact)
 	if err != nil {
 		err = fmt.Errorf("unable to acquire lock: %w", err)
@@ -386,7 +417,8 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	}
 	defer unlock()
 
-	// package chart
+	// Package the chart, we use the action here instead of relying on the
+	// chartutil.Save method as the action performs a dependency check for us
 	pkg := action.NewPackage()
 	pkg.Destination = tmpDir
 	src, err := pkg.Run(chartPath, nil)
@@ -395,7 +427,7 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 	}
 
-	// copy chart package
+	// Copy the packaged chart to the artifact path
 	cf, err := os.Open(src)
 	if err != nil {
 		err = fmt.Errorf("failed to open chart package: %w", err)
@@ -408,7 +440,7 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	}
 	cf.Close()
 
-	// update symlink
+	// Update symlink
 	cUrl, err := r.Storage.Symlink(artifact, fmt.Sprintf("%s-latest.tgz", chartMetadata.Name))
 	if err != nil {
 		err = fmt.Errorf("storage error: %w", err)
@@ -420,8 +452,8 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 }
 
 // getGitRepositoryWithArtifact attempts to get the GitRepository for the given
-// chart. It returns an error if the GitRepository could not be retrieved or
-// does not have an artifact.
+// chart. It returns an error if the v1alpha1.GitRepository could not be retrieved
+// or does not have an artifact.
 func (r *HelmChartReconciler) getGitRepositoryWithArtifact(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.GitRepository, error) {
 	if chart.Spec.SourceRef.Name == "" {
 		return sourcev1.GitRepository{}, fmt.Errorf("no GitRepository reference given")
@@ -449,18 +481,20 @@ func (r *HelmChartReconciler) getGitRepositoryWithArtifact(ctx context.Context,
 // resetStatus returns a modified v1alpha1.HelmChart and a boolean indicating
 // if the status field has been reset.
 func (r *HelmChartReconciler) resetStatus(chart sourcev1.HelmChart) (sourcev1.HelmChart, bool) {
+	// The artifact does no longer exist
 	if chart.GetArtifact() != nil && !r.Storage.ArtifactExist(*chart.GetArtifact()) {
 		chart = sourcev1.HelmChartProgressing(chart)
 		chart.Status.Artifact = nil
 		return chart, true
 	}
+	// The chart specification has changed
 	if chart.Generation != chart.Status.ObservedGeneration {
 		return sourcev1.HelmChartProgressing(chart), true
 	}
 	return chart, false
 }
 
-// gc performs a garbage collection on all but current artifacts of
+// gc performs a garbage collection on all but the current artifact of
 // the given chart.
 func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart, all bool) error {
 	if all {
@@ -472,7 +506,8 @@ func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart, all bool) error {
 	return nil
 }
 
-// event emits a Kubernetes event and forwards the event to notification controller if configured
+// event emits a Kubernetes event and forwards the event to notification
+// controller if configured.
 func (r *HelmChartReconciler) event(chart sourcev1.HelmChart, severity, msg string) {
 	if r.EventRecorder != nil {
 		r.EventRecorder.Eventf(&chart, "Normal", severity, msg)
diff --git a/docs/api/source.md b/docs/api/source.md
index 49c6bfd0..46508777 100644
--- a/docs/api/source.md
+++ b/docs/api/source.md
@@ -298,6 +298,19 @@ Kubernetes meta/v1.Duration
 <p>The interval at which to check the Source for updates.</p>
 </td>
 </tr>
+<tr>
+<td>
+<code>valuesFile</code><br>
+<em>
+string
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>Alternative values file to use as the default chart values, expected to be
+a relative path in the SourceRef. Ignored when omitted.</p>
+</td>
+</tr>
 </table>
 </td>
 </tr>
@@ -911,6 +924,19 @@ Kubernetes meta/v1.Duration
 <p>The interval at which to check the Source for updates.</p>
 </td>
 </tr>
+<tr>
+<td>
+<code>valuesFile</code><br>
+<em>
+string
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>Alternative values file to use as the default chart values, expected to be
+a relative path in the SourceRef. Ignored when omitted.</p>
+</td>
+</tr>
 </tbody>
 </table>
 </div>

From fd36d2d4f9be4720a50ff984f5948e9ad73d74f8 Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Fri, 11 Sep 2020 11:32:10 +0200
Subject: [PATCH 2/4] Allow overwriting chart values from HelmRepository

---
 controllers/helmchart_controller.go | 96 ++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 14 deletions(-)

diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
index 8a1d8f6d..09a1ff3f 100644
--- a/controllers/helmchart_controller.go
+++ b/controllers/helmchart_controller.go
@@ -283,10 +283,77 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
 	}
 
-	// Write the artifact to storage
-	if err := r.Storage.AtomicWriteFile(&artifact, res, 0644); err != nil {
-		err = fmt.Errorf("unable to write chart file: %w", err)
-		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+	switch {
+	case chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName:
+		// Create temporary working directory
+		tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
+		if err != nil {
+			err = fmt.Errorf("tmp dir error: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
+		defer os.RemoveAll(tmpDir)
+
+		// Untar chart into working directory
+		if _, err = untar.Untar(res, tmpDir); err != nil {
+			err = fmt.Errorf("chart untar error: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
+
+		// Overwrite values file
+		chartPath := path.Join(tmpDir, cv.Name)
+		srcPath := path.Join(chartPath, chart.Spec.ValuesFile)
+		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
+			err = fmt.Errorf("invalid values file: %s", chart.Spec.ValuesFile)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
+		}
+		src, err := os.Open(srcPath)
+		if err != nil {
+			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
+		}
+		t, err := os.OpenFile(path.Join(chartPath, chartutil.ValuesfileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
+		if err != nil {
+			src.Close()
+			err = fmt.Errorf("failed to open default values: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
+		}
+		if _, err := io.Copy(t, src); err != nil {
+			t.Close()
+			src.Close()
+			err = fmt.Errorf("failed to overwrite default values with '%s: %w", chart.Spec.ValuesFile, err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
+		}
+		t.Close()
+		src.Close()
+
+		// Package the chart, we use the action here instead of relying on the
+		// chartutil.Save method as the action performs a dependency check for us
+		pkg := action.NewPackage()
+		pkg.Destination = tmpDir
+		pkgPath, err := pkg.Run(chartPath, nil)
+		if err != nil {
+			err = fmt.Errorf("chart package error: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
+		}
+
+		// Copy the packaged chart to the artifact path
+		cf, err := os.Open(pkgPath)
+		if err != nil {
+			err = fmt.Errorf("failed to open chart package: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
+		if err := r.Storage.Copy(&artifact, cf); err != nil {
+			cf.Close()
+			err = fmt.Errorf("failed to copy chart package to storage: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
+		cf.Close()
+	default:
+		// Write artifact to storage
+		if err := r.Storage.AtomicWriteFile(&artifact, res, 0644); err != nil {
+			err = fmt.Errorf("unable to write chart file: %w", err)
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
 	}
 
 	// Update symlink
@@ -330,7 +397,7 @@ func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context
 func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	repository sourcev1.GitRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
 	// Create temporary working directory
-	tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s", chart.Namespace, chart.Name))
+	tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
 	if err != nil {
 		err = fmt.Errorf("tmp dir error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -365,7 +432,8 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	}
 
 	// Return early if the revision is still the same as the current chart artifact
-	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version, fmt.Sprintf("%s-%s.tgz", chartMetadata.Name, chartMetadata.Version))
+	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version,
+		fmt.Sprintf("%s-%s.tgz", chartMetadata.Name, chartMetadata.Version))
 	if chart.GetArtifact() != nil && chart.GetArtifact().Revision == chartMetadata.Version {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
@@ -375,28 +443,28 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	}
 
 	// Overwrite default values if instructed to
-	if chart.Spec.ValuesFile != "" {
-		srcPath := path.Join(tmpDir, chart.Spec.ValuesFile)
+	if chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName {
+		srcPath := path.Join(chartPath, chart.Spec.ValuesFile)
 		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
 			err = fmt.Errorf("invalid values file path: %s", chart.Spec.ValuesFile)
-			return chart, err
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}
 		src, err := os.Open(srcPath)
 		if err != nil {
 			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
-			return chart, err
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}
 		t, err := os.OpenFile(path.Join(tmpDir, chartutil.ValuesfileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
 		if err != nil {
 			src.Close()
 			err = fmt.Errorf("failed to open values file '%s': %w", chartutil.ValuesfileName, err)
-			return chart, err
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}
 		if _, err := io.Copy(t, src); err != nil {
 			t.Close()
 			src.Close()
 			err = fmt.Errorf("failed to copy values file '%s' to '%s: %w", chart.Spec.ValuesFile, chartutil.ValuesfileName, err)
-			return chart, err
+			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}
 		t.Close()
 		src.Close()
@@ -421,14 +489,14 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	// chartutil.Save method as the action performs a dependency check for us
 	pkg := action.NewPackage()
 	pkg.Destination = tmpDir
-	src, err := pkg.Run(chartPath, nil)
+	pkgPath, err := pkg.Run(chartPath, nil)
 	if err != nil {
 		err = fmt.Errorf("chart package error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 	}
 
 	// Copy the packaged chart to the artifact path
-	cf, err := os.Open(src)
+	cf, err := os.Open(pkgPath)
 	if err != nil {
 		err = fmt.Errorf("failed to open chart package: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err

From 7268c8b61d0b9feede89f1cf7a7984d49b7bfd4e Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Fri, 11 Sep 2020 12:16:41 +0200
Subject: [PATCH 3/4] Refactor and factor out chart values replacement

---
 controllers/helmchart_controller.go | 81 ++++++++---------------------
 internal/helm/chart.go              | 52 ++++++++++++++++++
 2 files changed, 75 insertions(+), 58 deletions(-)
 create mode 100644 internal/helm/chart.go

diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
index 09a1ff3f..1e6eced5 100644
--- a/controllers/helmchart_controller.go
+++ b/controllers/helmchart_controller.go
@@ -19,7 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -106,7 +105,8 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	}
 
 	// Conditionally set progressing condition in status
-	if resetChart, ok := r.resetStatus(chart); ok {
+	resetChart, changed := r.resetStatus(chart)
+	if changed {
 		chart = resetChart
 		if err := r.Status().Update(ctx, &chart); err != nil {
 			log.Error(err, "unable to update status")
@@ -132,7 +132,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 			}
 			return ctrl.Result{Requeue: true}, err
 		}
-		reconciledChart, reconcileErr = r.reconcileFromHelmRepository(ctx, repository, *chart.DeepCopy())
+		reconciledChart, reconcileErr = r.reconcileFromHelmRepository(ctx, repository, *chart.DeepCopy(), changed)
 	case sourcev1.GitRepositoryKind:
 		repository, err := r.getGitRepositoryWithArtifact(ctx, chart)
 		if err != nil {
@@ -142,7 +142,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 			}
 			return ctrl.Result{Requeue: true}, err
 		}
-		reconciledChart, reconcileErr = r.reconcileFromGitRepository(ctx, repository, *chart.DeepCopy())
+		reconciledChart, reconcileErr = r.reconcileFromGitRepository(ctx, repository, *chart.DeepCopy(), changed)
 	default:
 		err := fmt.Errorf("unable to reconcile unsupported source reference kind '%s'", chart.Spec.SourceRef.Kind)
 		return ctrl.Result{}, err
@@ -189,7 +189,7 @@ func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts
 }
 
 func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
-	repository sourcev1.HelmRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
+	repository sourcev1.HelmRepository, chart sourcev1.HelmChart, force bool) (sourcev1.HelmChart, error) {
 	cv, err := helm.GetDownloadableChartVersionFromIndex(r.Storage.LocalPath(*repository.GetArtifact()),
 		chart.Spec.Chart, chart.Spec.Version)
 	if err != nil {
@@ -199,7 +199,7 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 	// Return early if the revision is still the same as the current artifact
 	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), cv.Version,
 		fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version))
-	if repository.GetArtifact() != nil && repository.GetArtifact().Revision == cv.Version {
+	if !force && repository.GetArtifact() != nil && repository.GetArtifact().Revision == cv.Version {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
 			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
@@ -283,6 +283,12 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
 	}
 
+	// Either repackage the chart with the declared default values file,
+	// or write the chart directly to storage.
+	var (
+		readyReason  = sourcev1.ChartPullSucceededReason
+		readyMessage = fmt.Sprintf("Fetched revision: %s", artifact.Revision)
+	)
 	switch {
 	case chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName:
 		// Create temporary working directory
@@ -301,33 +307,11 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 
 		// Overwrite values file
 		chartPath := path.Join(tmpDir, cv.Name)
-		srcPath := path.Join(chartPath, chart.Spec.ValuesFile)
-		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
-			err = fmt.Errorf("invalid values file: %s", chart.Spec.ValuesFile)
+		if err := helm.OverwriteChartDefaultValues(chartPath, chart.Spec.ValuesFile); err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}
-		src, err := os.Open(srcPath)
-		if err != nil {
-			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
-		t, err := os.OpenFile(path.Join(chartPath, chartutil.ValuesfileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
-		if err != nil {
-			src.Close()
-			err = fmt.Errorf("failed to open default values: %w", err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
-		if _, err := io.Copy(t, src); err != nil {
-			t.Close()
-			src.Close()
-			err = fmt.Errorf("failed to overwrite default values with '%s: %w", chart.Spec.ValuesFile, err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
-		t.Close()
-		src.Close()
 
-		// Package the chart, we use the action here instead of relying on the
-		// chartutil.Save method as the action performs a dependency check for us
+		// Package the chart with the new default values
 		pkg := action.NewPackage()
 		pkg.Destination = tmpDir
 		pkgPath, err := pkg.Run(chartPath, nil)
@@ -348,6 +332,9 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 		}
 		cf.Close()
+
+		readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", artifact.Revision)
+		readyReason = sourcev1.ChartPackageSucceededReason
 	default:
 		// Write artifact to storage
 		if err := r.Storage.AtomicWriteFile(&artifact, res, 0644); err != nil {
@@ -363,8 +350,7 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	message := fmt.Sprintf("Fetched revision: %s", artifact.Revision)
-	return sourcev1.HelmChartReady(chart, artifact, chartUrl, sourcev1.ChartPullSucceededReason, message), nil
+	return sourcev1.HelmChartReady(chart, artifact, chartUrl, readyReason, readyMessage), nil
 }
 
 // getChartRepositoryWithArtifact attempts to get the v1alpha1.HelmRepository
@@ -388,14 +374,14 @@ func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context
 	}
 
 	if repository.GetArtifact() == nil {
-		err = fmt.Errorf("no repository index artifact found in HelmRepository '%s'", name)
+		err = fmt.Errorf("no repository index artifact found for HelmRepository '%s'", name)
 	}
 
 	return repository, err
 }
 
 func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
-	repository sourcev1.GitRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
+	repository sourcev1.GitRepository, chart sourcev1.HelmChart, force bool) (sourcev1.HelmChart, error) {
 	// Create temporary working directory
 	tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
 	if err != nil {
@@ -434,7 +420,7 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	// Return early if the revision is still the same as the current chart artifact
 	artifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version,
 		fmt.Sprintf("%s-%s.tgz", chartMetadata.Name, chartMetadata.Version))
-	if chart.GetArtifact() != nil && chart.GetArtifact().Revision == chartMetadata.Version {
+	if !force && chart.GetArtifact() != nil && chart.GetArtifact().Revision == chartMetadata.Version {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
 			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
@@ -443,31 +429,10 @@ func (r *HelmChartReconciler) reconcileFromGitRepository(ctx context.Context,
 	}
 
 	// Overwrite default values if instructed to
-	if chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName {
-		srcPath := path.Join(chartPath, chart.Spec.ValuesFile)
-		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
-			err = fmt.Errorf("invalid values file path: %s", chart.Spec.ValuesFile)
+	if chart.Spec.ValuesFile != "" {
+		if err := helm.OverwriteChartDefaultValues(chartPath, chart.Spec.ValuesFile); err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}
-		src, err := os.Open(srcPath)
-		if err != nil {
-			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
-		t, err := os.OpenFile(path.Join(tmpDir, chartutil.ValuesfileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
-		if err != nil {
-			src.Close()
-			err = fmt.Errorf("failed to open values file '%s': %w", chartutil.ValuesfileName, err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
-		if _, err := io.Copy(t, src); err != nil {
-			t.Close()
-			src.Close()
-			err = fmt.Errorf("failed to copy values file '%s' to '%s: %w", chart.Spec.ValuesFile, chartutil.ValuesfileName, err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
-		t.Close()
-		src.Close()
 	}
 
 	// Ensure artifact directory exists
diff --git a/internal/helm/chart.go b/internal/helm/chart.go
new file mode 100644
index 00000000..1ee16328
--- /dev/null
+++ b/internal/helm/chart.go
@@ -0,0 +1,52 @@
+/*
+Copyright 2020 The Flux CD contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package helm
+
+import (
+	"fmt"
+	"io"
+	"os"
+	"path"
+
+	"helm.sh/helm/v3/pkg/chartutil"
+)
+
+// OverwriteChartDefaultValues overwrites the chart default values file in the
+// given chartPath with the contents of the given valuesFile.
+func OverwriteChartDefaultValues(chartPath, valuesFile string) error {
+	if valuesFile == chartutil.ValuesfileName {
+		return nil
+	}
+	srcPath := path.Join(chartPath, valuesFile)
+	if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
+		return fmt.Errorf("invalid values file path: %s", valuesFile)
+	}
+	src, err := os.Open(srcPath)
+	if err != nil {
+		return fmt.Errorf("failed to open values file '%s': %w", valuesFile, err)
+	}
+	defer src.Close()
+	t, err := os.OpenFile(path.Join(chartPath, chartutil.ValuesfileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
+	if err != nil {
+		return fmt.Errorf("failed to open values file '%s': %w", chartutil.ValuesfileName, err)
+	}
+	defer t.Close()
+	if _, err := io.Copy(t, src); err != nil {
+		return fmt.Errorf("failed to overwrite default values with '%s': %w", valuesFile, err)
+	}
+	return nil
+}

From eaec41f6e92b3adc893c3bbd9213091f3017d513 Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Fri, 11 Sep 2020 13:18:34 +0200
Subject: [PATCH 4/4] e2e: test HelmChart ValuesFile

---
 .github/workflows/e2e.yaml                           |  8 +++++++-
 .../source_v1alpha1_helmchart_gitrepository.yaml     |  1 -
 .../testdata/helmchart-valuesfile/gitrepository.yaml | 12 ++++++++++++
 .../helmchart_gitrepository.yaml                     | 11 +++++++++++
 .../helmchart_helmrepository.yaml                    | 11 +++++++++++
 .../helmchart-valuesfile/helmrepository.yaml         |  7 +++++++
 6 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 config/testdata/helmchart-valuesfile/gitrepository.yaml
 create mode 100644 config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml
 create mode 100644 config/testdata/helmchart-valuesfile/helmchart_helmrepository.yaml
 create mode 100644 config/testdata/helmchart-valuesfile/helmrepository.yaml

diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml
index c0fce8fd..70be4e51 100644
--- a/.github/workflows/e2e.yaml
+++ b/.github/workflows/e2e.yaml
@@ -57,7 +57,13 @@ jobs:
           kubectl -n source-system wait gitrepository/gitrepository-sample --for=condition=ready --timeout=1m
           kubectl -n source-system wait helmrepository/helmrepository-sample --for=condition=ready --timeout=1m
           kubectl -n source-system wait helmchart/helmchart-sample --for=condition=ready --timeout=1m
-          kubectl -n source-system logs deploy/source-controller
+          kubectl -n source-system delete -f ./config/samples
+      - name: Run HelmChart values file tests
+        run: |
+          kubectl -n source-system apply -f ./config/testdata/helmchart-valuesfile
+          kubectl -n source-system wait helmchart/mariadb --for=condition=ready --timeout=5m
+          kubectl -n source-system wait helmchart/mariadb-git --for=condition=ready --timeout=5m
+          kubectl -n source-system delete -f ./config/testdata/helmchart-valuesfile
       - name: Debug failure
         if: failure()
         run: |
diff --git a/config/samples/source_v1alpha1_helmchart_gitrepository.yaml b/config/samples/source_v1alpha1_helmchart_gitrepository.yaml
index 5cb5feef..5e8fec85 100644
--- a/config/samples/source_v1alpha1_helmchart_gitrepository.yaml
+++ b/config/samples/source_v1alpha1_helmchart_gitrepository.yaml
@@ -4,7 +4,6 @@ metadata:
   name: helmchart-git-sample
 spec:
   chart: charts/podinfo
-  version: '^2.0.0'
   sourceRef:
     kind: GitRepository
     name: gitrepository-sample
diff --git a/config/testdata/helmchart-valuesfile/gitrepository.yaml b/config/testdata/helmchart-valuesfile/gitrepository.yaml
new file mode 100644
index 00000000..a72ff950
--- /dev/null
+++ b/config/testdata/helmchart-valuesfile/gitrepository.yaml
@@ -0,0 +1,12 @@
+apiVersion: source.toolkit.fluxcd.io/v1alpha1
+kind: GitRepository
+metadata:
+  name: bitnami-charts
+spec:
+  interval: 1m
+  url: https://github.com/bitnami/charts
+  ref:
+    branch: master
+  ignore:
+    /*
+    !/bitnami/
diff --git a/config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml b/config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml
new file mode 100644
index 00000000..f6e85fd8
--- /dev/null
+++ b/config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml
@@ -0,0 +1,11 @@
+apiVersion: source.toolkit.fluxcd.io/v1alpha1
+kind: HelmChart
+metadata:
+  name: mariadb-git
+spec:
+  chart: bitnami/mariadb
+  valuesFile: values-production.yaml
+  sourceRef:
+    kind: GitRepository
+    name: bitnami-charts
+  interval: 1m
diff --git a/config/testdata/helmchart-valuesfile/helmchart_helmrepository.yaml b/config/testdata/helmchart-valuesfile/helmchart_helmrepository.yaml
new file mode 100644
index 00000000..fa479fab
--- /dev/null
+++ b/config/testdata/helmchart-valuesfile/helmchart_helmrepository.yaml
@@ -0,0 +1,11 @@
+apiVersion: source.toolkit.fluxcd.io/v1alpha1
+kind: HelmChart
+metadata:
+  name: mariadb
+spec:
+  chart: mariadb
+  valuesFile: values-production.yaml
+  sourceRef:
+    kind: HelmRepository
+    name: bitnami-charts
+  interval: 1m
diff --git a/config/testdata/helmchart-valuesfile/helmrepository.yaml b/config/testdata/helmchart-valuesfile/helmrepository.yaml
new file mode 100644
index 00000000..432e3e3c
--- /dev/null
+++ b/config/testdata/helmchart-valuesfile/helmrepository.yaml
@@ -0,0 +1,7 @@
+apiVersion: source.toolkit.fluxcd.io/v1alpha1
+kind: HelmRepository
+metadata:
+  name: bitnami-charts
+spec:
+  interval: 1m
+  url: https://charts.bitnami.com/bitnami