Merge pull request #1141 from weaveworks/issue/1123-better-badyaml

Return a friendly message when flux can't parse YAMLs

cluster/cluster.go

@@ -8,11 +8,6 @@ import (
 	"github.com/weaveworks/flux/ssh"
 )
 
-var (
-	ErrNoResourceFilesFoundForService       = errors.New("no resource file found for service")
-	ErrMultipleResourceFilesFoundForService = errors.New("multiple resource files found for service")
-)
-
 // The things we can get from the running cluster. These used to form
 // the remote.Platform interface; but now we do more in the daemon so they
 // are distinct interfaces.

cluster/kubernetes/manifests.go

@@ -10,8 +10,6 @@ import (
 type Manifests struct {
 }
 
-// FindDefinedServices implementation in files.go
-
 func (c *Manifests) LoadManifests(base, first string, rest ...string) (map[string]resource.Resource, error) {
 	return kresource.Load(base, first, rest...)
 }

cluster/kubernetes/resource/load.go

@@ -39,19 +39,19 @@ func Load(base, atLeastOne string, more ...string) (map[string]resource.Resource
 			if !info.IsDir() && filepath.Ext(path) == ".yaml" || filepath.Ext(path) == ".yml" {
 				bytes, err := ioutil.ReadFile(path)
 				if err != nil {
-					return errors.Wrapf(err, "reading file at %q", path)
+					return errors.Wrapf(err, "unable to read file at %q", path)
 				}
 				source, err := filepath.Rel(base, path)
 				if err != nil {
-					return errors.Wrapf(err, "finding relative path for %q", path)
+					return errors.Wrapf(err, "path to scan %q is not under base %q", path, base)
 				}
 				docsInFile, err := ParseMultidoc(bytes, source)
 				if err != nil {
-					return errors.Wrapf(err, "parsing file at %q", path)
+					return err
 				}
 				for id, obj := range docsInFile {
 					if alreadyDefined, ok := objs[id]; ok {
-						return fmt.Errorf(`resource '%s' defined more than once (in %s and %s)`, id, alreadyDefined.Source(), source)
+						return fmt.Errorf(`duplicate definition of '%s' (in %s and %s)`, id, alreadyDefined.Source(), source)
 					}
 					objs[id] = obj
 				}

cluster/manifests.go

@@ -1,23 +1,29 @@
 package cluster
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 
 	"github.com/weaveworks/flux"
 	"github.com/weaveworks/flux/image"
 	"github.com/weaveworks/flux/policy"
 	"github.com/weaveworks/flux/resource"
 )
 
+type ManifestError struct {
+	error
+}
+
+func ErrResourceNotFound(name string) error {
+	return ManifestError{fmt.Errorf("manifest for resource %s not found under manifests path", name)}
+}
+
 // Manifests represents how a set of files are used as definitions of
 // resources, e.g., in Kubernetes, YAML files describing Kubernetes
 // resources.
 type Manifests interface {
-	// Given a directory with manifest files, find which files define
-	// which services.
-	// FIXME(michael): remove when redundant
-	FindDefinedServices(path string) (map[flux.ResourceID][]string, error)
 	// Update the image in a manifest's bytes to that given
 	UpdateImage(def []byte, resourceID flux.ResourceID, container string, newImageID image.Ref) ([]byte, error)
 	// Load all the resource manifests under the path given. `baseDir`
@@ -33,23 +39,22 @@ type Manifests interface {
 	ServicesWithPolicies(path string) (policy.ResourceMap, error)
 }
 
-// UpdateManifest looks for the manifest for a given service, reads
-// its contents, applies f(contents), and writes the results back to
-// the file.
-func UpdateManifest(m Manifests, root string, serviceID flux.ResourceID, f func(manifest []byte) ([]byte, error)) error {
-	services, err := m.FindDefinedServices(root)
+// UpdateManifest looks for the manifest for the identified resource,
+// reads its contents, applies f(contents), and writes the results
+// back to the file.
+func UpdateManifest(m Manifests, root string, id flux.ResourceID, f func(manifest []byte) ([]byte, error)) error {
+	resources, err := m.LoadManifests(root, root)
 	if err != nil {
 		return err
 	}
-	paths := services[serviceID]
-	if len(paths) == 0 {
-		return ErrNoResourceFilesFoundForService
-	}
-	if len(paths) > 1 {
-		return ErrMultipleResourceFilesFoundForService
+
+	resource, ok := resources[id.String()]
+	if !ok {
+		return ErrResourceNotFound(id.String())
 	}
 
-	def, err := ioutil.ReadFile(paths[0])
+	path := filepath.Join(root, resource.Source())
+	def, err := ioutil.ReadFile(path)
 	if err != nil {
 		return err
 	}
@@ -59,9 +64,9 @@ func UpdateManifest(m Manifests, root string, serviceID flux.ResourceID, f func(
 		return err
 	}
 
-	fi, err := os.Stat(paths[0])
+	fi, err := os.Stat(path)
 	if err != nil {
 		return err
 	}
-	return ioutil.WriteFile(paths[0], newDef, fi.Mode())
+	return ioutil.WriteFile(path, newDef, fi.Mode())
 }

cluster/mock.go

@@ -16,7 +16,6 @@ type Mock struct {
 	ExportFunc               func() ([]byte, error)
 	SyncFunc                 func(SyncDef) error
 	PublicSSHKeyFunc         func(regenerate bool) (ssh.PublicKey, error)
-	FindDefinedServicesFunc  func(path string) (map[flux.ResourceID][]string, error)
 	UpdateImageFunc          func(def []byte, id flux.ResourceID, container string, newImageID image.Ref) ([]byte, error)
 	LoadManifestsFunc        func(base, first string, rest ...string) (map[string]resource.Resource, error)
 	ParseManifestsFunc       func([]byte) (map[string]resource.Resource, error)
@@ -49,10 +48,6 @@ func (m *Mock) PublicSSHKey(regenerate bool) (ssh.PublicKey, error) {
 	return m.PublicSSHKeyFunc(regenerate)
 }
 
-func (m *Mock) FindDefinedServices(path string) (map[flux.ResourceID][]string, error) {
-	return m.FindDefinedServicesFunc(path)
-}
-
 func (m *Mock) UpdateImage(def []byte, id flux.ResourceID, container string, newImageID image.Ref) ([]byte, error) {
 	return m.UpdateImageFunc(def, id, container, newImageID)
 }

daemon/daemon.go

@@ -16,7 +16,6 @@ import (
 	"github.com/weaveworks/flux/api/v6"
 	"github.com/weaveworks/flux/api/v9"
 	"github.com/weaveworks/flux/cluster"
-	fluxerr "github.com/weaveworks/flux/errors"
 	"github.com/weaveworks/flux/event"
 	"github.com/weaveworks/flux/git"
 	"github.com/weaveworks/flux/guid"
@@ -88,7 +87,7 @@ func (d *Daemon) getPolicyResourceMap(ctx context.Context) (policy.ResourceMap,
 	case err == git.ErrNoConfig:
 		globalReadOnly = v6.ReadOnlyNoRepo
 	case err != nil:
-		return nil, globalReadOnly, errors.Wrap(err, "getting service policies")
+		return nil, globalReadOnly, manifestLoadError(err)
 	default:
 		globalReadOnly = v6.ReadOnlyMissing
 	}
@@ -366,16 +365,16 @@ func (d *Daemon) updatePolicy(spec update.Spec, updates policy.Updates) updateFu
 				}
 				return newDef, nil
 			})
-			switch err {
-			case cluster.ErrNoResourceFilesFoundForService, cluster.ErrMultipleResourceFilesFoundForService:
-				result.Result[serviceID] = update.ControllerResult{
-					Status: update.ReleaseStatusFailed,
-					Error:  err.Error(),
+			if err != nil {
+				switch err := err.(type) {
+				case cluster.ManifestError:
+					result.Result[serviceID] = update.ControllerResult{
+						Status: update.ReleaseStatusFailed,
+						Error:  err.Error(),
+					}
+				default:
+					return result, err
 				}
-			case nil:
-				// continue
-			default:
-				return result, err
 			}
 		}
 		if len(serviceIDs) == 0 {
@@ -568,32 +567,6 @@ func (d *Daemon) WithClone(ctx context.Context, fn func(*git.Checkout) error) er
 	return fn(co)
 }
 
-func unknownJobError(id job.ID) error {
-	return &fluxerr.Error{
-		Type: fluxerr.Missing,
-		Err:  fmt.Errorf("unknown job %q", string(id)),
-		Help: `Job not found
-
-This is often because the job did not result in committing changes,
-and therefore had no lasting effect. A release dry-run is an example
-of a job that does not result in a commit.
-
-If you were expecting changes to be committed, this may mean that the
-job failed, but its status was lost.
-
-In both of the above cases it is OK to retry the operation that
-resulted in this error.
-
-If you get this error repeatedly, it's probably a bug. Please log an
-issue describing what you were attempting, and posting logs from the
-daemon if possible:
-
-    https://github.com/weaveworks/flux/issues
-
-`,
-	}
-}
-
 func (d *Daemon) LogEvent(ev event.Event) error {
 	if d.EventWriter == nil {
 		d.Logger.Log("event", ev, "logupstream", "false")

daemon/daemon_test.go

@@ -542,7 +542,6 @@ func mockDaemon(t *testing.T) (*Daemon, func(), func(), *cluster.Mock, *mockEven
 			return []cluster.Controller{}, nil
 		}
 		k8s.ExportFunc = func() ([]byte, error) { return testBytes, nil }
-		k8s.FindDefinedServicesFunc = (&kubernetes.Manifests{}).FindDefinedServices
 		k8s.LoadManifestsFunc = kresource.Load
 		k8s.ParseManifestsFunc = func(allDefs []byte) (map[string]resource.Resource, error) {
 			return kresource.ParseMultidoc(allDefs, "test")

daemon/errors.go

@@ -0,0 +1,62 @@
+package daemon
+
+import (
+	"fmt"
+
+	fluxerr "github.com/weaveworks/flux/errors"
+	"github.com/weaveworks/flux/job"
+)
+
+func manifestLoadError(reason error) error {
+	return &fluxerr.Error{
+		Type: fluxerr.User,
+		Err:  reason,
+		Help: `Unable to parse files as manifests
+
+Flux was unable to parse the files in the git repo as manifests,
+giving this error:
+
+    ` + reason.Error() + `
+
+Check that any files mentioned are well-formed, and resources are not
+defined more than once. It's also worth reviewing
+
+    https://github.com/weaveworks/flux/blob/master/site/requirements.md
+
+to make sure you're not running into any corner cases.
+
+If you think your files are all OK and you are still getting this
+message, please log an issue at
+
+    https://github.com/weaveworks/flux/issues
+
+and include the problematic file, if possible.
+`,
+	}
+}
+
+func unknownJobError(id job.ID) error {
+	return &fluxerr.Error{
+		Type: fluxerr.Missing,
+		Err:  fmt.Errorf("unknown job %q", string(id)),
+		Help: `Job not found
+
+This is often because the job did not result in committing changes,
+and therefore had no lasting effect. A release dry-run is an example
+of a job that does not result in a commit.
+
+If you were expecting changes to be committed, this may mean that the
+job failed, but its status was lost.
+
+In both of the above cases it is OK to retry the operation that
+resulted in this error.
+
+If you get this error repeatedly, it's probably a bug. Please log an
+issue describing what you were attempting, and posting logs from the
+daemon if possible:
+
+    https://github.com/weaveworks/flux/issues
+
+`,
+	}
+}

daemon/loop_test.go

@@ -48,7 +48,6 @@ func daemon(t *testing.T) (*Daemon, func()) {
 		return kresource.ParseMultidoc(allDefs, "exported")
 	}
 	k8s.ExportFunc = func() ([]byte, error) { return nil, nil }
-	k8s.FindDefinedServicesFunc = (&kubernetes.Manifests{}).FindDefinedServices
 	k8s.ServicesWithPoliciesFunc = (&kubernetes.Manifests{}).ServicesWithPolicies
 
 	events = &mockEventWriter{}