Skip to content

assets: add tests for validating asset fetching of targets #890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 7 commits into from
Feb 4, 2019
Merged

assets: add tests for validating asset fetching of targets #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd375cd
asset/manifest: do not load Common Manifest dependent manifests from …
staebler Dec 11, 2018
b25db3d
asset/templates: remove fileName field
staebler Dec 12, 2018
a7fddff
assets/templates: remove common templates asset
staebler Dec 12, 2018
c6f6449
assets: move asset store impl to own package
staebler Dec 12, 2018
75ab106
assets: add tests for validating asset fetching of targets
staebler Dec 12, 2018
ef1bae2
assets/manifests: sort files list in manifests assets
staebler Dec 12, 2018
17a3965
docs: rebuild dependency graph
staebler Jan 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions cmd/openshift-install/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -76,7 +70,7 @@ var (
Short: "Generates the unrendered Kubernetes manifest templates",
Long: "",
},
assets: []asset.WritableAsset{&templates.Templates{}},
assets: targetassets.ManifestTemplates,
}

ignitionConfigsTarget = target{
Expand All @@ -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{
Expand Down Expand Up @@ -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}
Expand All @@ -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")
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/openshift-install/destroy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}
Expand Down
1,306 changes: 728 additions & 578 deletions docs/design/resource_dep.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 8 additions & 2 deletions pkg/asset/asset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"sort"

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 })
}
48 changes: 0 additions & 48 deletions pkg/asset/filefetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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
}
4 changes: 4 additions & 0 deletions pkg/asset/machines/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions pkg/asset/machines/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
34 changes: 3 additions & 31 deletions pkg/asset/manifests/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package manifests

import (
"os"
"path/filepath"

"github.com/ghodss/yaml"
Expand All @@ -22,7 +21,6 @@ var (

// DNS generates the cluster-dns-*.yml files.
type DNS struct {
config *configv1.DNS
FileList []*asset.File
}

Expand All @@ -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",
Expand All @@ -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())
}
Expand Down Expand Up @@ -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) {
Copy link
Member

@wking wking Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why gut this method instead of removing it completely? Can't we get away with just Files() for use in (*Manifests).Generate(...) and turn *DNS from a WritableAsset into an Asset? Maybe we want to split interfaces:

// WritableAsset is an Asset that has files that can be written to disk.
type WritableAsset interface {
  Asset

  // Files returns the files to write.
  Files() []*File
}

// ReadWriteableAsset is a WriteableAsset that can be loaded from disk.
type ReadWritableAsset interface {
  WriteableAsset

  // Load returns the on-disk asset if it exists.
  // The asset object should be changed only when it's loaded successfully.
  Load(FileFetcher) (found bool, err error)
}

Copy link
Contributor Author

@staebler staebler Jan 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had those separate interfaces when I wrote the asset store originally. I'll have to dig back into past PRs to refresh my memory about who had what objections to that approach.

Copy link
Contributor Author

@staebler staebler Jan 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to pass on making any change along these lines for this PR. I agree with you that it is ugly to have asset with Load functions that do nothing. However, the problem is more widespread than just these manifest assets. I think that changes that we make to remove unused Load functions are worthy of their own PR.

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
}
36 changes: 4 additions & 32 deletions pkg/asset/manifests/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package manifests

import (
"fmt"
"os"
"path/filepath"

"github.com/ghodss/yaml"
Expand All @@ -23,7 +22,6 @@ var (

// Ingress generates the cluster-ingress-*.yml files.
type Ingress struct {
config *configv1.Ingress
FileList []*asset.File
}

Expand All @@ -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",
Expand All @@ -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())
}
Expand Down Expand Up @@ -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
}
Loading