Repository: fluxcd/flux
Pull request #2565 (2019-10-30 17:17:06)

Improve experience with .flux.yaml files
 - include more information like filenames in errors
 - correct the implementation of resourceID so it uses the name of the
   resource

These two address a comment https://github.com/fluxcd/flux/issues/2428#issuecomment-540031938 that fluxd should at least provide information about which file/resource is failing, when syncing goes wrong.

 - return an error if asked to update a manifest, but there are no (relevant) update commands in the config file

This addresses #2324 by making it more obvious what has failed (in that issue, the complaint can be interpreted as "nothing happened and I don't know why").

squaremo: @2opremio I've moved this out of draft, and done more work to surface mistakes with config files; PTAnotherL when you have a sec. :apple: 
2opremio: Very minor stuff, otherwise LGTM! 💯 (+ an extra 🥇 for the dispatching refactoring!)

Patch:
diff --git a/go.mod b/go.mod
index 15e27bd7..8b3fdbc1 100644
--- a/go.mod
+++ b/go.mod
@@ -22,6 +22,7 @@ require (
 	github.com/pkg/errors v0.8.1
 	github.com/pkg/term v0.0.0-20190109203006-aa71e9d9e942
 	github.com/prometheus/client_golang v1.1.0
+	github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90
 	github.com/ryanuber/go-glob v1.0.0
 	github.com/shurcooL/httpfs v0.0.0-20190707220628-8d4bc4ba7749
 	github.com/shurcooL/vfsgen v0.0.0-20181202132449-6a9ea43bcacd
diff --git a/pkg/cluster/kubernetes/patch.go b/pkg/cluster/kubernetes/patch.go
index 94f215dc..850efd64 100644
--- a/pkg/cluster/kubernetes/patch.go
+++ b/pkg/cluster/kubernetes/patch.go
@@ -74,7 +74,7 @@ func applyManifestPatch(originalManifests, patchManifests []byte, originalSource
 	// Make sure all patch resources have a matching resource
 	for id, patchResource := range patchResources {
 		if _, ok := originalResources[id]; !ok {
-			return nil, fmt.Errorf("missing resource (%s) for patch", resourceID(patchResource))
+			return nil, fmt.Errorf("patch refers to missing resource (%s)", resourceID(patchResource))
 		}
 	}
 
@@ -94,7 +94,7 @@ func applyManifestPatch(originalManifests, patchManifests []byte, originalSource
 			// There was a patch, apply it
 			patched, err := applyPatch(originalResource, patchedResource, scheme)
 			if err != nil {
-				return nil, fmt.Errorf("cannot obtain patch for resource %s: %s", id, err)
+				return nil, fmt.Errorf("cannot apply patch for resource %s: %s", id, err)
 			}
 			resourceBytes = patched
 		}
@@ -221,8 +221,7 @@ func applyPatch(originalManifest, patchManifest kresource.KubeManifest, scheme *
 	}
 	patched, err := jsonyaml.JSONToYAML(patchedJSON)
 	if err != nil {
-		return nil, fmt.Errorf("cannot transform patched resource (%s) to YAML: %s",
-			resourceID(originalManifest), err)
+		return nil, fmt.Errorf("cannot transform patched resource (%s) to YAML: %s", resourceID(originalManifest), err)
 	}
 	return patched, nil
 }
@@ -230,5 +229,5 @@ func applyPatch(originalManifest, patchManifest kresource.KubeManifest, scheme *
 // resourceID works like Resource.ID() but avoids <cluster> namespaces,
 // since they may be incorrect
 func resourceID(manifest kresource.KubeManifest) resource.ID {
-	return resource.MakeID(manifest.GetNamespace(), manifest.GetKind(), manifest.GetKind())
+	return resource.MakeID(manifest.GetNamespace(), manifest.GetKind(), manifest.GetName())
 }
diff --git a/pkg/manifests/configaware.go b/pkg/manifests/configaware.go
index 814ce7e1..5aa1056f 100644
--- a/pkg/manifests/configaware.go
+++ b/pkg/manifests/configaware.go
@@ -1,11 +1,8 @@
 package manifests
 
 import (
-	"bytes"
 	"context"
-	"errors"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -143,7 +140,7 @@ func (ca *configAware) SetWorkloadContainerImage(ctx context.Context, resourceID
 		if err := ca.rawFiles.setManifestWorkloadContainerImage(resWithOrigin.resource, container, newImageID); err != nil {
 			return err
 		}
-	} else if err := ca.setConfigFileWorkloadContainerImage(ctx, resWithOrigin.configFile, resWithOrigin.resource, container, newImageID); err != nil {
+	} else if err := resWithOrigin.configFile.SetWorkloadContainerImage(ctx, ca.manifests, resWithOrigin.resource, container, newImageID); err != nil {
 		return err
 	}
 	// Reset resources, since we have modified one
@@ -151,123 +148,7 @@ func (ca *configAware) SetWorkloadContainerImage(ctx context.Context, resourceID
 	return nil
 }
 
-func (ca *configAware) setConfigFileWorkloadContainerImage(ctx context.Context, cf *ConfigFile, r resource.Resource,
-	container string, newImageID image.Ref) error {
-	if cf.PatchUpdated != nil {
-		return ca.updatePatchFile(ctx, cf, func(previousManifests []byte) ([]byte, error) {
-			return ca.manifests.SetWorkloadContainerImage(previousManifests, r.ResourceID(), container, newImageID)
-		})
-	}
-
-	// Command-updated
-	result := cf.ExecContainerImageUpdaters(ctx,
-		r.ResourceID(),
-		container,
-		newImageID.Name.String(), newImageID.Tag,
-	)
-	if len(result) > 0 && result[len(result)-1].Error != nil {
-		updaters := cf.CommandUpdated.Updaters
-		return fmt.Errorf("error executing image updater command %q from file %q: %s\noutput:\n%s",
-			updaters[len(result)-1].ContainerImage.Command,
-			result[len(result)-1].Error,
-			r.Source(),
-			result[len(result)-1].Output,
-		)
-	}
-	return nil
-}
-
-func (ca *configAware) updatePatchFile(ctx context.Context, cf *ConfigFile,
-	updateF func(previousManifests []byte) ([]byte, error)) error {
-
-	patchUpdated := *cf.PatchUpdated
-	generatedManifests, patchedManifests, patchFilePath, err := ca.getGeneratedAndPatchedManifests(ctx, cf, patchUpdated)
-	if err != nil {
-		relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
-		if err != nil {
-			return err
-		}
-		return fmt.Errorf("error parsing generated, patched output from file %s: %s", relConfigFilePath, err)
-	}
-	finalManifests, err := updateF(patchedManifests)
-	if err != nil {
-		return err
-	}
-	newPatch, err := ca.manifests.CreateManifestPatch(generatedManifests, finalManifests,
-		"generated manifests", "patched and updated manifests")
-	if err != nil {
-		return err
-	}
-	return ioutil.WriteFile(patchFilePath, newPatch, 0600)
-}
-
-func (ca *configAware) getGeneratedAndPatchedManifests(ctx context.Context, cf *ConfigFile, patchUpdated PatchUpdated) ([]byte, []byte, string, error) {
-	generatedManifests, err := ca.getGeneratedManifests(ctx, cf, patchUpdated.Generators)
-	if err != nil {
-		return nil, nil, "", err
-	}
-
-	// The patch file is expressed relatively to the configuration file's working directory
-	explicitPatchFilePath := patchUpdated.PatchFile
-	patchFilePath := filepath.Join(cf.WorkingDir, explicitPatchFilePath)
-
-	// Make sure that the patch file doesn't fall out of the Git repository checkout
-	_, _, err = cleanAndEnsureParentPath(ca.baseDir, patchFilePath)
-	if err != nil {
-		return nil, nil, "", err
-	}
-	patch, err := ioutil.ReadFile(patchFilePath)
-	if err != nil {
-		if !os.IsNotExist(err) {
-			return nil, nil, "", err
-		}
-		// Tolerate a missing patch file, since it may not have been created yet.
-		// However, its base path must exist.
-		patchBaseDir := filepath.Dir(patchFilePath)
-		if stat, err := os.Stat(patchBaseDir); err != nil || !stat.IsDir() {
-			err := fmt.Errorf("base directory (%q) of patchFile (%q) does not exist",
-				filepath.Dir(explicitPatchFilePath), explicitPatchFilePath)
-			return nil, nil, "", err
-		}
-		patch = nil
-	}
-	relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
-	if err != nil {
-		return nil, nil, "", err
-	}
-	patchedManifests, err := ca.manifests.ApplyManifestPatch(generatedManifests, patch, relConfigFilePath, explicitPatchFilePath)
-	if err != nil {
-		return nil, nil, "", fmt.Errorf("cannot patch generated resources: %s", err)
-	}
-	return generatedManifests, patchedManifests, patchFilePath, nil
-}
-
-func (ca *configAware) getGeneratedManifests(ctx context.Context, cf *ConfigFile, generators []Generator) ([]byte, error) {
-	buf := bytes.NewBuffer(nil)
-	for i, cmdResult := range cf.ExecGenerators(ctx, generators) {
-		relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
-		if err != nil {
-			return nil, err
-		}
-		if cmdResult.Error != nil {
-			err := fmt.Errorf("error executing generator command %q from file %q: %s\nerror output:\n%s\ngenerated output:\n%s",
-				generators[i].Command,
-				relConfigFilePath,
-				cmdResult.Error,
-				string(cmdResult.Stderr),
-				string(cmdResult.Stderr),
-			)
-			return nil, err
-		}
-		if err := ca.manifests.AppendManifestToBuffer(cmdResult.Stdout, buf); err != nil {
-			return nil, err
-		}
-	}
-	return buf.Bytes(), nil
-}
-
-func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID resource.ID,
-	update resource.PolicyUpdate) (bool, error) {
+func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID resource.ID, update resource.PolicyUpdate) (bool, error) {
 	resourcesByID, err := ca.getResourcesByID(ctx)
 	if err != nil {
 		return false, err
@@ -280,7 +161,8 @@ func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID re
 	if resWithOrigin.configFile == nil {
 		changed, err = ca.rawFiles.updateManifestWorkloadPolicies(resWithOrigin.resource, update)
 	} else {
-		changed, err = ca.updateConfigFileWorkloadPolicies(ctx, resWithOrigin.configFile, resWithOrigin.resource, update)
+		cf := resWithOrigin.configFile
+		changed, err = cf.UpdateWorkloadPolicies(ctx, ca.manifests, resWithOrigin.resource, update)
 	}
 	if err != nil {
 		return false, err
@@ -290,48 +172,6 @@ func (ca *configAware) UpdateWorkloadPolicies(ctx context.Context, resourceID re
 	return changed, nil
 }
 
-func (ca *configAware) updateConfigFileWorkloadPolicies(ctx context.Context, cf *ConfigFile, r resource.Resource,
-	update resource.PolicyUpdate) (bool, error) {
-	if cf.PatchUpdated != nil {
-		var changed bool
-		err := ca.updatePatchFile(ctx, cf, func(previousManifests []byte) ([]byte, error) {
-			updatedManifests, err := ca.manifests.UpdateWorkloadPolicies(previousManifests, r.ResourceID(), update)
-			if err == nil {
-				changed = bytes.Compare(previousManifests, updatedManifests) != 0
-			}
-			return updatedManifests, err
-		})
-		return changed, err
-	}
-
-	// Command-updated
-	workload, ok := r.(resource.Workload)
-	if !ok {
-		return false, errors.New("resource " + r.ResourceID().String() + " does not have containers")
-	}
-	changes, err := resource.ChangesForPolicyUpdate(workload, update)
-	if err != nil {
-		return false, err
-	}
-
-	for key, value := range changes {
-		result := cf.ExecPolicyUpdaters(ctx, r.ResourceID(), key, value)
-		if len(result) > 0 && result[len(result)-1].Error != nil {
-			updaters := cf.CommandUpdated.Updaters
-			err := fmt.Errorf("error executing annotation updater command %q from file %q: %s\noutput:\n%s",
-				updaters[len(result)-1].Policy.Command,
-				result[len(result)-1].Error,
-				r.Source(),
-				result[len(result)-1].Output,
-			)
-			return false, err
-		}
-	}
-	// We assume that the update changed the resource. Alternatively, we could generate the resources
-	// again and compare the output, but that's expensive.
-	return true, nil
-}
-
 func (ca *configAware) GetAllResourcesByID(ctx context.Context) (map[string]resource.Resource, error) {
 	resourcesByID, err := ca.getResourcesByID(ctx)
 	if err != nil {
@@ -364,23 +204,11 @@ func (ca *configAware) getResourcesByID(ctx context.Context) (map[string]resourc
 	}
 
 	for _, cf := range ca.configFiles {
-		var (
-			resourceManifests []byte
-			err               error
-		)
-		if cf.CommandUpdated != nil {
-			var err error
-			resourceManifests, err = ca.getGeneratedManifests(ctx, cf, cf.CommandUpdated.Generators)
-			if err != nil {
-				return nil, err
-			}
-		} else {
-			_, resourceManifests, _, err = ca.getGeneratedAndPatchedManifests(ctx, cf, *cf.PatchUpdated)
-		}
+		resourceManifests, err := cf.GenerateManifests(ctx, ca.manifests)
 		if err != nil {
 			return nil, err
 		}
-		relConfigFilePath, err := filepath.Rel(ca.baseDir, cf.Path)
+		relConfigFilePath, err := cf.RelativeConfigPath()
 		if err != nil {
 			return nil, err
 		}
diff --git a/pkg/manifests/configfile.go b/pkg/manifests/configfile.go
index a9ff84f2..4427bca3 100644
--- a/pkg/manifests/configfile.go
+++ b/pkg/manifests/configfile.go
@@ -8,11 +8,13 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"time"
 
 	"github.com/pkg/errors"
 	"gopkg.in/yaml.v2"
 
+	"github.com/fluxcd/flux/pkg/image"
 	"github.com/fluxcd/flux/pkg/resource"
 )
 
@@ -21,42 +23,60 @@ const (
 	CommandTimeout = time.Minute
 )
 
+// ConfigFile holds the values necessary for generating and updating
+// manifests according to a `.flux.yaml` file. It does double duty as
+// the format for the file (to deserialise into), and the state
+// necessary for running commands.
 type ConfigFile struct {
-	Path       string
-	WorkingDir string
-	Version    int
+	Version int
 	// Only one of the following should be set simultaneously
 	CommandUpdated *CommandUpdated `yaml:"commandUpdated"`
 	PatchUpdated   *PatchUpdated   `yaml:"patchUpdated"`
+
+	path       string // the absolute path to the .flux.yaml
+	workingDir string // the absolute path to the dir in which to run commands or find a patch file
 }
 
+// CommandUpdated represents a config in which updates are done by
+// execing commands as given.
 type CommandUpdated struct {
 	Generators []Generator
 	Updaters   []Updater
 }
 
+// Generator is an individual command for generating manifests.
 type Generator struct {
 	Command string
 }
 
+// Updater gives a means for updating image refs and a means for
+// updating policy in a manifest.
 type Updater struct {
 	ContainerImage ContainerImageUpdater `yaml:"containerImage"`
 	Policy         PolicyUpdater
 }
 
+// ContainerImageUpdater is a command for updating the image used by a
+// container, in a manifest.
 type ContainerImageUpdater struct {
 	Command string
 }
 
+// PolicyUpdater is a command for updating a policy for a manifest.
 type PolicyUpdater struct {
 	Command string
 }
 
+// PatchUpdated represents a config in which updates are done by
+// maintaining a patch, which is calculating from, and applied to, the
+// generated manifests.
 type PatchUpdated struct {
 	Generators []Generator
 	PatchFile  string `yaml:"patchFile"`
 }
 
+// NewConfigFile constructs a ConfigFile from the file at the absolute
+// path given, with the absolute working dir given.
 func NewConfigFile(path, workingDir string) (*ConfigFile, error) {
 	var result ConfigFile
 	fileBytes, err := ioutil.ReadFile(path)
@@ -66,20 +86,119 @@ func NewConfigFile(path, workingDir string) (*ConfigFile, error) {
 	if err := yaml.Unmarshal(fileBytes, &result); err != nil {
 		return nil, fmt.Errorf("cannot parse: %s", err)
 	}
-	result.Path = path
-	result.WorkingDir = workingDir
+	result.path = path
+	result.workingDir = workingDir
 	switch {
+	case result.Version != 1:
+		return nil, errors.New("incorrect version, only version 1 is supported for now")
 	case (result.CommandUpdated != nil && result.PatchUpdated != nil) ||
 		(result.CommandUpdated == nil && result.PatchUpdated == nil):
 		return nil, errors.New("a single commandUpdated or patchUpdated entry must be defined")
 	case result.PatchUpdated != nil && result.PatchUpdated.PatchFile == "":
 		return nil, errors.New("patchUpdated's patchFile cannot be empty")
-	case result.Version != 1:
-		return nil, errors.New("incorrect version, only version 1 is supported for now")
 	}
 	return &result, nil
 }
 
+// -- entry points for using a config file to generate or update manifests
+
+func makeNoCommandsRunErr(field string, cf *ConfigFile) error {
+	relConfigPath, err := cf.RelativeConfigPath()
+	if err != nil {
+		return fmt.Errorf("config file not relative to working dir: %s", err)
+	}
+	return fmt.Errorf("no %s commands to run in %s", field, relConfigPath)
+}
+
+// RelativeConfigPath returns the path to the config file, relative to
+// the working directory. This is used in error messages and to
+// identify resources generated from the config file.
+func (cf *ConfigFile) RelativeConfigPath() (string, error) {
+	return filepath.Rel(cf.workingDir, cf.path)
+}
+
+// GenerateManifests returns the manifests generated (and patched, if
+// necessary) according to the config file.
+func (cf *ConfigFile) GenerateManifests(ctx context.Context, manifests Manifests) ([]byte, error) {
+	if cf.PatchUpdated != nil {
+		finalBytes, _, _, err := cf.getGeneratedAndPatchedManifests(ctx, manifests)
+		return finalBytes, err
+	}
+	return cf.getGeneratedManifests(ctx, manifests, cf.CommandUpdated.Generators)
+}
+
+func (cf *ConfigFile) SetWorkloadContainerImage(ctx context.Context, manifests Manifests, r resource.Resource, container string, newImageID image.Ref) error {
+	if cf.PatchUpdated != nil {
+		return cf.updatePatchFile(ctx, manifests, func(previousManifests []byte) ([]byte, error) {
+			return manifests.SetWorkloadContainerImage(previousManifests, r.ResourceID(), container, newImageID)
+		})
+	}
+
+	// Command-updated
+	result := cf.execContainerImageUpdaters(ctx, r.ResourceID(), container, newImageID.Name.String(), newImageID.Tag)
+	if len(result) == 0 {
+		return makeNoCommandsRunErr("update.containerImage", cf)
+	}
+
+	if len(result) > 0 && result[len(result)-1].Error != nil {
+		updaters := cf.CommandUpdated.Updaters
+		return fmt.Errorf("error executing image updater command %q from file %q: %s\noutput:\n%s",
+			updaters[len(result)-1].ContainerImage.Command,
+			result[len(result)-1].Error,
+			r.Source(),
+			result[len(result)-1].Output,
+		)
+	}
+	return nil
+}
+
+// UpdateWorkloadPolicies updates policies for a workload, using
+// commands or patching according to the config file.
+func (cf *ConfigFile) UpdateWorkloadPolicies(ctx context.Context, manifests Manifests, r resource.Resource, update resource.PolicyUpdate) (bool, error) {
+	if cf.PatchUpdated != nil {
+		var changed bool
+		err := cf.updatePatchFile(ctx, manifests, func(previousManifests []byte) ([]byte, error) {
+			updatedManifests, err := manifests.UpdateWorkloadPolicies(previousManifests, r.ResourceID(), update)
+			if err == nil {
+				changed = bytes.Compare(previousManifests, updatedManifests) != 0
+			}
+			return updatedManifests, err
+		})
+		return changed, err
+	}
+
+	// Command-updated
+	workload, ok := r.(resource.Workload)
+	if !ok {
+		return false, errors.New("resource " + r.ResourceID().String() + " does not have containers")
+	}
+	changes, err := resource.ChangesForPolicyUpdate(workload, update)
+	if err != nil {
+		return false, err
+	}
+
+	for key, value := range changes {
+		result := cf.execPolicyUpdaters(ctx, r.ResourceID(), key, value)
+		if len(result) == 0 {
+			return false, makeNoCommandsRunErr("updaters.policy", cf)
+		}
+
+		if len(result) > 0 && result[len(result)-1].Error != nil {
+			updaters := cf.CommandUpdated.Updaters
+			err := fmt.Errorf("error executing annotation updater command %q from file %q: %s\noutput:\n%s",
+				updaters[len(result)-1].Policy.Command,
+				result[len(result)-1].Error,
+				r.Source(),
+				result[len(result)-1].Output,
+			)
+			return false, err
+		}
+	}
+	// We assume that the update changed the resource. Alternatively, we could generate the resources
+	// again and compare the output, but that's expensive.
+	return true, nil
+}
+
 type ConfigFileExecResult struct {
 	Error  error
 	Stderr []byte
@@ -91,7 +210,99 @@ type ConfigFileCombinedExecResult struct {
 	Output []byte
 }
 
-func (cf *ConfigFile) ExecGenerators(ctx context.Context, generators []Generator) []ConfigFileExecResult {
+// -- these are helpers to support the entry points above
+
+// getGeneratedAndPatchedManifests is used to generated manifests when
+// the config is patchUpdated.
+func (cf *ConfigFile) getGeneratedAndPatchedManifests(ctx context.Context, manifests Manifests) ([]byte, []byte, string, error) {
+	generatedManifests, err := cf.getGeneratedManifests(ctx, manifests, cf.PatchUpdated.Generators)
+	if err != nil {
+		return nil, nil, "", err
+	}
+
+	// The patch file is given in the config file as a path relative
+	// to the working directory
+	relPatchFilePath := cf.PatchUpdated.PatchFile
+	patchFilePath := filepath.Join(cf.workingDir, relPatchFilePath)
+
+	patch, err := ioutil.ReadFile(patchFilePath)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			return nil, nil, "", err
+		}
+		// Tolerate a missing patch file, since it may not have been created yet.
+		// However, its base path must exist.
+		patchBaseDir := filepath.Dir(patchFilePath)
+		if stat, err := os.Stat(patchBaseDir); err != nil || !stat.IsDir() {
+			err := fmt.Errorf("base directory (%q) of patchFile (%q) does not exist",
+				filepath.Dir(relPatchFilePath), relPatchFilePath)
+			return nil, nil, "", err
+		}
+		patch = nil
+	}
+	relConfigFilePath, err := cf.RelativeConfigPath()
+	if err != nil {
+		return nil, nil, "", err
+	}
+	patchedManifests, err := manifests.ApplyManifestPatch(generatedManifests, patch, relConfigFilePath, relPatchFilePath)
+	if err != nil {
+		return nil, nil, "", fmt.Errorf("processing %q, cannot apply patchFile %q to generated resources: %s", relConfigFilePath, relPatchFilePath, err)
+	}
+	return generatedManifests, patchedManifests, patchFilePath, nil
+}
+
+// getGeneratedManifests is used to produce the manifests based _only_
+// on the generators in the config. This is sufficient for
+// commandUpdated config, and the first step for patchUpdated config.
+func (cf *ConfigFile) getGeneratedManifests(ctx context.Context, manifests Manifests, generators []Generator) ([]byte, error) {
+	buf := bytes.NewBuffer(nil)
+	for i, cmdResult := range cf.execGenerators(ctx, generators) {
+		relConfigFilePath, err := cf.RelativeConfigPath()
+		if err != nil {
+			return nil, err
+		}
+		if cmdResult.Error != nil {
+			err := fmt.Errorf("error executing generator command %q from file %q: %s\nerror output:\n%s\ngenerated output:\n%s",
+				generators[i].Command,
+				relConfigFilePath,
+				cmdResult.Error,
+				string(cmdResult.Stderr),
+				string(cmdResult.Stderr),
+			)
+			return nil, err
+		}
+		if err := manifests.AppendManifestToBuffer(cmdResult.Stdout, buf); err != nil {
+			return nil, err
+		}
+	}
+	return buf.Bytes(), nil
+}
+
+// updatePatchFile calculates the patch given a transformation, and
+// updates the patch file given in the config.
+func (cf *ConfigFile) updatePatchFile(ctx context.Context, manifests Manifests, updateFn func(previousManifests []byte) ([]byte, error)) error {
+	generatedManifests, patchedManifests, patchFilePath, err := cf.getGeneratedAndPatchedManifests(ctx, manifests)
+	if err != nil {
+		relConfigFilePath, err := cf.RelativeConfigPath()
+		if err != nil {
+			return err
+		}
+		return fmt.Errorf("error parsing generated, patched output from file %s: %s", relConfigFilePath, err)
+	}
+	finalManifests, err := updateFn(patchedManifests)
+	if err != nil {
+		return err
+	}
+	newPatch, err := manifests.CreateManifestPatch(generatedManifests, finalManifests, "generated manifests", "patched and updated manifests")
+	if err != nil {
+		return err
+	}
+	return ioutil.WriteFile(patchFilePath, newPatch, 0600)
+}
+
+// execGenerators executes all the generators given and returns the
+// results; it will stop at the first failing command.
+func (cf *ConfigFile) execGenerators(ctx context.Context, generators []Generator) []ConfigFileExecResult {
 	result := []ConfigFileExecResult{}
 	for _, g := range generators {
 		stdErr := bytes.NewBuffer(nil)
@@ -111,9 +322,9 @@ func (cf *ConfigFile) ExecGenerators(ctx context.Context, generators []Generator
 	return result
 }
 
-// ExecContainerImageUpdaters executes all the image updates in the configuration file.
+// execContainerImageUpdaters executes all the image updates in the configuration file.
 // It will stop at the first error, in which case the returned error will be non-nil
-func (cf *ConfigFile) ExecContainerImageUpdaters(ctx context.Context,
+func (cf *ConfigFile) execContainerImageUpdaters(ctx context.Context,
 	workload resource.ID, container string, image, imageTag string) []ConfigFileCombinedExecResult {
 	env := makeEnvFromResourceID(workload)
 	env = append(env,
@@ -132,11 +343,11 @@ func (cf *ConfigFile) ExecContainerImageUpdaters(ctx context.Context,
 	return cf.execCommandsWithCombinedOutput(ctx, env, commands)
 }
 
-// ExecPolicyUpdaters executes all the policy update commands given in
+// execPolicyUpdaters executes all the policy update commands given in
 // the configuration file. An empty policyValue means remove the
 // policy. It will stop at the first error, in which case the returned
 // error will be non-nil
-func (cf *ConfigFile) ExecPolicyUpdaters(ctx context.Context,
+func (cf *ConfigFile) execPolicyUpdaters(ctx context.Context,
 	workload resource.ID, policyName, policyValue string) []ConfigFileCombinedExecResult {
 	env := makeEnvFromResourceID(workload)
 	env = append(env, "FLUX_POLICY="+policyName)
@@ -178,7 +389,7 @@ func (cf *ConfigFile) execCommand(ctx context.Context, env []string, stdOut, std
 	defer cancel()
 	cmd := exec.CommandContext(ctx, "/bin/sh", "-c", command)
 	cmd.Env = env
-	cmd.Dir = cf.WorkingDir
+	cmd.Dir = cf.workingDir
 	cmd.Stdout = stdOut
 	cmd.Stderr = stdErr
 	err := cmd.Run()


Test patch:
diff --git a/pkg/manifests/configaware_test.go b/pkg/manifests/configaware_test.go
index 499b6a8b..b2b947ec 100644
--- a/pkg/manifests/configaware_test.go
+++ b/pkg/manifests/configaware_test.go
@@ -90,8 +90,20 @@ commandUpdated:
 
 	assert.Len(t, configFiles, 2)
 	// We assume config files are processed in order to simplify the checks
-	assert.Equal(t, filepath.Join(baseDir, "envs/staging"), configFiles[0].WorkingDir)
-	assert.Equal(t, filepath.Join(baseDir, "envs/production"), configFiles[1].WorkingDir)
+	assert.Equal(t, filepath.Join(baseDir, "envs/staging"), configFiles[0].workingDir)
+	assert.Equal(t, filepath.Join(baseDir, "envs/production"), configFiles[1].workingDir)
+
+	mustRelativeConfigPath := func(cf *ConfigFile) string {
+		p, err := cf.RelativeConfigPath()
+		if err != nil {
+			t.Error(err)
+		}
+		return p
+	}
+
+	assert.Equal(t, "../.flux.yaml", mustRelativeConfigPath(configFiles[0]))
+	assert.Equal(t, "../.flux.yaml", mustRelativeConfigPath(configFiles[1]))
+
 	assert.NotNil(t, configFiles[0].CommandUpdated)
 	assert.Len(t, configFiles[0].CommandUpdated.Generators, 1)
 	assert.Equal(t, "echo g1", configFiles[0].CommandUpdated.Generators[0].Command)
@@ -223,3 +235,44 @@ spec:
 	assert.NoError(t, err)
 	assert.Equal(t, expectedPatch, string(patch))
 }
+
+const mistakenConf = `
+version: 1
+commandUpdated: # <-- because this is commandUpdated, patchFile is ignored
+  generators:
+    - command: |
+       echo "apiVersion: extensions/v1beta1
+       kind: Deployment
+       metadata:
+         name: helloworld
+       spec:
+         template:
+           metadata:
+             labels:
+              name: helloworld
+           spec:
+             containers:
+             - name: greeter
+               image: quay.io/weaveworks/helloworld:master-a000001
+       ---
+       apiVersion: v1
+       kind: Namespace
+       metadata:
+         name: demo"
+  patchFile: patchfile.yaml
+`
+
+// This tests that when using a config with no update commands, and
+// update operation results in an error, rather than a silent failure
+// to make any changes.
+func TestMistakenConfigFile(t *testing.T) {
+	frs, cleanup := setup(t, mistakenConf)
+	defer cleanup()
+
+	deploymentID := resource.MustParseID("default:deployment/helloworld")
+	ref, _ := image.ParseRef("repo/image:tag")
+
+	ctx := context.Background()
+	err := frs.SetWorkloadContainerImage(ctx, deploymentID, "greeter", ref)
+	assert.Error(t, err)
+}
diff --git a/pkg/manifests/configfile_test.go b/pkg/manifests/configfile_test.go
index 736a13e1..eb992947 100644
--- a/pkg/manifests/configfile_test.go
+++ b/pkg/manifests/configfile_test.go
@@ -73,7 +73,7 @@ func TestExecGenerators(t *testing.T) {
 	var cf ConfigFile
 	err := yaml.Unmarshal([]byte(echoCmdUpdatedConfigFile), &cf)
 	assert.NoError(t, err)
-	result := cf.ExecGenerators(context.Background(), cf.CommandUpdated.Generators)
+	result := cf.execGenerators(context.Background(), cf.CommandUpdated.Generators)
 	assert.Equal(t, 2, len(result), "result: %s", result)
 	assert.Equal(t, "g1\n", string(result[0].Stdout))
 	assert.Equal(t, "g2\n", string(result[1].Stdout))
@@ -84,7 +84,7 @@ func TestExecContainerImageUpdaters(t *testing.T) {
 	err := yaml.Unmarshal([]byte(echoCmdUpdatedConfigFile), &cf)
 	assert.NoError(t, err)
 	resourceID := resource.MustParseID("default:deployment/foo")
-	result := cf.ExecContainerImageUpdaters(context.Background(), resourceID, "bar", "repo/image", "latest")
+	result := cf.execContainerImageUpdaters(context.Background(), resourceID, "bar", "repo/image", "latest")
 	assert.Equal(t, 2, len(result), "result: %s", result)
 	assert.Equal(t,
 		"uci1 default:deployment/foo default deployment foo bar repo/image latest\n",
@@ -102,7 +102,7 @@ func TestExecAnnotationUpdaters(t *testing.T) {
 
 	// Test the update/addition of annotations
 	annotationValue := "value"
-	result := cf.ExecPolicyUpdaters(context.Background(), resourceID, "key", annotationValue)
+	result := cf.execPolicyUpdaters(context.Background(), resourceID, "key", annotationValue)
 	assert.Equal(t, 2, len(result), "result: %s", result)
 	assert.Equal(t,
 		"ua1 default:deployment/foo default deployment foo key value\n",
@@ -112,7 +112,7 @@ func TestExecAnnotationUpdaters(t *testing.T) {
 		string(result[1].Output))
 
 	// Test the deletion of annotations "
-	result = cf.ExecPolicyUpdaters(context.Background(), resourceID, "key", "")
+	result = cf.execPolicyUpdaters(context.Background(), resourceID, "key", "")
 	assert.Equal(t, 2, len(result), "result: %s", result)
 	assert.Equal(t,
 		"ua1 default:deployment/foo default deployment foo key delete\n",
@@ -120,5 +120,4 @@ func TestExecAnnotationUpdaters(t *testing.T) {
 	assert.Equal(t,
 		"ua2 default:deployment/foo default deployment foo key delete\n",
 		string(result[1].Output))
-
 }