Change API from StateOK/Apply to CheckApply()

This simplifies the API, and reduces code duplication for most
resources. It became obvious when writing the pkg resource, that the two
operations should really be one. Hopefully this will last! Comments
welcome.

exec.go

@@ -20,6 +20,7 @@ package main
 import (
 	"bufio"
 	"bytes"
+	"errors"
 	"log"
 	"os/exec"
 	"strings"
@@ -193,13 +194,14 @@ func (obj *ExecRes) Watch() {
 }
 
 // TODO: expand the IfCmd to be a list of commands
-func (obj *ExecRes) StateOK() bool {
+func (obj *ExecRes) CheckApply(apply bool) (stateok bool, err error) {
+	log.Printf("%v[%v]: CheckApply(%t)", obj.GetRes(), obj.GetName(), apply)
 
 	// if there is a watch command, but no if command, run based on state
-	if b := obj.isStateOK; obj.WatchCmd != "" && obj.IfCmd == "" {
+	if obj.WatchCmd != "" && obj.IfCmd == "" {
-		obj.isStateOK = true // reset
+		if obj.isStateOK {
-		//if !obj.isStateOK { obj.isStateOK = true; return false }
+			return true, nil
-		return b
+		}
 
 		// if there is no watcher, but there is an onlyif check, run it to see
 	} else if obj.IfCmd != "" { // && obj.WatchCmd == ""
@@ -229,20 +231,23 @@ func (obj *ExecRes) StateOK() bool {
 		err := exec.Command(cmdName, cmdArgs...).Run()
 		if err != nil {
 			// TODO: check exit value
-			return true // don't run
+			return true, nil // don't run
 		}
-		return false // just run
 
 		// if there is no watcher and no onlyif check, assume we should run
 	} else { // if obj.WatchCmd == "" && obj.IfCmd == "" {
-		b := obj.isStateOK
+		// just run if state is dirty
-		obj.isStateOK = true
+		if obj.isStateOK {
-		return b // just run if state is dirty
+			return true, nil
+		}
 	}
-}
 
-func (obj *ExecRes) Apply() bool {
+	// state is not okay, no work done, exit, but without error
-	log.Printf("%v[%v]: Apply", obj.GetRes(), obj.GetName())
+	if !apply {
+		return false, nil
+	}
+
+	// apply portion
 	var cmdName string
 	var cmdArgs []string
 	if obj.Shell == "" {
@@ -263,9 +268,9 @@ func (obj *ExecRes) Apply() bool {
 	var out bytes.Buffer
 	cmd.Stdout = &out
 
-	if err := cmd.Start(); err != nil {
+	if err = cmd.Start(); err != nil {
 		log.Printf("%v[%v]: Error starting Cmd: %v", obj.GetRes(), obj.GetName(), err)
-		return false
+		return false, err
 	}
 
 	timeout := obj.Timeout
@@ -276,16 +281,16 @@ func (obj *ExecRes) Apply() bool {
 	go func() { done <- cmd.Wait() }()
 
 	select {
-	case err := <-done:
+	case err = <-done:
 		if err != nil {
 			log.Printf("%v[%v]: Error waiting for Cmd: %v", obj.GetRes(), obj.GetName(), err)
-			return false
+			return false, err
 		}
 
 	case <-TimeAfterOrBlock(timeout):
 		log.Printf("%v[%v]: Timeout waiting for Cmd", obj.GetRes(), obj.GetName())
 		//cmd.Process.Kill() // TODO: is this necessary?
-		return false
+		return false, errors.New("Timeout waiting for Cmd!")
 	}
 
 	// TODO: if we printed the stdout while the command is running, this
@@ -298,7 +303,13 @@ func (obj *ExecRes) Apply() bool {
 		log.Printf(out.String())
 	}
 	// XXX: return based on exit value!!
-	return true
+
+	// the state tracking is for exec resources that can't "detect" their
+	// state, and assume it's invalid when the Watch() function triggers.
+	// if we apply state successfully, we should reset it here so that we
+	// know that we have applied since the state was set not ok by event!
+	obj.isStateOK = true // reset
+	return false, nil    // success
 }
 
 func (obj *ExecRes) Compare(res Res) bool {

file.go

@@ -62,6 +62,20 @@ func (obj *FileRes) GetRes() string {
 	return "File"
 }
 
+func (obj *FileRes) GetPath() string {
+	d := Dirname(obj.Path)
+	b := Basename(obj.Path)
+	if !obj.Validate() || (obj.Dirname == "" && obj.Basename == "") {
+		return obj.Path
+	} else if obj.Dirname == "" {
+		return d + obj.Basename
+	} else if obj.Basename == "" {
+		return obj.Dirname + b
+	} else { // if obj.dirname != "" && obj.basename != "" {
+		return obj.Dirname + obj.Basename
+	}
+}
+
 // validate if the params passed in are valid data
 func (obj *FileRes) Validate() bool {
 	if obj.Dirname != "" {
@@ -79,20 +93,6 @@ func (obj *FileRes) Validate() bool {
 	return true
 }
 
-func (obj *FileRes) GetPath() string {
-	d := Dirname(obj.Path)
-	b := Basename(obj.Path)
-	if !obj.Validate() || (obj.Dirname == "" && obj.Basename == "") {
-		return obj.Path
-	} else if obj.Dirname == "" {
-		return d + obj.Basename
-	} else if obj.Basename == "" {
-		return obj.Dirname + b
-	} else { // if obj.dirname != "" && obj.basename != "" {
-		return obj.Dirname + obj.Basename
-	}
-}
-
 // File watcher for files and directories
 // Modify with caution, probably important to write some test cases first!
 // obj.GetPath(): file or directory
@@ -270,84 +270,29 @@ func (obj *FileRes) HashSHA256fromContent() string {
 	return obj.sha256sum
 }
 
-// FIXME: add the obj.CleanState() calls all over the true returns!
+func (obj *FileRes) FileHashSHA256Check() (bool, error) {
-func (obj *FileRes) StateOK() bool {
+	if PathIsDir(obj.GetPath()) { // assert
-	if obj.isStateOK { // cache the state
-		return true
-	}
-
-	if _, err := os.Stat(obj.GetPath()); os.IsNotExist(err) {
-		// no such file or directory
-		if obj.State == "absent" {
-			return obj.CleanState() // missing file should be missing, phew :)
-		} else {
-			// state invalid, skip expensive checksums
-			return false
-		}
-	}
-
-	// TODO: add file mode check here...
-
-	if PathIsDir(obj.GetPath()) {
-		return obj.StateOKDir()
-	} else {
-		return obj.StateOKFile()
-	}
-}
-
-func (obj *FileRes) StateOKFile() bool {
-	if PathIsDir(obj.GetPath()) {
 		log.Fatal("This should only be called on a File resource.")
 	}
-
 	// run a diff, and return true if needs changing
-
 	hash := sha256.New()
-
 	f, err := os.Open(obj.GetPath())
 	if err != nil {
-		//log.Fatal(err)
+		return false, err
-		return false
 	}
 	defer f.Close()
-
 	if _, err := io.Copy(hash, f); err != nil {
-		//log.Fatal(err)
+		return false, err
-		return false
 	}
-
 	sha256sum := hex.EncodeToString(hash.Sum(nil))
 	//log.Printf("sha256sum: %v", sha256sum)
-
 	if obj.HashSHA256fromContent() == sha256sum {
-		return true
+		return true, nil
 	}
-
+	return false, nil
-	return false
 }
 
-func (obj *FileRes) StateOKDir() bool {
+func (obj *FileRes) FileApply() error {
-	if !PathIsDir(obj.GetPath()) {
-		log.Fatal("This should only be called on a Dir resource.")
-	}
-
-	// XXX: not implemented
-	log.Fatal("Not implemented!")
-	return false
-}
-
-func (obj *FileRes) Apply() bool {
-	log.Printf("%v[%v]: Apply", obj.GetRes(), obj.GetName())
-
-	if PathIsDir(obj.GetPath()) {
-		return obj.ApplyDir()
-	} else {
-		return obj.ApplyFile()
-	}
-}
-
-func (obj *FileRes) ApplyFile() bool {
-
 	if PathIsDir(obj.GetPath()) {
 		log.Fatal("This should only be called on a File resource.")
 	}
@@ -355,37 +300,72 @@ func (obj *FileRes) ApplyFile() bool {
 	if obj.State == "absent" {
 		log.Printf("About to remove: %v", obj.GetPath())
 		err := os.Remove(obj.GetPath())
-		if err != nil {
+		return err // either nil or not, for success or failure
-			return false
-		}
-		return true
 	}
 
-	//log.Println("writing: " + filename)
 	f, err := os.Create(obj.GetPath())
 	if err != nil {
-		log.Println("error:", err)
+		return nil
-		return false
 	}
 	defer f.Close()
 
 	_, err = io.WriteString(f, obj.Content)
 	if err != nil {
-		log.Println("error:", err)
+		return err
-		return false
 	}
 
-	return true
+	return nil // success
 }
 
-func (obj *FileRes) ApplyDir() bool {
+func (obj *FileRes) CheckApply(apply bool) (stateok bool, err error) {
-	if !PathIsDir(obj.GetPath()) {
+	log.Printf("%v[%v]: CheckApply(%t)", obj.GetRes(), obj.GetName(), apply)
-		log.Fatal("This should only be called on a Dir resource.")
+
+	if obj.isStateOK { // cache the state
+		return true, nil
 	}
 
-	// XXX: not implemented
+	if _, err := os.Stat(obj.GetPath()); os.IsNotExist(err) {
-	log.Fatal("Not implemented!")
+		// no such file or directory
-	return true
+		if obj.State == "absent" {
+			// missing file should be missing, phew :)
+			obj.isStateOK = true
+			return true, nil
+		}
+	}
+
+	// FIXME: add file mode check here...
+
+	if PathIsDir(obj.GetPath()) {
+		log.Fatal("Not implemented!") // XXX
+	} else {
+		ok, err := obj.FileHashSHA256Check()
+		if err != nil {
+			return false, err
+		}
+		if ok {
+			obj.isStateOK = true
+			return true, nil
+		}
+		// if no err, but !ok, then we continue on...
+	}
+
+	// state is not okay, no work done, exit, but without error
+	if !apply {
+		return false, nil
+	}
+
+	// apply portion
+	if PathIsDir(obj.GetPath()) {
+		log.Fatal("Not implemented!") // XXX
+	} else {
+		err = obj.FileApply()
+		if err != nil {
+			return false, err
+		}
+	}
+
+	obj.isStateOK = true
+	return false, nil // success
 }
 
 func (obj *FileRes) Compare(res Res) bool {

resources.go

@@ -47,8 +47,7 @@ type Res interface {
 	GetName() string // can't be named "Name()" because of struct field
 	GetRes() string
 	Watch()
-	StateOK() bool // TODO: can we rename this to something better?
+	CheckApply(bool) (bool, error)
-	Apply() bool
 	SetVertex(*Vertex)
 	SetConvergedCallback(ctimeout int, converged chan bool)
 	Compare(Res) bool
@@ -101,7 +100,7 @@ func (obj *BaseRes) Init() {
 	obj.events = make(chan Event)
 }
 
-// this method gets used by all the types, if we have one of (obj NoopRes) it would get overridden in that case!
+// this method gets used by all the resources, if we have one of (obj NoopRes) it would get overridden in that case!
 func (obj *BaseRes) GetName() string {
 	return obj.Name
 }
@@ -293,13 +292,6 @@ func (obj *BaseRes) ReadEvent(event *Event) (exit, poke bool) {
 	return true, false // required to keep the stupid go compiler happy
 }
 
-// useful for using as: return CleanState() in the StateOK functions when there
-// are multiple `true` return exits
-func (obj *BaseRes) CleanState() bool {
-	obj.isStateOK = true
-	return true
-}
-
 // XXX: rename this function
 func Process(obj Res) {
 	if DEBUG {
@@ -315,14 +307,19 @@ func Process(obj Res) {
 		if DEBUG {
 			log.Printf("%v[%v]: OKTimestamp(%v)", obj.GetRes(), obj.GetName(), obj.GetTimestamp())
 		}
-		if !obj.StateOK() { // TODO: can we rename this to something better?
+
-			if DEBUG {
+		obj.SetState(resStateCheckApply)
-				log.Printf("%v[%v]: !StateOK()", obj.GetRes(), obj.GetName())
+		// if this fails, don't UpdateTimestamp()
-			}
+		stateok, err := obj.CheckApply(true)
-			// throw an error if apply fails...
+		if stateok && err != nil { // should never return this way
-			// if this fails, don't UpdateTimestamp()
+			log.Fatalf("%v[%v]: CheckApply(): %t, %+v", obj.GetRes(), obj.GetName(), stateok, err)
-			obj.SetState(resStateCheckApply)
+		}
-			if !obj.Apply() { // check for error
+		if DEBUG {
+			log.Printf("%v[%v]: CheckApply(): %t, %v", obj.GetRes(), obj.GetName(), stateok, err)
+		}
+
+		if !stateok { // if state *was* not ok, we had to have apply'ed
+			if err != nil { // error during check or apply
 				ok = false
 			} else {
 				apply = true
@@ -347,6 +344,12 @@ func (obj *NoopRes) GetRes() string {
 	return "Noop"
 }
 
+// validate if the params passed in are valid data
+// FIXME: where should this get called ?
+func (obj *NoopRes) Validate() bool {
+	return true
+}
+
 func (obj *NoopRes) Watch() {
 	if obj.IsWatching() {
 		return
@@ -383,13 +386,10 @@ func (obj *NoopRes) Watch() {
 	}
 }
 
-func (obj *NoopRes) StateOK() bool {
+// CheckApply method for Noop resource. Does nothing, returns happy!
-	return true // never needs updating
+func (obj *NoopRes) CheckApply(apply bool) (stateok bool, err error) {
-}
+	log.Printf("%v[%v]: CheckApply(%t)", obj.GetRes(), obj.GetName(), apply)
-
+	return true, nil // state is always okay
-func (obj *NoopRes) Apply() bool {
-	log.Printf("%v[%v]: Apply", obj.GetRes(), obj.GetName())
-	return true
 }
 
 func (obj *NoopRes) Compare(res Res) bool {

svc.go

@@ -15,21 +15,22 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-// NOTE: docs are found at: https://godoc.org/github.com/coreos/go-systemd/dbus
+// DOCS: https://godoc.org/github.com/coreos/go-systemd/dbus
 
 package main
 
 import (
+	"errors"
 	"fmt"
 	systemd "github.com/coreos/go-systemd/dbus" // change namespace
-	"github.com/coreos/go-systemd/util"
+	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus" // namespace collides with systemd wrapper
 	"log"
 )
 
 type SvcRes struct {
 	BaseRes `yaml:",inline"`
-	State   string `yaml:"state"`   // state: running, stopped
+	State   string `yaml:"state"`   // state: running, stopped, undefined
 	Startup string `yaml:"startup"` // enabled, disabled, undefined
 }
 
@@ -49,6 +50,16 @@ func (obj *SvcRes) GetRes() string {
 	return "Svc"
 }
 
+func (obj *SvcRes) Validate() bool {
+	if obj.State != "running" && obj.State != "stopped" && obj.State != "" {
+		return false
+	}
+	if obj.Startup != "enabled" && obj.Startup != "disabled" && obj.Startup != "" {
+		return false
+	}
+	return true
+}
+
 // Service watcher
 func (obj *SvcRes) Watch() {
 	if obj.IsWatching() {
@@ -59,7 +70,7 @@ func (obj *SvcRes) Watch() {
 
 	// obj.Name: svc name
 	//vertex := obj.GetVertex()         // stored with SetVertex
-	if !util.IsRunningSystemd() {
+	if !systemdUtil.IsRunningSystemd() {
 		log.Fatal("Systemd is not running.")
 	}
 
@@ -203,18 +214,20 @@ func (obj *SvcRes) Watch() {
 	}
 }
 
-func (obj *SvcRes) StateOK() bool {
+func (obj *SvcRes) CheckApply(apply bool) (stateok bool, err error) {
+	log.Printf("%v[%v]: CheckApply(%t)", obj.GetRes(), obj.GetName(), apply)
+
 	if obj.isStateOK { // cache the state
-		return true
+		return true, nil
 	}
 
-	if !util.IsRunningSystemd() {
+	if !systemdUtil.IsRunningSystemd() {
-		log.Fatal("Systemd is not running.")
+		return false, errors.New("Systemd is not running.")
 	}
 
 	conn, err := systemd.NewSystemdConnection() // needs root access
 	if err != nil {
-		log.Fatal("Failed to connect to systemd: ", err)
+		return false, errors.New(fmt.Sprintf("Failed to connect to systemd: %v", err))
 	}
 	defer conn.Close()
 
@@ -222,15 +235,13 @@ func (obj *SvcRes) StateOK() bool {
 
 	loadstate, err := conn.GetUnitProperty(svc, "LoadState")
 	if err != nil {
-		log.Printf("Failed to get load state: %v", err)
+		return false, errors.New(fmt.Sprintf("Failed to get load state: %v", err))
-		return false
 	}
 
 	// NOTE: we have to compare variants with other variants, they are really strings...
 	var notFound = (loadstate.Value == dbus.MakeVariant("not-found"))
 	if notFound {
-		log.Printf("Failed to find svc: %v", svc)
+		return false, errors.New(fmt.Sprintf("Failed to find svc: %v", svc))
-		return false
 	}
 
 	// XXX: check svc "enabled at boot" or not status...
@@ -238,85 +249,61 @@ func (obj *SvcRes) StateOK() bool {
 	//conn.GetUnitProperties(svc)
 	activestate, err := conn.GetUnitProperty(svc, "ActiveState")
 	if err != nil {
-		log.Fatal("Failed to get active state: ", err)
+		return false, errors.New(fmt.Sprintf("Failed to get active state: %v", err))
 	}
 
 	var running = (activestate.Value == dbus.MakeVariant("active"))
+	var stateOK = ((obj.State == "") || (obj.State == "running" && running) || (obj.State == "stopped" && !running))
+	var startupOK = true // XXX DETECT AND SET
 
-	if obj.State == "running" {
+	if stateOK && startupOK {
-		if !running {
+		return true, nil // we are in the correct state
-			return false // we are in the wrong state
-		}
-	} else if obj.State == "stopped" {
-		if running {
-			return false
-		}
-	} else {
-		log.Fatal("Unknown state: ", obj.State)
 	}
 
-	return true // all is good, no state change needed
+	// state is not okay, no work done, exit, but without error
-}
+	if !apply {
-
+		return false, nil
-func (obj *SvcRes) Apply() bool {
-	log.Printf("%v[%v]: Apply", obj.GetRes(), obj.GetName())
-
-	if !util.IsRunningSystemd() {
-		log.Fatal("Systemd is not running.")
 	}
 
-	conn, err := systemd.NewSystemdConnection() // needs root access
+	// apply portion
-	if err != nil {
+	var files = []string{svc} // the svc represented in a list
-		log.Fatal("Failed to connect to systemd: ", err)
-	}
-	defer conn.Close()
-
-	var svc = fmt.Sprintf("%v.service", obj.Name) // systemd name
-	var files = []string{svc}                     // the svc represented in a list
 	if obj.Startup == "enabled" {
 		_, _, err = conn.EnableUnitFiles(files, false, true)
 
 	} else if obj.Startup == "disabled" {
 		_, err = conn.DisableUnitFiles(files, false)
-	} else {
-		err = nil
-	}
-	if err != nil {
-		log.Printf("Unable to change startup status: %v", err)
-		return false
 	}
 
+	if err != nil {
+		return false, errors.New(fmt.Sprintf("Unable to change startup status: %v", err))
+	}
+
+	// XXX: do we need to use a buffered channel here?
 	result := make(chan string, 1) // catch result information
 
 	if obj.State == "running" {
-		_, err := conn.StartUnit(svc, "fail", result)
+		_, err = conn.StartUnit(svc, "fail", result)
 		if err != nil {
-			log.Fatal("Failed to start unit: ", err)
+			return false, errors.New(fmt.Sprintf("Failed to start unit: %v", err))
-			return false
 		}
 	} else if obj.State == "stopped" {
 		_, err = conn.StopUnit(svc, "fail", result)
 		if err != nil {
-			log.Fatal("Failed to stop unit: ", err)
+			return false, errors.New(fmt.Sprintf("Failed to stop unit: %v", err))
-			return false
 		}
-	} else {
-		log.Fatal("Unknown state: ", obj.State)
 	}
 
 	status := <-result
 	if &status == nil {
-		log.Fatal("Result is nil")
+		return false, errors.New("Systemd service action result is nil")
-		return false
 	}
 	if status != "done" {
-		log.Fatal("Unknown return string: ", status)
+		return false, errors.New(fmt.Sprintf("Unknown systemd return string: %v", status))
-		return false
 	}
 
 	// XXX: also set enabled on boot
 
-	return true
+	return false, nil // success
 }
 
 func (obj *SvcRes) Compare(res Res) bool {