Merge pull request #890 from staebler/asset_loading_tests

 assets: add tests for validating asset fetching of targets

cmd/openshift-install/create.go

@@ -26,14 +26,8 @@ import (
 	configclient "github.com/openshift/client-go/config/clientset/versioned"
 	routeclient "github.com/openshift/client-go/route/clientset/versioned"
 	"github.com/openshift/installer/pkg/asset"
-	"github.com/openshift/installer/pkg/asset/cluster"
-	"github.com/openshift/installer/pkg/asset/ignition/bootstrap"
-	"github.com/openshift/installer/pkg/asset/ignition/machine"
-	"github.com/openshift/installer/pkg/asset/installconfig"
-	"github.com/openshift/installer/pkg/asset/kubeconfig"
-	"github.com/openshift/installer/pkg/asset/manifests"
-	"github.com/openshift/installer/pkg/asset/templates"
-	"github.com/openshift/installer/pkg/asset/tls"
+	assetstore "github.com/openshift/installer/pkg/asset/store"
+	targetassets "github.com/openshift/installer/pkg/asset/targets"
 	destroybootstrap "github.com/openshift/installer/pkg/destroy/bootstrap"
 	cov1helpers "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers"
 )
@@ -55,7 +49,7 @@ var (
 			// FIXME: add longer descriptions for our commands with examples for better UX.
 			// Long:  "",
 		},
-		assets: []asset.WritableAsset{&installconfig.InstallConfig{}},
+		assets: targetassets.InstallConfig,
 	}
 
 	manifestsTarget = target{
@@ -66,7 +60,7 @@ var (
 			// FIXME: add longer descriptions for our commands with examples for better UX.
 			// Long:  "",
 		},
-		assets: []asset.WritableAsset{&manifests.Manifests{}, &manifests.Openshift{}},
+		assets: targetassets.Manifests,
 	}
 
 	manifestTemplatesTarget = target{
@@ -76,7 +70,7 @@ var (
 			Short: "Generates the unrendered Kubernetes manifest templates",
 			Long:  "",
 		},
-		assets: []asset.WritableAsset{&templates.Templates{}},
+		assets: targetassets.ManifestTemplates,
 	}
 
 	ignitionConfigsTarget = target{
@@ -87,7 +81,7 @@ var (
 			// FIXME: add longer descriptions for our commands with examples for better UX.
 			// Long:  "",
 		},
-		assets: []asset.WritableAsset{&bootstrap.Bootstrap{}, &machine.Master{}, &machine.Worker{}, &kubeconfig.Admin{}, &cluster.Metadata{}},
+		assets: targetassets.IgnitionConfigs,
 	}
 
 	clusterTarget = target{
@@ -128,7 +122,7 @@ var (
 				}
 			},
 		},
-		assets: []asset.WritableAsset{&cluster.TerraformVariables{}, &kubeconfig.Admin{}, &tls.JournalCertKey{}, &cluster.Metadata{}, &cluster.Cluster{}},
+		assets: targetassets.Cluster,
 	}
 
 	targets = []target{installConfigTarget, manifestTemplatesTarget, manifestsTarget, ignitionConfigsTarget, clusterTarget}
@@ -154,7 +148,7 @@ func newCreateCmd() *cobra.Command {
 
 func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args []string) {
 	runner := func(directory string) error {
-		assetStore, err := asset.NewStore(directory)
+		assetStore, err := assetstore.NewStore(directory)
 		if err != nil {
 			return errors.Wrapf(err, "failed to create asset store")
 		}

cmd/openshift-install/destroy.go

@@ -5,7 +5,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 
-	"github.com/openshift/installer/pkg/asset"
+	assetstore "github.com/openshift/installer/pkg/asset/store"
 	"github.com/openshift/installer/pkg/destroy"
 	"github.com/openshift/installer/pkg/destroy/bootstrap"
 	_ "github.com/openshift/installer/pkg/destroy/libvirt"
@@ -52,7 +52,7 @@ func runDestroyCmd(directory string) error {
 		return errors.Wrap(err, "Failed to destroy cluster")
 	}
 
-	store, err := asset.NewStore(directory)
+	store, err := assetstore.NewStore(directory)
 	if err != nil {
 		return errors.Wrapf(err, "failed to create asset store")
 	}

pkg/asset/asset.go

@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sort"
 
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -58,9 +59,9 @@ func PersistToFile(asset WritableAsset, directory string) error {
 	return nil
 }
 
-// deleteAssetFromDisk removes all the files for asset from disk.
+// DeleteAssetFromDisk removes all the files for asset from disk.
 // this is function is not safe for calling concurrently on the same directory.
-func deleteAssetFromDisk(asset WritableAsset, directory string) error {
+func DeleteAssetFromDisk(asset WritableAsset, directory string) error {
 	logrus.Debugf("Purging asset %q from disk", asset.Name())
 	for _, f := range asset.Files() {
 		path := filepath.Join(directory, f.Filename)
@@ -95,3 +96,8 @@ func isDirEmpty(name string) (bool, error) {
 	}
 	return false, err // Either not empty or error, suits both cases
 }
+
+// SortFiles sorts the specified files by file name.
+func SortFiles(files []*File) {
+	sort.Slice(files, func(i, j int) bool { return files[i].Filename < files[j].Filename })
+}

pkg/asset/filefetcher.go

@@ -1,11 +1,5 @@
 package asset
 
-import (
-	"io/ioutil"
-	"path/filepath"
-	"sort"
-)
-
 //go:generate mockgen -source=./filefetcher.go -destination=./mock/filefetcher_generated.go -package=mock
 
 // FileFetcher fetches the asset files from disk.
@@ -15,45 +9,3 @@ type FileFetcher interface {
 	// FetchByPattern returns the files whose name match the given glob.
 	FetchByPattern(pattern string) ([]*File, error)
 }
-
-type fileFetcher struct {
-	directory string
-}
-
-// FetchByName returns the file with the given name.
-func (f *fileFetcher) FetchByName(name string) (*File, error) {
-	data, err := ioutil.ReadFile(filepath.Join(f.directory, name))
-	if err != nil {
-		return nil, err
-	}
-	return &File{Filename: name, Data: data}, nil
-}
-
-// FetchByPattern returns the files whose name match the given regexp.
-func (f *fileFetcher) FetchByPattern(pattern string) (files []*File, err error) {
-	matches, err := filepath.Glob(filepath.Join(f.directory, pattern))
-	if err != nil {
-		return nil, err
-	}
-
-	files = make([]*File, 0, len(matches))
-	for _, path := range matches {
-		data, err := ioutil.ReadFile(path)
-		if err != nil {
-			return nil, err
-		}
-
-		filename, err := filepath.Rel(f.directory, path)
-		if err != nil {
-			return nil, err
-		}
-
-		files = append(files, &File{
-			Filename: filename,
-			Data:     data,
-		})
-	}
-
-	sort.Slice(files, func(i, j int) bool { return files[i].Filename < files[j].Filename })
-	return files, nil
-}

pkg/asset/machines/master.go

@@ -111,6 +111,10 @@ func (m *Master) Generate(dependencies asset.Parents) error {
 		}
 		m.MachinesRaw = raw
 	case nonetypes.Name:
+		// This is needed to ensure that roundtrip generate-load tests pass when
+		// comparing this value. Otherwise, generate will use a nil value while
+		// load will use an empty byte slice.
+		m.MachinesRaw = []byte{}
 	case openstacktypes.Name:
 		mpool := defaultOpenStackMachinePoolPlatform(ic.Platform.OpenStack.FlavorName)
 		mpool.Set(ic.Platform.OpenStack.DefaultMachinePlatform)

pkg/asset/machines/worker.go

@@ -131,6 +131,10 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
 		}
 		w.MachineSetRaw = raw
 	case nonetypes.Name:
+		// This is needed to ensure that roundtrip generate-load tests pass when
+		// comparing this value. Otherwise, generate will use a nil value while
+		// load will use an empty byte slice.
+		w.MachineSetRaw = []byte{}
 	case openstacktypes.Name:
 		mpool := defaultOpenStackMachinePoolPlatform(ic.Platform.OpenStack.FlavorName)
 		mpool.Set(ic.Platform.OpenStack.DefaultMachinePlatform)

pkg/asset/manifests/dns.go

@@ -1,7 +1,6 @@
 package manifests
 
 import (
-	"os"
 	"path/filepath"
 
 	"github.com/ghodss/yaml"
@@ -22,7 +21,6 @@ var (
 
 // DNS generates the cluster-dns-*.yml files.
 type DNS struct {
-	config   *configv1.DNS
 	FileList []*asset.File
 }
 
@@ -46,7 +44,7 @@ func (d *DNS) Generate(dependencies asset.Parents) error {
 	installConfig := &installconfig.InstallConfig{}
 	dependencies.Get(installConfig)
 
-	d.config = &configv1.DNS{
+	config := &configv1.DNS{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: configv1.SchemeGroupVersion.String(),
 			Kind:       "DNS",
@@ -60,7 +58,7 @@ func (d *DNS) Generate(dependencies asset.Parents) error {
 		},
 	}
 
-	configData, err := yaml.Marshal(d.config)
+	configData, err := yaml.Marshal(config)
 	if err != nil {
 		return errors.Wrapf(err, "failed to create %s manifests from InstallConfig", d.Name())
 	}
@@ -91,31 +89,5 @@ func (d *DNS) Files() []*asset.File {
 
 // Load loads the already-rendered files back from disk.
 func (d *DNS) Load(f asset.FileFetcher) (bool, error) {
-	crdFile, err := f.FetchByName(filepath.Join(manifestDir, dnsCrdFilename))
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, err
-	}
-
-	cfgFile, err := f.FetchByName(dnsCfgFilename)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-
-		return false, err
-	}
-
-	dnsConfig := &configv1.DNS{}
-	if err := yaml.Unmarshal(cfgFile.Data, dnsConfig); err != nil {
-		return false, errors.Wrapf(err, "failed to unmarshal %s", dnsCfgFilename)
-	}
-
-	fileList := []*asset.File{crdFile, cfgFile}
-
-	d.FileList, d.config = fileList, dnsConfig
-
-	return true, nil
+	return false, nil
 }

pkg/asset/manifests/ingress.go

@@ -2,7 +2,6 @@ package manifests
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 
 	"github.com/ghodss/yaml"
@@ -23,7 +22,6 @@ var (
 
 // Ingress generates the cluster-ingress-*.yml files.
 type Ingress struct {
-	config   *configv1.Ingress
 	FileList []*asset.File
 }
 
@@ -47,7 +45,7 @@ func (ing *Ingress) Generate(dependencies asset.Parents) error {
 	installConfig := &installconfig.InstallConfig{}
 	dependencies.Get(installConfig)
 
-	ing.config = &configv1.Ingress{
+	config := &configv1.Ingress{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: configv1.SchemeGroupVersion.String(),
 			Kind:       "Ingress",
@@ -61,7 +59,7 @@ func (ing *Ingress) Generate(dependencies asset.Parents) error {
 		},
 	}
 
-	configData, err := yaml.Marshal(ing.config)
+	configData, err := yaml.Marshal(config)
 	if err != nil {
 		return errors.Wrapf(err, "failed to create %s manifests from InstallConfig", ing.Name())
 	}
@@ -90,33 +88,7 @@ func (ing *Ingress) Files() []*asset.File {
 	return ing.FileList
 }
 
-// Load loads the already-rendered files back from disk.
+// Load returns false since this asset is not written to disk by the installer.
 func (ing *Ingress) Load(f asset.FileFetcher) (bool, error) {
-	crdFile, err := f.FetchByName(filepath.Join(manifestDir, ingCrdFilename))
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, err
-	}
-
-	cfgFile, err := f.FetchByName(ingCfgFilename)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-
-		return false, err
-	}
-
-	ingressConfig := &configv1.Ingress{}
-	if err := yaml.Unmarshal(cfgFile.Data, ingressConfig); err != nil {
-		return false, errors.Wrapf(err, "failed to unmarshal %s", ingCfgFilename)
-	}
-
-	fileList := []*asset.File{crdFile, cfgFile}
-
-	ing.FileList, ing.config = fileList, ingressConfig
-
-	return true, nil
+	return false, nil
 }