Merge pull request #154 from fluxcd/small-refactors

hiddeco · web-flow · commit c1fb9b7fa530 · 2020-09-22T12:42:37.000+02:00
diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml
@@ -67,10 +67,13 @@ jobs:
           kubectl -n source-system wait helmchart/mariadb-git --for=condition=ready --timeout=5m
           kubectl -n source-system delete -f ./config/testdata/helmchart-valuesfile
       - name: Setup Minio
+        env:
+          MINIO_VER: ${{ 'v6.3.1' }}
         run: |
           kubectl create ns minio
           helm repo add minio https://helm.min.io/
           helm upgrade --wait -i minio minio/minio \
+          --version $MINIO_VER \
           --namespace minio \
           --set accessKey=myaccesskey \
           --set secretKey=mysecretkey \
diff --git a/api/v1alpha1/artifact.go b/api/v1alpha1/artifact.go
@@ -49,6 +49,15 @@ type Artifact struct {
 	LastUpdateTime metav1.Time `json:"lastUpdateTime,omitempty"`
 }
 
+// HasRevision returns true if the given revision matches the current
+// Revision of the Artifact.
+func (in *Artifact) HasRevision(revision string) bool {
+	if in == nil {
+		return false
+	}
+	return in.Revision == revision
+}
+
 // ArtifactDir returns the artifact dir path in the form of
 // <source-kind>/<source-namespace>/<source-name>.
 func ArtifactDir(kind, namespace, name string) string {
diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go
@@ -211,7 +211,7 @@ func (r *BucketReconciler) reconcile(ctx context.Context, bucket sourcev1.Bucket
 
 	// return early on unchanged revision
 	artifact := r.Storage.NewArtifactFor(bucket.Kind, bucket.GetObjectMeta(), revision, fmt.Sprintf("%s.tar.gz", revision))
-	if bucket.GetArtifact() != nil && bucket.GetArtifact().Revision == revision {
+	if bucket.GetArtifact().HasRevision(artifact.Revision) {
 		if artifact.URL != bucket.GetArtifact().URL {
 			r.Storage.SetArtifactURL(bucket.GetArtifact())
 			bucket.Status.URL = r.Storage.SetHostname(bucket.Status.URL)
diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go
@@ -198,7 +198,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, repository sour
 
 	// return early on unchanged revision
 	artifact := r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), revision, fmt.Sprintf("%s.tar.gz", commit.Hash.String()))
-	if repository.GetArtifact() != nil && repository.GetArtifact().Revision == revision {
+	if repository.GetArtifact().HasRevision(artifact.Revision) {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
 			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
@@ -238,12 +238,12 @@ 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,
+	newArtifact := r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), cv.Version,
 		fmt.Sprintf("%s-%s.tgz", cv.Name, cv.Version))
-	if !force && repository.GetArtifact() != nil && repository.GetArtifact().Revision == cv.Version {
-		if artifact.URL != repository.GetArtifact().URL {
+	if !force && repository.GetArtifact().HasRevision(newArtifact.Revision) {
+		if newArtifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(chart.GetArtifact())
-			repository.Status.URL = r.Storage.SetHostname(chart.Status.URL)
+			chart.Status.URL = r.Storage.SetHostname(chart.Status.URL)
 		}
 		return chart, nil
 	}
@@ -296,21 +296,19 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 			err = fmt.Errorf("auth options error: %w", err)
 			return sourcev1.HelmChartNotReady(chart, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
-		if cleanup != nil {
-			defer cleanup()
-		}
+		defer cleanup()
 		clientOpts = opts
 	}
 
 	// Ensure artifact directory exists
-	err = r.Storage.MkdirAll(artifact)
+	err = r.Storage.MkdirAll(newArtifact)
 	if err != nil {
 		err = fmt.Errorf("unable to create chart directory: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
 	// Acquire a lock for the artifact
-	unlock, err := r.Storage.Lock(artifact)
+	unlock, err := r.Storage.Lock(newArtifact)
 	if err != nil {
 		err = fmt.Errorf("unable to acquire lock: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -328,7 +326,7 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 	// or write the chart directly to storage.
 	var (
 		readyReason  = sourcev1.ChartPullSucceededReason
-		readyMessage = fmt.Sprintf("Fetched revision: %s", artifact.Revision)
+		readyMessage = fmt.Sprintf("Fetched revision: %s", newArtifact.Revision)
 	)
 	switch {
 	case chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName:
@@ -362,36 +360,29 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		}
 
 		// 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)
+		if err := r.Storage.CopyFromPath(&newArtifact, pkgPath); err != nil {
+			err = fmt.Errorf("failed to write chart package to storage: %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()
 
-		readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", artifact.Revision)
+		readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", newArtifact.Revision)
 		readyReason = sourcev1.ChartPackageSucceededReason
 	default:
 		// Write artifact to storage
-		if err := r.Storage.AtomicWriteFile(&artifact, res, 0644); err != nil {
+		if err := r.Storage.AtomicWriteFile(&newArtifact, 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
-	chartUrl, err := r.Storage.Symlink(artifact, fmt.Sprintf("%s-latest.tgz", cv.Name))
+	chartUrl, err := r.Storage.Symlink(newArtifact, fmt.Sprintf("%s-latest.tgz", cv.Name))
 	if err != nil {
 		err = fmt.Errorf("storage error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	return sourcev1.HelmChartReady(chart, artifact, chartUrl, readyReason, readyMessage), nil
+	return sourcev1.HelmChartReady(chart, newArtifact, chartUrl, readyReason, readyMessage), nil
 }
 
 func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
@@ -432,32 +423,30 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 	}
 
 	// Return early if the revision is still the same as the current chart artifact
-	chartArtifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version,
+	newArtifact := r.Storage.NewArtifactFor(chart.Kind, chart.ObjectMeta.GetObjectMeta(), chartMetadata.Version,
 		fmt.Sprintf("%s-%s.tgz", chartMetadata.Name, chartMetadata.Version))
-	if !force && chart.GetArtifact() != nil && chart.GetArtifact().Revision == chartMetadata.Version {
-		if chartArtifact.URL != artifact.URL {
-			r.Storage.SetArtifactURL(&chartArtifact)
+	if !force && chart.GetArtifact().HasRevision(newArtifact.Revision) {
+		if newArtifact.URL != artifact.URL {
+			r.Storage.SetArtifactURL(&newArtifact)
 			chart.Status.URL = r.Storage.SetHostname(chart.Status.URL)
 		}
 		return chart, nil
 	}
 
-	// Overwrite default values if instructed to
-	if chart.Spec.ValuesFile != "" {
-		if err := helm.OverwriteChartDefaultValues(chartPath, chart.Spec.ValuesFile); err != nil {
-			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
-		}
+	// Overwrite default values if configured
+	if err := helm.OverwriteChartDefaultValues(chartPath, chart.Spec.ValuesFile); err != nil {
+		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 	}
 
 	// Ensure artifact directory exists
-	err = r.Storage.MkdirAll(chartArtifact)
+	err = r.Storage.MkdirAll(newArtifact)
 	if err != nil {
 		err = fmt.Errorf("unable to create artifact directory: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
 	// Acquire a lock for the artifact
-	unlock, err := r.Storage.Lock(chartArtifact)
+	unlock, err := r.Storage.Lock(newArtifact)
 	if err != nil {
 		err = fmt.Errorf("unable to acquire lock: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -475,27 +464,20 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 	}
 
 	// 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(&chartArtifact, cf); err != nil {
-		cf.Close()
-		err = fmt.Errorf("failed to copy chart package to storage: %w", err)
+	if err := r.Storage.CopyFromPath(&newArtifact, pkgPath); err != nil {
+		err = fmt.Errorf("failed to write chart package to storage: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
-	cf.Close()
 
 	// Update symlink
-	cUrl, err := r.Storage.Symlink(chartArtifact, fmt.Sprintf("%s-latest.tgz", chartMetadata.Name))
+	cUrl, err := r.Storage.Symlink(newArtifact, fmt.Sprintf("%s-latest.tgz", chartMetadata.Name))
 	if err != nil {
 		err = fmt.Errorf("storage error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
-	message := fmt.Sprintf("Fetched and packaged revision: %s", chartArtifact.Revision)
-	return sourcev1.HelmChartReady(chart, chartArtifact, cUrl, sourcev1.ChartPackageSucceededReason, message), nil
+	message := fmt.Sprintf("Fetched and packaged revision: %s", newArtifact.Revision)
+	return sourcev1.HelmChartReady(chart, newArtifact, cUrl, sourcev1.ChartPackageSucceededReason, message), nil
 }
 
 // resetStatus returns a modified v1alpha1.HelmChart and a boolean indicating
diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go
@@ -195,9 +195,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repository sou
 			err = fmt.Errorf("auth options error: %w", err)
 			return sourcev1.HelmRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
-		if cleanup != nil {
-			defer cleanup()
-		}
+		defer cleanup()
 		clientOpts = opts
 	}
 
@@ -219,7 +217,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repository sou
 	// return early on unchanged generation
 	artifact := r.Storage.NewArtifactFor(repository.Kind, repository.ObjectMeta.GetObjectMeta(), i.Generated.Format(time.RFC3339Nano),
 		fmt.Sprintf("index-%s.yaml", url.PathEscape(i.Generated.Format(time.RFC3339Nano))))
-	if repository.GetArtifact() != nil && repository.GetArtifact().Revision == i.Generated.Format(time.RFC3339Nano) {
+	if repository.GetArtifact().HasRevision(artifact.Revision) {
 		if artifact.URL != repository.GetArtifact().URL {
 			r.Storage.SetArtifactURL(repository.GetArtifact())
 			repository.Status.URL = r.Storage.SetHostname(repository.Status.URL)
diff --git a/controllers/storage.go b/controllers/storage.go
@@ -339,6 +339,18 @@ func (s *Storage) Copy(artifact *sourcev1.Artifact, reader io.Reader) (err error
 	return nil
 }
 
+// CopyFromPath atomically copies the contents of the given path to the path of
+// the v1alpha1.Artifact. If successful, the checksum and last update time on the
+// artifact is set.
+func (s *Storage) CopyFromPath(artifact *sourcev1.Artifact, path string) (err error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	return s.Copy(artifact, f)
+}
+
 // Symlink creates or updates a symbolic link for the given v1alpha1.Artifact
 // and returns the URL for the symlink.
 func (s *Storage) Symlink(artifact sourcev1.Artifact, linkName string) (string, error) {
diff --git a/internal/helm/chart.go b/internal/helm/chart.go
@@ -28,7 +28,7 @@ import (
 // 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 {
+	if valuesFile == "" || valuesFile == chartutil.ValuesfileName {
 		return nil
 	}
 	srcPath := path.Join(chartPath, valuesFile)
diff --git a/internal/helm/getter.go b/internal/helm/getter.go
@@ -60,7 +60,7 @@ func TLSClientConfigFromSecret(secret corev1.Secret) (getter.Option, func(), err
 	certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"]
 	switch {
 	case len(certBytes)+len(keyBytes)+len(caBytes) == 0:
-		return nil, nil, nil
+		return nil, func() {}, nil
 	case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0):
 		return nil, nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence",
 			secret.Name)

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ import (`
`28`	`28`	`// OverwriteChartDefaultValues overwrites the chart default values file in the`
`29`	`29`	`// given chartPath with the contents of the given valuesFile.`
`30`	`30`	`func OverwriteChartDefaultValues(chartPath, valuesFile string) error {`
`31`		`- if valuesFile == chartutil.ValuesfileName {`
	`31`	`+ if valuesFile == "" \|\| valuesFile == chartutil.ValuesfileName {`
`32`	`32`	`return nil`
`33`	`33`	`}`
`34`	`34`	`srcPath := path.Join(chartPath, valuesFile)`