Merge pull request #289 from seans3/kinflate-errors

Checkpoint toward better kinflate error messages.

pkg/kinflate/commands/inflate.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/kubectl/pkg/kinflate/tree"
 	"k8s.io/kubectl/pkg/kinflate/types"
 	kutil "k8s.io/kubectl/pkg/kinflate/util"
+	"k8s.io/kubectl/pkg/kinflate/util/fs"
 )
 
 type inflateOptions struct {
@@ -82,7 +83,8 @@ func (o *inflateOptions) Complete(cmd *cobra.Command, args []string) error {
 // RunKinflate runs inflate command (do real work).
 func (o *inflateOptions) RunKinflate(out, errOut io.Writer) error {
 	// Build a tree of ManifestData.
-	root, err := tree.LoadManifestDataFromPath(o.manifestPath)
+	loader := tree.Loader{FS: fs.MakeRealFS(), InitialPath: o.manifestPath}
+	root, err := loader.LoadManifestDataFromPath()
 	if err != nil {
 		return err
 	}

pkg/kinflate/tree/builder.go

@@ -18,7 +18,6 @@ package tree
 
 import (
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 
@@ -29,31 +28,55 @@ import (
 	"k8s.io/kubectl/pkg/kinflate/util/fs"
 )
 
-// LoadManifestDataFromPath takes a path to a Kube-manifest.yaml or a dir that has a Kube-manifest.yaml.
+type Loader struct {
-// It returns a tree of ManifestData.
+	// Allows unit tests with fake filesystem.
-func LoadManifestDataFromPath(path string) (*ManifestData, error) {
+	FS fs.FileSystem
-	return loadManifestDataFromPath(path)
+	// Unexpanded manifest directory or manifest filename.
+	// Examples: "." or "sean-manifest.yaml"
+	InitialPath string
+	// Full expanded manifest file path.
+	// Examples:  "/usr/local/Kube-manifest.yaml" or "/home/seans/project/sean-manifest.yaml"
+	FullFilePath string
 }
 
-// loadManifestDataFromPath make a ManifestData from path
+// LoadManifestDataFromPath takes a path to a Kube-manifest.yaml or a dir that has a Kube-manifest.yaml.
-func loadManifestDataFromPath(path string) (*ManifestData, error) {
+// It returns a tree of ManifestData.
-	m, err := loadManifestFileFromPath(path)
+func (l *Loader) LoadManifestDataFromPath() (*ManifestData, error) {
+	m, err := l.loadManifestFileFromPath()
 	if err != nil {
 		return nil, err
 	}
-	return manifestToManifestData(m)
+	return l.manifestToManifestData(m)
+}
+
+// loadManifestFileFromPath loads a manifest object from file.
+func (l *Loader) loadManifestFileFromPath() (*manifest.Manifest, error) {
+	mLoader := kutil.ManifestLoader{FS: l.FS}
+	// Expand the initial directory or file path into the full manifest file path.
+	fullFilepath, err := mLoader.MakeValidManifestPath(l.InitialPath)
+	if err != nil {
+		return nil, err
+	}
+	l.FullFilePath = fullFilepath
+	m, err := mLoader.Read(fullFilepath)
+	if err != nil {
+		return nil, err
+	}
+	mLoader.Validate(m)
+	return m, err
 }
 
 // manifestToManifestData make a ManifestData given an Manifest object
-func manifestToManifestData(m *manifest.Manifest) (*ManifestData, error) {
+func (l *Loader) manifestToManifestData(m *manifest.Manifest) (*ManifestData, error) {
-	mdata, err := loadManifestDataFromManifestFileAndResources(m)
+	mdata, err := l.loadManifestDataFromManifestFileAndResources(m)
 	if err != nil {
 		return nil, err
 	}
 
 	pkgs := []*ManifestData{}
 	for _, pkg := range m.Packages {
-		pkgNode, err := loadManifestDataFromPath(pkg)
+		loader := &Loader{FS: l.FS, InitialPath: pkg}
+		pkgNode, err := loader.LoadManifestDataFromPath()
 		if err != nil {
 			return nil, err
 		}
@@ -63,17 +86,7 @@ func manifestToManifestData(m *manifest.Manifest) (*ManifestData, error) {
 	return mdata, nil
 }
 
-// loadManifestFileFromPath loads a manifest object from file.
+func (l *Loader) loadManifestDataFromManifestFileAndResources(m *manifest.Manifest) (*ManifestData, error) {
-func loadManifestFileFromPath(filename string) (*manifest.Manifest, error) {
-	loader := kutil.ManifestLoader{fs.MakeRealFS()}
-	m, err := loader.Read(filename)
-	if err != nil {
-		return nil, err
-	}
-	return m, err
-}
-
-func loadManifestDataFromManifestFileAndResources(m *manifest.Manifest) (*ManifestData, error) {
 	mdata := &ManifestData{}
 	var err error
 	mdata.Name = m.Name
@@ -81,13 +94,15 @@ func loadManifestDataFromManifestFileAndResources(m *manifest.Manifest) (*Manife
 	mdata.ObjectLabels = m.ObjectLabels
 	mdata.ObjectAnnotations = m.ObjectAnnotations
 
-	res, err := loadKObjectFromPaths(m.Resources)
+	res, err := l.loadKObjectFromPaths(m.Resources)
 	if err != nil {
-		return nil, err
+		errorMsg := fmt.Sprintf("Resource from Manifest (%s) couldn't be loaded properly.\n%v\n"+
+			"Please check the Resource subsection in (%s).", l.FullFilePath, err, l.FullFilePath)
+		return nil, fmt.Errorf(errorMsg)
 	}
 	mdata.Resources = ResourcesType(res)
 
-	pat, err := loadKObjectFromPaths(m.Patches)
+	pat, err := l.loadKObjectFromPaths(m.Patches)
 	if err != nil {
 		return nil, err
 	}
@@ -113,10 +128,10 @@ func loadManifestDataFromManifestFileAndResources(m *manifest.Manifest) (*Manife
 	return mdata, nil
 }
 
-func loadKObjectFromPaths(paths []string) (types.KObject, error) {
+func (l *Loader) loadKObjectFromPaths(paths []string) (types.KObject, error) {
 	res := types.KObject{}
 	for _, path := range paths {
-		err := loadKObjectFromPath(path, res)
+		err := l.loadKObjectFromPath(path, res)
 		if err != nil {
 			return nil, err
 		}
@@ -124,8 +139,8 @@ func loadKObjectFromPaths(paths []string) (types.KObject, error) {
 	return res, nil
 }
 
-func loadKObjectFromPath(path string, into types.KObject) error {
+func (l *Loader) loadKObjectFromPath(path string, into types.KObject) error {
-	_, err := os.Stat(path)
+	_, err := l.FS.Stat(path)
 	if err != nil {
 		return err
 	}
@@ -144,21 +159,21 @@ func loadKObjectFromPath(path string, into types.KObject) error {
 			return nil
 		}
 
-		err = loadKObjectFromFile(filepath, into)
+		err = l.loadKObjectFromFile(filepath, into)
 		return nil
 	})
 	return e
 }
 
-func loadKObjectFromFile(filename string, into types.KObject) error {
+func (l *Loader) loadKObjectFromFile(filename string, into types.KObject) error {
-	f, err := os.Stat(filename)
+	f, err := l.FS.Stat(filename)
 	if err != nil {
 		return err
 	}
 	if f.IsDir() {
 		return fmt.Errorf("%q is NOT expected to be an dir", filename)
 	}
-	content, err := ioutil.ReadFile(filename)
+	content, err := l.FS.ReadFile(filename)
 	if err != nil {
 		return err
 	}

pkg/kinflate/tree/builder_test.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	manifest "k8s.io/kubectl/pkg/apis/manifest/v1alpha1"
 	"k8s.io/kubectl/pkg/kinflate/types"
+	"k8s.io/kubectl/pkg/kinflate/util/fs"
 )
 
 func makeMapOfConfigMap() types.KObject {
@@ -129,9 +130,12 @@ func TestFileToMap(t *testing.T) {
 		},
 	}
 
+	// TODO: Convert to a fake filesystem instead of using test files.
+	loader := Loader{FS: fs.MakeRealFS()}
+
 	for _, tc := range testcases {
 		actual := types.KObject{}
-		err := loadKObjectFromFile(tc.filename, actual)
+		err := loader.loadKObjectFromFile(tc.filename, actual)
 		if err == nil {
 			if tc.expectErr {
 				t.Errorf("filename: %q, expect an error containing %q, but didn't get an error", tc.filename, tc.errorStr)
@@ -179,9 +183,12 @@ func TestPathToMap(t *testing.T) {
 		},
 	}
 
+	// TODO: Convert to a fake filesystem instead of using test files.
+	loader := Loader{FS: fs.MakeRealFS()}
+
 	for _, tc := range testcases {
 		actual := types.KObject{}
-		err := loadKObjectFromPath(tc.filename, actual)
+		err := loader.loadKObjectFromPath(tc.filename, actual)
 		if err == nil {
 			if tc.expectErr {
 				t.Errorf("filename: %q, expect an error containing %q, but didn't get an error", tc.filename, tc.errorStr)
@@ -240,8 +247,11 @@ func TestPathsToMap(t *testing.T) {
 		},
 	}
 
+	// TODO: Convert to a fake filesystem instead of using test files.
+	loader := Loader{FS: fs.MakeRealFS()}
+
 	for _, tc := range testcases {
-		actual, err := loadKObjectFromPaths(tc.filenames)
+		actual, err := loader.loadKObjectFromPaths(tc.filenames)
 		if err == nil {
 			if tc.expectErr {
 				t.Errorf("filenames: %q, expect an error containing %q, but didn't get an error", tc.filenames, tc.errorStr)
@@ -301,7 +311,9 @@ func TestManifestToManifestData(t *testing.T) {
 		Secrets:           SecretsType(types.KObject{}),
 	}
 
-	actual, err := loadManifestDataFromManifestFileAndResources(m)
+	// TODO: Convert to a fake filesystem instead of using test files.
+	loader := Loader{FS: fs.MakeRealFS()}
+	actual, err := loader.loadManifestDataFromManifestFileAndResources(m)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -321,8 +333,9 @@ func TestLoadManifestDataFromPath(t *testing.T) {
 	parent1.Packages = []*ManifestData{child1}
 	parent2.Packages = []*ManifestData{child2}
 
+	loader := Loader{FS: fs.MakeRealFS(), InitialPath: "testdata/hierarchy"}
 	expected := grandparent
-	actual, err := loadManifestDataFromPath("testdata/hierarchy")
+	actual, err := loader.LoadManifestDataFromPath()
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

pkg/kinflate/util/manifestloader.go

@@ -33,6 +33,12 @@ type ManifestLoader struct {
 	FS fs.FileSystem
 }
 
+// First pass to encapsulate fields for more informative error messages.
+type ManifestErrors struct {
+	filepath string
+	errorMsg string
+}
+
 func (m *ManifestLoader) fs() fs.FileSystem {
 	if m.FS == nil {
 		m.FS = fs.MakeRealFS()
@@ -43,7 +49,7 @@ func (m *ManifestLoader) fs() fs.FileSystem {
 // makeValidManifestPath returns a path to a KubeManifest file known to exist.
 // The argument is either the full path to the file itself, or a path to a directory
 // that immediately contains the file. Anything else is an error.
-func (m *ManifestLoader) makeValidManifestPath(mPath string) (string, error) {
+func (m *ManifestLoader) MakeValidManifestPath(mPath string) (string, error) {
 	f, err := m.fs().Stat(mPath)
 	if err != nil {
 		errorMsg := fmt.Sprintf("Manifest (%s) missing\nRun `kinflate init` first", mPath)
@@ -65,11 +71,7 @@ func (m *ManifestLoader) makeValidManifestPath(mPath string) (string, error) {
 }
 
 // Read loads a manifest file and parse it in to the Manifest object.
-func (m *ManifestLoader) Read(mPath string) (*manifest.Manifest, error) {
+func (m *ManifestLoader) Read(filename string) (*manifest.Manifest, error) {
-	filename, err := m.makeValidManifestPath(mPath)
-	if err != nil {
-		return nil, err
-	}
 	bytes, err := m.fs().ReadFile(filename)
 	if err != nil {
 		return nil, err
@@ -97,3 +99,25 @@ func (m *ManifestLoader) Write(filename string, manifest *manifest.Manifest) err
 
 	return m.fs().WriteFile(filename, bytes)
 }
+
+// Read must have already been called and we have a loaded manifest
+func (m *ManifestLoader) Validate(manifest *manifest.Manifest) []ManifestErrors {
+	//TODO: implement this function
+	//// validate Packages
+	//merrors := m.validatePackages(manifest.Packages)
+	//// validate Resources
+	//merrors = merrors + m.validateResources(manifest.Resources)
+	//
+	//// validate Patches
+	//merrors = append(merrors, m.validatePatches(manifest.Patches))
+	//
+	//// validate Configmaps
+	//merrors = append(merrors, m.validateConfigmaps(manifest.Configmaps))
+	//
+	//// validate GenericSecrets
+	//merrors = append(merrors, m.validateGenericSecrets(manifest.GenericSecrets))
+	//
+	//// validate TLSSecrets
+	//merrors = append(merrors, m.validateTLSSecrets(manifest.TLSSecrets))
+	return nil
+}

pkg/kinflate/util/manifestloader_test.go

@@ -62,21 +62,21 @@ func TestLoadNotExist(t *testing.T) {
 	fakeFS.Mkdir(".", 0644)
 	fakeFS.Create(badSuffix)
 	loader := kutil.ManifestLoader{FS: fakeFS}
-	_, err := loader.Read("Kube-manifest.yaml")
+	_, err := loader.MakeValidManifestPath("Kube-manifest.yaml")
 	if err == nil {
 		t.Fatalf("expect an error")
 	}
 	if !strings.Contains(err.Error(), "Run `kinflate init` first") {
 		t.Fatalf("expect an error contains %q, but got %v", "does not exist", err)
 	}
-	_, err = loader.Read(".")
+	_, err = loader.MakeValidManifestPath(".")
 	if err == nil {
 		t.Fatalf("expect an error")
 	}
 	if !strings.Contains(err.Error(), "Run `kinflate init` first") {
 		t.Fatalf("expect an error contains %q, but got %v", "does not exist", err)
 	}
-	_, err = loader.Read(badSuffix)
+	_, err = loader.MakeValidManifestPath(badSuffix)
 	if err == nil {
 		t.Fatalf("expect an error")
 	}