misc: Improve tagging script

This way we can push the tag *after* all the builds succeed. If
something goes wrong, we can always delete our local tag and try again.

.github/FUNDING.yml

@@ -0,0 +1,2 @@
+# You can add one username per supported platform and one custom link
+patreon: purpleidea

.travis.yml

@@ -24,21 +24,21 @@ install: 'make deps'
 matrix:
   fast_finish: false
   allow_failures:
-  - go: 1.11.x
+  - go: 1.12.x
   - go: tip
   - os: osx
   # include only one build for osx for a quicker build as the nr. of these runners are sparse
   include:
   - name: "basic tests"
-    go: 1.10.x
+    go: 1.11.x
     env: TEST_BLOCK=basic
   - name: "shell tests"
-    go: 1.10.x
+    go: 1.11.x
     env: TEST_BLOCK=shell
   - name: "race tests"
-    go: 1.10.x
+    go: 1.11.x
     env: TEST_BLOCK=race
-  - go: 1.11.x
+  - go: 1.12.x
   - go: tip
   - os: osx
 script: 'TEST_BLOCK="$TEST_BLOCK" make test'

COPYRIGHT

@@ -1,5 +1,5 @@
 Mgmt
-Copyright (C) 2013-2018+ James Shubin and the project contributors
+Copyright (C) 2013-2019+ James Shubin and the project contributors
 Written by James Shubin <james@shubin.ca> and the project contributors
 
 This program is free software: you can redistribute it and/or modify

Makefile

@@ -1,5 +1,5 @@
 # Mgmt
-# Copyright (C) 2013-2018+ James Shubin and the project contributors
+# Copyright (C) 2013-2019+ James Shubin and the project contributors
 # Written by James Shubin <james@shubin.ca> and the project contributors
 #
 # This program is free software: you can redistribute it and/or modify
@@ -16,11 +16,16 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 SHELL = /usr/bin/env bash
-.PHONY: all art cleanart version program lang path deps run race bindata generate build build-debug crossbuild clean test gofmt yamlfmt format docs rpmbuild mkdirs rpm srpm spec tar upload upload-sources upload-srpms upload-rpms copr tag release funcgen
+.PHONY: all art cleanart version program lang path deps run race bindata generate build build-debug crossbuild clean test gofmt yamlfmt format docs
+.PHONY: rpmbuild mkdirs rpm srpm spec tar upload upload-sources upload-srpms upload-rpms upload-releases copr tag
+.PHONY: mkosi mkosi_fedora-30 mkosi_fedora-29 mkosi_debian-10 mkosi_ubuntu-bionic mkosi_archlinux
+.PHONY: release releases_path release_fedora-30 release_fedora-29 release_debian-10 release_ubuntu-bionic release_archlinux
+.PHONY: funcgen
 .SILENT: clean bindata
 
 # a large amount of output from this `find`, can cause `make` to be much slower!
 GO_FILES := $(shell find * -name '*.go' -not -path 'old/*' -not -path 'tmp/*')
+MCL_FILES := $(shell find lang/funcs/ -name '*.mcl' -not -path 'old/*' -not -path 'tmp/*')
 
 SVERSION := $(or $(SVERSION),$(shell git describe --match '[0-9]*\.[0-9]*\.[0-9]*' --tags --dirty --always))
 VERSION := $(or $(VERSION),$(shell git describe --match '[0-9]*\.[0-9]*\.[0-9]*' --tags --abbrev=0))
@@ -48,9 +53,23 @@ GOOSARCHES ?= linux/amd64 linux/ppc64 linux/ppc64le linux/arm64 darwin/amd64
 GOHOSTOS = $(shell go env GOHOSTOS)
 GOHOSTARCH = $(shell go env GOHOSTARCH)
 
-RPM_PKG = releases/$(VERSION)/rpm/mgmt-$(VERSION)-1.x86_64.rpm
-DEB_PKG = releases/$(VERSION)/deb/mgmt_$(VERSION)_amd64.deb
-PACMAN_PKG = releases/$(VERSION)/pacman/mgmt-$(VERSION)-1-x86_64.pkg.tar.xz
+TOKEN_FEDORA-30 = fedora-30
+TOKEN_FEDORA-29 = fedora-29
+TOKEN_DEBIAN-10 = debian-10
+TOKEN_UBUNTU-BIONIC = ubuntu-bionic
+TOKEN_ARCHLINUX = archlinux
+
+FILE_FEDORA-30 = mgmt-$(TOKEN_FEDORA-30)-$(VERSION)-1.x86_64.rpm
+FILE_FEDORA-29 = mgmt-$(TOKEN_FEDORA-29)-$(VERSION)-1.x86_64.rpm
+FILE_DEBIAN-10 = mgmt_$(TOKEN_DEBIAN-10)_$(VERSION)_amd64.deb
+FILE_UBUNTU-BIONIC = mgmt_$(TOKEN_UBUNTU-BIONIC)_$(VERSION)_amd64.deb
+FILE_ARCHLINUX = mgmt-$(TOKEN_ARCHLINUX)-$(VERSION)-1-x86_64.pkg.tar.xz
+
+PKG_FEDORA-30 = releases/$(VERSION)/$(TOKEN_FEDORA-30)/$(FILE_FEDORA-30)
+PKG_FEDORA-29 = releases/$(VERSION)/$(TOKEN_FEDORA-29)/$(FILE_FEDORA-29)
+PKG_DEBIAN-10 = releases/$(VERSION)/$(TOKEN_DEBIAN-10)/$(FILE_DEBIAN-10)
+PKG_UBUNTU-BIONIC = releases/$(VERSION)/$(TOKEN_UBUNTU-BIONIC)/$(FILE_UBUNTU-BIONIC)
+PKG_ARCHLINUX = releases/$(VERSION)/$(TOKEN_ARCHLINUX)/$(FILE_ARCHLINUX)
 
 SHA256SUMS = releases/$(VERSION)/SHA256SUMS
 SHA256SUMS_ASC = $(SHA256SUMS).asc
@@ -131,7 +150,7 @@ lang: ## generates the lexer/parser for the language frontend
 $(PROGRAM): build/mgmt-${GOHOSTOS}-${GOHOSTARCH} ## build an mgmt binary for current host os/arch
 	cp -a $< $@
 
-$(PROGRAM).static: $(GO_FILES)
+$(PROGRAM).static: $(GO_FILES) $(MCL_FILES)
 	@echo "Building: $(PROGRAM).static, version: $(SVERSION)..."
 	go generate
 	go build -a -installsuffix cgo -tags netgo -ldflags '-extldflags "-static" -X main.program=$(PROGRAM) -X main.version=$(SVERSION) -s -w' -o $(PROGRAM).static $(BUILD_FLAGS);
@@ -146,10 +165,10 @@ build-debug: $(PROGRAM)
 # extract os and arch from target pattern
 GOOS=$(firstword $(subst -, ,$*))
 GOARCH=$(lastword $(subst -, ,$*))
-build/mgmt-%: $(GO_FILES) | bindata lang funcgen
+build/mgmt-%: $(GO_FILES) $(MCL_FILES) | bindata lang funcgen
 	@echo "Building: $(PROGRAM), os/arch: $*, version: $(SVERSION)..."
 	@# reassigning GOOS and GOARCH to make build command copy/pastable
-	@# go 1.10 requires specifying the package for ldflags
+	@# go 1.10+ requires specifying the package for ldflags
 	@if go version | grep -qE 'go1.9'; then \
 		time env GOOS=${GOOS} GOARCH=${GOARCH} go build -i -ldflags "-X main.program=$(PROGRAM) -X main.version=$(SVERSION) ${LDFLAGS}" -o $@ $(BUILD_FLAGS); \
 	else \
@@ -164,6 +183,7 @@ clean: ## clean things up
 	$(MAKE) --quiet -C bindata clean
 	$(MAKE) --quiet -C lang/funcs clean
 	$(MAKE) --quiet -C lang clean
+	$(MAKE) --quiet -C misc/mkosi clean
 	rm -f lang/funcs/core/generated_funcs.go || true
 	rm -f lang/funcs/core/generated_funcs_test.go || true
 	[ ! -e $(PROGRAM) ] || rm $(PROGRAM)
@@ -188,8 +208,8 @@ $(addprefix test-shell-,${test_shell}): test-shell-%: build
 
 gofmt:
 	# TODO: remove gofmt once goimports has a -s option
-	find . -maxdepth 6 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec gofmt -s -w {} \;
-	find . -maxdepth 6 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec goimports -w {} \;
+	find . -maxdepth 9 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec gofmt -s -w {} \;
+	find . -maxdepth 9 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec goimports -w {} \;
 
 yamlfmt:
 	find . -maxdepth 3 -type f -name '*.yaml' -not -path './old/*' -not -path './tmp/*' -not -path './omv.yaml' -exec ruby -e "require 'yaml'; x=YAML.load_file('{}').to_yaml.each_line.map(&:rstrip).join(10.chr)+10.chr; File.open('{}', 'w').write x" \;
@@ -326,6 +346,10 @@ upload-rpms: rpmbuild/RPMS/ rpmbuild/RPMS/SHA256SUMS rpmbuild/RPMS/SHA256SUMS.as
 		rsync -avz --prune-empty-dirs rpmbuild/RPMS/ $(SERVER):$(REMOTE_PATH)/RPMS/; \
 	fi
 
+upload-releases:
+	echo Running releases/ upload...
+	rsync -avz --exclude '.mkdir' --exclude 'mgmt-release.url' releases/ $(SERVER):$(REMOTE_PATH)/releases/
+
 #
 #	copr build
 #
@@ -338,18 +362,57 @@ copr: upload-srpms ## build in copr
 tag: ## tags a new release
 	./misc/tag.sh
 
+#
+#	mkosi
+#
+mkosi: mkosi_fedora-30 mkosi_fedora-29 mkosi_debian-10 mkosi_ubuntu-bionic mkosi_archlinux ## builds distro packages via mkosi
+
+mkosi_fedora-30: releases/$(VERSION)/.mkdir
+	@title='$@' ; echo "Generating: $${title#'mkosi_'} via mkosi..."
+	@title='$@' ; distro=$${title#'mkosi_'} ; ./misc/mkosi/make.sh $${distro} `realpath "releases/$(VERSION)/"`
+
+mkosi_fedora-29: releases/$(VERSION)/.mkdir
+	@title='$@' ; echo "Generating: $${title#'mkosi_'} via mkosi..."
+	@title='$@' ; distro=$${title#'mkosi_'} ; ./misc/mkosi/make.sh $${distro} `realpath "releases/$(VERSION)/"`
+
+mkosi_debian-10: releases/$(VERSION)/.mkdir
+	@title='$@' ; echo "Generating: $${title#'mkosi_'} via mkosi..."
+	@title='$@' ; distro=$${title#'mkosi_'} ; ./misc/mkosi/make.sh $${distro} `realpath "releases/$(VERSION)/"`
+
+mkosi_ubuntu-bionic: releases/$(VERSION)/.mkdir
+	@title='$@' ; echo "Generating: $${title#'mkosi_'} via mkosi..."
+	@title='$@' ; distro=$${title#'mkosi_'} ; ./misc/mkosi/make.sh $${distro} `realpath "releases/$(VERSION)/"`
+
+mkosi_archlinux: releases/$(VERSION)/.mkdir
+	@title='$@' ; echo "Generating: $${title#'mkosi_'} via mkosi..."
+	@title='$@' ; distro=$${title#'mkosi_'} ; ./misc/mkosi/make.sh $${distro} `realpath "releases/$(VERSION)/"`
+
 #
 #	release
 #
 release: releases/$(VERSION)/mgmt-release.url ## generates and uploads a release
 
-releases/$(VERSION)/mgmt-release.url: $(RPM_PKG) $(DEB_PKG) $(PACMAN_PKG) $(SHA256SUMS_ASC)
+releases_path:
+	@#Don't put any other output or dependencies in here or they'll show!
+	@echo "releases/$(VERSION)/"
+
+release_fedora-30: $(PKG_FEDORA-30)
+release_fedora-29: $(PKG_FEDORA-29)
+release_debian-10: $(PKG_DEBIAN-10)
+release_ubuntu-bionic: $(PKG_UBUNTU-BIONIC)
+release_archlinux: $(PKG_ARCHLINUX)
+
+releases/$(VERSION)/mgmt-release.url: $(PKG_FEDORA-30) $(PKG_FEDORA-29) $(PKG_DEBIAN-10) $(PKG_UBUNTU-BIONIC) $(PKG_ARCHLINUX) $(SHA256SUMS_ASC)
+	@echo "Pushing git tag $(VERSION) to origin..."
+	git push origin $(VERSION)
 	@echo "Creating github release..."
 	hub release create \
 		-F <( echo -e "$(VERSION)\n";echo "Verify the signatures of all packages before you use them. The signing key can be downloaded from https://purpleidea.com/contact/#pgp-key to verify the release." ) \
-		-a $(RPM_PKG) \
-		-a $(DEB_PKG) \
-		-a $(PACMAN_PKG) \
+		-a $(PKG_FEDORA-30) \
+		-a $(PKG_FEDORA-29) \
+		-a $(PKG_DEBIAN-10) \
+		-a $(PKG_UBUNTU-BIONIC) \
+		-a $(PKG_ARCHLINUX) \
 		-a $(SHA256SUMS_ASC) \
 		$(VERSION) \
 		> releases/$(VERSION)/mgmt-release.url \
@@ -357,32 +420,48 @@ releases/$(VERSION)/mgmt-release.url: $(RPM_PKG) $(DEB_PKG) $(PACMAN_PKG) $(SHA2
 		|| rm -f releases/$(VERSION)/mgmt-release.url
 
 releases/$(VERSION)/.mkdir:
-	mkdir -p releases/$(VERSION)/{deb,rpm,pacman}/ && touch releases/$(VERSION)/.mkdir
+	mkdir -p releases/$(VERSION)/{$(TOKEN_FEDORA-30),$(TOKEN_FEDORA-29),$(TOKEN_DEBIAN-10),$(TOKEN_UBUNTU-BIONIC),$(TOKEN_ARCHLINUX)}/ && touch releases/$(VERSION)/.mkdir
 
-releases/$(VERSION)/rpm/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
-	@echo "Generating: rpm changelog..."
-	./misc/make-rpm-changelog.sh $(VERSION)
+releases/$(VERSION)/$(TOKEN_FEDORA-30)/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Generating: $${distro} changelog..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/make-rpm-changelog.sh "$${distro}" $(VERSION)
 
-$(RPM_PKG): releases/$(VERSION)/rpm/changelog
-	@echo "Building: rpm package..."
-	./misc/fpm-pack.sh rpm $(VERSION) libvirt-devel augeas-devel
+$(PKG_FEDORA-30): releases/$(VERSION)/$(TOKEN_FEDORA-30)/changelog
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Building: $${distro} package..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/fpm-pack.sh $${distro} $(VERSION) "$(FILE_FEDORA-30)" libvirt-devel augeas-devel
 
-releases/$(VERSION)/deb/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
-	@echo "Generating: deb changelog..."
-	./misc/make-deb-changelog.sh $(VERSION)
+releases/$(VERSION)/$(TOKEN_FEDORA-29)/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Generating: $${distro} changelog..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/make-rpm-changelog.sh "$${distro}" $(VERSION)
 
-$(DEB_PKG): releases/$(VERSION)/deb/changelog
-	@echo "Building: deb package..."
-	./misc/fpm-pack.sh deb $(VERSION) libvirt-dev libaugeas-dev
+$(PKG_FEDORA-29): releases/$(VERSION)/$(TOKEN_FEDORA-29)/changelog
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Building: $${distro} package..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/fpm-pack.sh $${distro} $(VERSION) "$(FILE_FEDORA-29)" libvirt-devel augeas-devel
 
-$(PACMAN_PKG): $(PROGRAM) releases/$(VERSION)/.mkdir
-	@echo "Building: pacman package..."
-	./misc/fpm-pack.sh pacman $(VERSION) libvirt augeas
+releases/$(VERSION)/$(TOKEN_DEBIAN-10)/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Generating: $${distro} changelog..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/make-deb-changelog.sh "$${distro}" $(VERSION)
 
-$(SHA256SUMS): $(RPM_PKG) $(DEB_PKG) $(PACMAN_PKG)
+$(PKG_DEBIAN-10): releases/$(VERSION)/$(TOKEN_DEBIAN-10)/changelog
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Building: $${distro} package..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/fpm-pack.sh $${distro} $(VERSION) "$(FILE_DEBIAN-10)" libvirt-dev libaugeas-dev
+
+releases/$(VERSION)/$(TOKEN_UBUNTU-BIONIC)/changelog: $(PROGRAM) releases/$(VERSION)/.mkdir
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Generating: $${distro} changelog..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/make-deb-changelog.sh "$${distro}" $(VERSION)
+
+$(PKG_UBUNTU-BIONIC): releases/$(VERSION)/$(TOKEN_UBUNTU-BIONIC)/changelog
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Building: $${distro} package..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/fpm-pack.sh $${distro} $(VERSION) "$(FILE_UBUNTU-BIONIC)" libvirt-dev libaugeas-dev
+
+$(PKG_ARCHLINUX): $(PROGRAM) releases/$(VERSION)/.mkdir
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; echo "Building: $${distro} package..."
+	@title='$(@D)' ; distro=$${title#'releases/$(VERSION)/'} ; ./misc/fpm-pack.sh $${distro} $(VERSION) "$(FILE_ARCHLINUX)" libvirt augeas
+
+$(SHA256SUMS): $(PKG_FEDORA-30) $(PKG_FEDORA-29) $(PKG_DEBIAN-10) $(PKG_UBUNTU-BIONIC) $(PKG_ARCHLINUX)
 	@# remove the directory separator in the SHA256SUMS file
 	@echo "Generating: sha256 sum..."
-	sha256sum $(RPM_PKG) $(DEB_PKG) $(PACMAN_PKG) | awk -F '/| ' '{print $$1"  "$$6}' > $(SHA256SUMS)
+	sha256sum $(PKG_FEDORA-30) $(PKG_FEDORA-29) $(PKG_DEBIAN-10) $(PKG_UBUNTU-BIONIC) $(PKG_ARCHLINUX) | awk -F '/| ' '{print $$1"  "$$6}' > $(SHA256SUMS)
 
 $(SHA256SUMS_ASC): $(SHA256SUMS)
 	@echo "Signing sha256 sum..."

README.md

@@ -9,6 +9,56 @@
 [![Patreon](https://img.shields.io/badge/patreon-donate-yellow.svg?style=flat-square)](https://www.patreon.com/purpleidea)
 [![Liberapay](https://img.shields.io/badge/liberapay-donate-yellow.svg?style=flat-square)](https://liberapay.com/purpleidea/donate)
 
+## About:
+
+`Mgmt` is a real-time automation tool. It is familiar to existing configuration
+management software, but is drastically more powerful as it can allow you to
+build real-time, closed-loop feedback systems, in a very safe way, and with a
+surprisingly small amout of our `mcl` code. For example, the following code will
+ensure that your file server is set to read-only when it's friday.
+
+```mcl
+import "datetime"
+$is_friday = datetime.weekday(datetime.now()) == "friday"
+file "/srv/files/" {
+	state => "exists",
+	mode => if $is_friday { # this updates the mode, the instant it changes!
+		"0550"
+	} else {
+		"0770"
+	},
+}
+```
+
+It can run continuously, intermittently, or on-demand, and in the first case, it
+will guarantee that your system is always in the desired state for that instant!
+In this mode it can run as a decentralized cluster of agents across your
+network, each exchanging information with the others in real-time, to respond to
+your changing needs. For example, if you want to ensure that some resource runs
+on a maximum of two hosts in your cluster, you can specify that as well:
+
+```mcl
+import "sys"
+import "world"
+
+# we'll set a few scheduling options:
+$opts = struct{strategy => "rr", max => 2, ttl => 10,}
+
+# schedule in a particular namespace with options:
+$set = world.schedule("xsched", $opts)
+
+if sys.hostname() in $set {
+	# use your imagination to put something more complex right here...
+	print "i got scheduled" {} # this will run on the chosen machines
+}
+```
+
+As you add and remove hosts from the cluster, the real-time `schedule` function
+will dynamically pick up to two hosts from the available pool. These specific
+functions aren't intrinsic to the core design, and new ones can be easily added.
+
+Please read on if you'd like to learn more...
+
 ## Community:
 
 Come join us in the `mgmt` community!
@@ -30,7 +80,7 @@ approach. The project contains an engine and a language.
 
 Mgmt is a fairly new project. It is usable today, but not yet feature complete.
 With your help you'll be able to influence our design and get us to 1.0 sooner!
-Interested developers should read the [quick start guide](docs/quick-start-guide.md).
+Interested users should read the [quick start guide](docs/quick-start-guide.md).
 
 ## Documentation:
 
@@ -38,7 +88,7 @@ Please read, enjoy and help improve our documentation!
 
 | Documentation | Additional Notes |
 |---|---|
-| [quick start guide](docs/quick-start-guide.md) | for mgmt developers |
+| [quick start guide](docs/quick-start-guide.md) | for everyone |
 | [frequently asked questions](docs/faq.md) | for everyone |
 | [general documentation](docs/documentation.md) | for everyone |
 | [language guide](docs/language-guide.md) | for everyone |
@@ -57,22 +107,18 @@ If you have a well phrased question that might benefit others, consider asking
 it by sending a patch to the [FAQ](docs/faq.md) section. I'll merge your
 question, and a patch with the answer!
 
-## Roadmap:
+## Get involved:
 
 Feel free to grab one of the straightforward [#mgmtlove](https://github.com/purpleidea/mgmt/labels/mgmtlove)
 issues if you're a first time contributor to the project or if you're unsure
-about what to hack on!
-Please see: [TODO.md](TODO.md) for a list of upcoming work and TODO items.
-Please get involved by working on one of these items or by suggesting something
-else!
+about what to hack on! Please get involved by working on one of these items or
+by suggesting something else! There are some lower priority issues and harder
+issues available in our [TODO](TODO.md) file. Please have a look.
 
 ## Bugs:
 
 Please set the `DEBUG` constant in [main.go](https://github.com/purpleidea/mgmt/blob/master/main.go)
 to `true`, and post the logs when you report the [issue](https://github.com/purpleidea/mgmt/issues).
-Bonus points if you provide a [shell](https://github.com/purpleidea/mgmt/tree/master/test/shell)
-or [OMV](https://github.com/purpleidea/mgmt/tree/master/test/omv) reproducible
-test case.
 Feel free to read my article on [debugging golang programs](https://purpleidea.com/blog/2016/02/15/debugging-golang-programs/).
 
 ## Patches:

TODO.md

@@ -1,10 +1,18 @@
 # TODO
 
-If you're looking for something to do, look here!
-Let us know if you're working on one of the items.
-If you'd like something to work on, ping @purpleidea and I'll create an issue
-tailored especially for you! Just let me know your approximate golang skill
-level and how many hours you'd like to spend on the patch.
+Here is a TODO list of longstanding items that are either lower-priority, or
+more involved in terms of time, skill-level, and/or motivation.
+
+Please have a look, and let us know if you're working on one of the items. It's
+best to open an issue to track your progress and to discuss any implementation
+questions you might have.
+
+Lastly, if you'd like something different to work on, please ping @purpleidea
+and I'll create an issue tailored especially for your approximate golang skill
+level and available time commitment in terms of hours you'd need to spend on the
+patch.
+
+Happy Hacking!
 
 ## Package resource
 
@@ -19,7 +27,7 @@ level and how many hours you'd like to spend on the patch.
 
 ## Svc resource
 
-- [ ] base resource improvements
+- [ ] refreshonly support [:heart:](https://github.com/purpleidea/mgmt/issues/464)
 
 ## Exec resource
 
@@ -33,33 +41,14 @@ level and how many hours you'd like to spend on the patch.
 
 - [ ] automatic edges to file resource [:heart:](https://github.com/purpleidea/mgmt/labels/mgmtlove)
 
-## Virt (libvirt) resource
-
-- [ ] base resource improvements [:heart:](https://github.com/purpleidea/mgmt/labels/mgmtlove)
-
-## Net (systemd-networkd) resource
-
-- [ ] base resource
-
-## Nspawn (systemd-nspawn) resource
-
-- [ ] base resource [:heart:](https://github.com/purpleidea/mgmt/labels/mgmtlove)
-
-## Mount (systemd-mount) resource
-
-- [ ] base resource [:heart:](https://github.com/purpleidea/mgmt/labels/mgmtlove)
-
-## Cron (systemd-timer) resource
-
-- [ ] base resource [:heart:](https://github.com/purpleidea/mgmt/labels/mgmtlove)
-
 ## Http resource
 
 - [ ] base resource [:heart:](https://github.com/purpleidea/mgmt/labels/mgmtlove)
 
 ## Etcd improvements
 
-- [ ] fix embedded etcd master race
+- [ ] fix etcd race bug that only happens during CI testing (intermittently
+failing test case issue)
 
 ## Torrent/dht file transfer
 
@@ -69,17 +58,33 @@ level and how many hours you'd like to spend on the patch.
 
 - [ ] base plumbing
 
+## Resource improvements
+
+- [ ] more reversible resources implemented
+- [ ] more "cloud" resources
+
 ## Language improvements
 
 - [ ] more core functions
 - [ ] automatic language formatter, ala `gofmt`
 - [ ] gedit/gnome-builder/gtksourceview syntax highlighting
 - [ ] vim syntax highlighting
-- [x] emacs syntax highlighting: see `misc/emacs/`
+- [ ] emacs syntax highlighting: see `misc/emacs/` (needs updating)
+- [ ] exposed $error variable for feedback in the language
+- [ ] improve the printf function to add %[]s, %[]f ([]str, []float) and map,
+struct, nested etc... %v would be nice too!
+- [ ] add line/col/file annotations to AST so we can get locations of errors
+that the parser finds
+- [ ] add more error messages with the `%error` pattern in parser.y
+- [ ] we should have helper functions or language sugar to pull a field out of a
+struct, or a value out of a map, or an index out of a list, etc...
+
+## Engine improvements
+
+- [ ] add a "waiting for func" message in the func engine to notify the user
+about slow functions...
 
 ## Other
 
-- [ ] better error/retry handling
-- [ ] deb package target in Makefile
 - [ ] reproducible builds
 - [ ] add your suggestions!

bindata/Makefile

@@ -1,5 +1,5 @@
 # Mgmt
-# Copyright (C) 2013-2018+ James Shubin and the project contributors
+# Copyright (C) 2013-2019+ James Shubin and the project contributors
 # Written by James Shubin <james@shubin.ca> and the project contributors
 #
 # This program is free software: you can redistribute it and/or modify

converger/converger.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,8 +25,7 @@ import (
 	"time"
 
 	"github.com/purpleidea/mgmt/util"
-
-	multierr "github.com/hashicorp/go-multierror"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // New builds a new converger coordinator.
@@ -323,9 +322,8 @@ func (obj *Coordinator) runStateFns(converged bool) error {
 	for _, name := range keys { // run in deterministic order
 		fn := obj.stateFns[name]
 		// call an arbitrary function
-		if e := fn(converged); e != nil {
-			err = multierr.Append(err, e) // list of errors
-		}
+		e := fn(converged)
+		err = errwrap.Append(err, e) // list of errors
 	}
 	return err
 }

converger/converger_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

debian/copyright

@@ -3,7 +3,7 @@ Upstream-Name: mgmt
 Source: <https://github.com/purpleidea/mgmt>
 
 Files: *
-Copyright: Copyright (C) 2013-2018+ James Shubin and the project contributors
+Copyright: Copyright (C) 2013-2019+ James Shubin and the project contributors
 License: GPL-3.0
 
 License: GPL-3.0

doc.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

docs/conf.py

@@ -51,7 +51,7 @@ master_doc = 'index'
 
 # General information about the project.
 project = u'mgmt'
-copyright = u'2013-2018+ James Shubin and the project contributors'
+copyright = u'2013-2019+ James Shubin and the project contributors'
 author = u'James Shubin'
 
 # The version info for the project you're documenting, acts as replacement for

docs/development.md

@@ -3,7 +3,119 @@
 This document contains some additional information and help regarding
 developing `mgmt`. Useful tools, conventions, etc.
 
-Be sure to read [quick start guide](docs/quick-start-guide.md) first.
+Be sure to read [quick start guide](quick-start-guide.md) first.
+
+## Vagrant
+
+If you would like to avoid doing the above steps manually, we have prepared a
+[Vagrant](https://www.vagrantup.com/) environment for your convenience. From the
+project directory, run a `vagrant up`, and then a `vagrant status`. From there,
+you can `vagrant ssh` into the `mgmt` machine. The `MOTD` will explain the rest.
+This environment isn't commonly used by the `mgmt` developers, so it might not
+be working properly.
+
+## Using Docker
+
+Alternatively, you can check out the [docker-guide](docker-guide.md) in order to
+develop or deploy using docker. This method is not endorsed or supported, so use
+at your own risk, as it might not be working properly.
+
+## Information about dependencies
+
+Software projects have a few different kinds of dependencies. There are _build_
+dependencies, _runtime_ dependencies, and additionally, a few extra dependencies
+required for running the _test_ suite.
+
+### Build
+
+* `golang` 1.11 or higher (required, available in some distros and distributed
+as a binary officially by [golang.org](https://golang.org/dl/))
+
+### Runtime
+
+A relatively modern GNU/Linux system should be able to run `mgmt` without any
+problems. Since `mgmt` runs as a single statically compiled binary, all of the
+library dependencies are included. It is expected, that certain advanced
+resources require host specific facilities to work. These requirements are
+listed below:
+
+| Resource | Dependency        | Version                     | Check version with                                        |
+|----------|-------------------|-----------------------------|-----------------------------------------------------------|
+| augeas   | augeas-devel      | `augeas 1.6` or greater     | `dnf info augeas-devel` or `apt-cache show libaugeas-dev` |
+| file     | inotify           | `Linux 2.6.27` or greater   | `uname -a`                                                |
+| hostname | systemd-hostnamed | `systemd 25` or greater     | `systemctl --version`                                     |
+| nspawn   | systemd-nspawn    | `systemd ???` or greater    | `systemctl --version`                                     |
+| pkg      | packagekitd       | `packagekit 1.x` or greater | `pkcon --version`                                         |
+| svc      | systemd           | `systemd ???` or greater    | `systemctl --version`                                     |
+| virt     | libvirt-devel     | `libvirt 1.2.0` or greater  | `dnf info libvirt-devel` or `apt-cache show libvirt-dev`  |
+| virt     | libvirtd          | `libvirt 1.2.0` or greater  | `libvirtd --version`                                      |
+
+For building a visual representation of the graph, `graphviz` is required.
+
+To build `mgmt` without augeas support please run:
+`GOTAGS='noaugeas' make build`
+
+To build `mgmt` without libvirt support please run:
+`GOTAGS='novirt' make build`
+
+To build `mgmt` without docker support please run:
+`GOTAGS='nodocker' make build`
+
+To build `mgmt` without augeas, libvirt or docker support please run:
+`GOTAGS='noaugeas novirt nodocker' make build`
+
+## OSX/macOS/Darwin development
+
+Developing and running `mgmt` on macOS is currently not supported (but not
+discouraged either). Meaning it might work but in the case it doesn't you would
+have to provide your own patches to fix problems (the project maintainer and
+community are glad to assist where needed).
+
+There are currently some issues that make `mgmt` less suitable to run for
+provisioning macOS. But as a client to provision remote servers it should run
+fine.
+
+Since the primary supported systems are Linux and these are the environments
+tested, it is wise to run these suites during macOS development as well. To ease
+this, Docker can be leveraged ([Docker for Mac](https://docs.docker.com/docker-for-mac/)).
+
+Before running any of the commands below create the development Docker image:
+
+```
+docker/scripts/build-development
+```
+
+This image requires updating every time dependencies (`make-deps.sh`) changes.
+
+Then to run the test suite:
+
+```
+docker run --rm -ti \
+	-v $PWD:/go/src/github.com/purpleidea/mgmt/ \
+	-w /go/src/github.com/purpleidea/mgmt/ \
+	purpleidea/mgmt:development \
+	make test
+```
+
+For convenience this command is wrapped in `docker/scripts/exec-development`.
+
+Basically any command can be executed this way. Because the repository source is
+mounted into the Docker container invocation will be quick and allow rapid
+testing, for example:
+
+```
+docker/scripts/exec-development test/test-shell.sh load0.sh
+```
+
+Other examples:
+
+```
+docker/scripts/exec-development make build
+docker/scripts/exec-development ./mgmt run --tmp-prefix lang examples/lang/load0.mcl
+```
+
+Be advised that this method is not supported and it might not be working
+properly.
 
 ## Testing
 
@@ -45,5 +157,6 @@ individual tests to run.
 
 ### IDE/Editor support
 
-- Emacs: see `misc/emacs/`
-- [Textmate](https://github.com/aequitas/mgmt.tmbundle)
+* Emacs: see `misc/emacs/`
+* [Textmate](https://github.com/aequitas/mgmt.tmbundle)
+* [VSCode](https://github.com/aequitas/mgmt.vscode)

docs/documentation.md

@@ -147,7 +147,7 @@ Invoke `mgmt` with the `--puppet` switch, which supports 3 variants:
 
 	`mgmt run puppet --puppet 'file { "/etc/ntp.conf": ensure => file }'`
 
-For more details and caveats see [Puppet.md](Puppet.md).
+For more details and caveats see [puppet-guide.md](puppet-guide.md).
 
 #### Blog post
 
@@ -250,6 +250,43 @@ integer, then that value is the max size for that semaphore. Valid semaphore
 id's include: `some_id`, `hello:42`, `not:smart:4` and `:13`. It is expected
 that the last bare example be only used by the engine to add a global semaphore.
 
+#### Rewatch
+
+Boolean. Rewatch specifies whether we re-run the Watch worker during a graph
+swap if it has errored. When doing a graph compare to swap the graphs, if this
+is true, and this particular worker has errored, then we'll remove it and add it
+back as a new vertex, thus causing it to run again. This is different from the
+`Retry` metaparam which applies during the normal execution. It is only when
+this is exhausted that we're in permanent worker failure, and only then can we
+rely on this metaparam.
+
+#### Realize
+
+Boolean. Realize ensures that the resource is guaranteed to converge at least
+once before a potential graph swap removes or changes it. This guarantee is
+useful for fast changing graphs, to ensure that the brief creation of a resource
+is seen. This guarantee does not prevent against the engine quitting normally,
+and it can't guarantee it if the resource is blocked because of a failed
+pre-requisite resource.
+*XXX: This is currently not implemented!*
+
+#### Reverse
+
+Boolean. Reverse is a property that some resources can implement that specifies
+that some "reverse" operation should happen when that resource "disappears". A
+disappearance happens when a resource is defined in one instance of the graph,
+and is gone in the subsequent one. This disappearance can happen if it was
+previously in an if statement that then becomes false.
+
+This is helpful for building robust programs with the engine. The engine adds a
+"reversed" resource to that subsequent graph to accomplish the desired "reverse"
+mechanics. The specifics of what this entails is a property of the particular
+resource that is being "reversed".
+
+It might be wise to combine the use of this meta parameter with the use of the
+`realize` meta parameter to ensure that your reversed resource actually runs at
+least once, if there's a chance that it might be gone for a while.
+
 ### Lang metadata file
 
 Any module *must* have a metadata file in its root. It must be named
@@ -298,10 +335,6 @@ recommended that you use this, since it's preferable to write code in the
 The main interface to the `mgmt` tool is the command line. For the most recent
 documentation, please run `mgmt --help`.
 
-#### `--yaml <graph.yaml>`
-
-Point to a graph file to run.
-
 #### `--converged-timeout <seconds>`
 
 Exit if the machine has converged for approximately this many seconds.
@@ -455,7 +488,7 @@ To report any bugs, please file a ticket at: [https://github.com/purpleidea/mgmt
 
 ## Authors
 
-Copyright (C) 2013-2018+ James Shubin and the project contributors
+Copyright (C) 2013-2019+ James Shubin and the project contributors
 
 Please see the
 [AUTHORS](https://github.com/purpleidea/mgmt/tree/master/AUTHORS) file

docs/faq.md

@@ -9,6 +9,18 @@ I wanted a next generation config management solution that didn't have all of
 the design flaws or limitations that the current generation of tools do, and no
 tool existed!
 
+### Why did you choose `golang` for the project?
+
+When I started working on the project, I needed to choose a language that
+already had an implementation of a distributed consensus algorithm available.
+That meant [Paxos](https://en.wikipedia.org/wiki/Paxos_(computer_science)) or
+[Raft](https://en.wikipedia.org/wiki/Raft_(computer_science)). Golang was one
+language that actually had two different Raft implementations, `etcd`, and
+`consul`. Other design requirements included something that was reasonably fast,
+typed and memory-safe, and suited for systems engineering. After a reasonably
+extensive search, I chose `golang`. I think it was the right decision. There are
+a number of other features of the language which helped influence the decision.
+
 ### How do I contribute to the project if I don't know `golang`?
 
 There are many different ways you can contribute to the project. They can be
@@ -125,6 +137,58 @@ The downside to this approach is that you won't benefit from the automatic
 elastic nature of the embedded etcd servers, and that you're responsible if you
 accidentally break your etcd cluster, or if you use an unsupported version.
 
+### In `mgmt` you talk about events. What is this referring to?
+
+Mgmt has two main concepts that involve "events":
+1. Events in the [resource primitive](resource-guide.md).
+2. Events in the [reactive language](language-guide.md).
+
+Each resource primitive in mgmt can test (check) and set (apply) the desired
+state that was requested of it. This is familiar to what is common with existing
+tools such as `Puppet`, `Ansible`, `Chef`, `Terraform`, etc... In addition,
+`mgmt` can also **watch** the state and detect changes. As a result, it never
+has to waste time and cpu resources by polling to test and set state, leading to
+a design which is algorithmically much faster than the existing generation of
+tools.
+
+To describe the set of resources to apply, mgmt describes this collection with a
+language. In order to model the time component of infrastructure, we use a
+special kind of language called an [FRP](https://en.wikipedia.org/wiki/Functional_reactive_programming).
+This language has a built-in concept that we call "events", and which means that
+we re-evaluate the relevant portions of the code whenever a value or function
+has an event that tells us that it changed. The `R` in `FRP` stands for
+reactive. This is similar to how a spreadsheet updates dependent cells when a
+pre-requisite value is modified. [This article](https://en.wikipedia.org/wiki/Reactive_programming)
+provides a bit more background.
+
+Whenever any of the streams of values in the language change, the program is
+partially re-evaluated. The output of any mgmt program is a [DAG](https://en.wikipedia.org/wiki/Directed_acyclic_graph)
+of resources, or more precisely, a stream of resource graphs. Since we have
+events per-resource, we can efficiently switch from one desired-state resource
+graph to the next without re-checking their individual states, since we've been
+monitoring them all along.
+
+One side-effect of all this, is that if a rogue systems administrator manually
+changes the state of any managed resource, mgmt will detect this and attempt to
+revert the change. This makes for excellent live demos, but is not the primary
+design goal. It is a consequence of tracking state so that graph changes are
+efficient. We implement the event detection via an intentional per-resource
+[main loop](https://en.wikipedia.org/wiki/Event_loop) which can enable other
+interesting functionality too!
+
+Make sure to get rid of your rogue sysadmin! ;)
+
+### Do I need to run `mgmt` as `root`?
+
+No and yes. It depends. Nothing in mgmt explicitly requires root in the design,
+however mgmt will require root only if the changes to your system that you want
+it to make require root.
+
+For example, if you use it to manage files that require root access to modify,
+then you'll need root. If you only use it to manage files and resources
+elsewhere, then it shouldn't need root. Many resources are perfectly usable
+without root, and virtually all of my live demos are done without root.
+
 ### How can I run `mgmt` on-demand, or in `cron`, instead of continuously?
 
 By default, `mgmt` will run continuously in an attempt to keep your machine in a
@@ -148,43 +212,80 @@ requires a number of seconds as an argument.
 #### Example:
 
 ```
-./mgmt run lang --lang examples/lang/hello0.mcl --converged-timeout=5
+./mgmt run lang examples/lang/hello0.mcl --converged-timeout=5
 ```
 
-### What does the error message about an inconsistent dataDir mean?
+### When I try to build `mgmt` I see: `no Go files in $GOPATH/src/github.com/purpleidea/mgmt/bindata`.
+
+Due to the arcane way that `golang` designed its `$GOPATH`, the main project
+directory must be inside your `$GOPATH`, and at the appropriate FQDN. This is:
+`$GOPATH/src/github.com/purpleidea/mgmt/`. If you have your project root outside
+of that directory, then you may get this error when you try to build it. In this
+case there is likely a `go get` version of the project at this location. Remove
+it and replace it with your git cloned directory. In my case, I like to work on
+things in `~/code/mgmt/`, so that path is a symlink that points to the long
+project directory.
+
+### Why does my file resource error with `no such file or directory`?
+
+If you create a file resource and only specify the content like this:
+
+```
+file "/tmp/foo" {
+	content => "hello world\n",
+}
+```
+
+Then this will attempt to set the contents of that file to the desired string,
+but *only* if that file already exists. If you'd like to ensure that it also
+gets created in case it is not present, then you must also specify the state:
+
+```
+file "/tmp/foo" {
+	state => "exists",
+	content => "hello world\n",
+}
+```
+
+Similar logic applies for situations when you only specify the `mode` parameter.
+
+This all turns out to be more safe and "correct", in that it would error and
+prevent masking an error for a situation when you expected a file to already be
+at that location. It also turns out to simplify the internals significantly, and
+remove an ambiguous scenario with the reversable file resource.
+
+### On startup `mgmt` hangs after: `etcd: server: starting...`.
 
 If you get an error message similar to:
 
 ```
-Etcd: Connect: CtxError...
-Etcd: CtxError: Reason: CtxDelayErr(5s): No endpoints available yet!
-Etcd: Connect: Endpoints: []
-Etcd: The dataDir (/var/lib/mgmt/etcd) might be inconsistent or corrupt.
+etcd: server: starting...
+etcd: server: start timeout of 1m0s reached
+etcd: server: close timeout of 15s reached
 ```
 
-This happens when there are a series of fatal connect errors in a row. This can
-happen when you start `mgmt` using a dataDir that doesn't correspond to the
-current cluster view. As a result, the embedded etcd server never finishes
-starting up, and as a result, a default endpoint never gets added. The solution
-is to either reconcile the mistake, and if there is no important data saved, you
-can remove the etcd dataDir. This is typically `/var/lib/mgmt/etcd/member/`.
+But nothing happens afterwards, this can be due to a corrupt etcd storage
+directory. Each etcd server embedded in mgmt must have a special directory where
+it stores local state. It must not be shared by more than one individual member.
+This dir is typically `/var/lib/mgmt/etcd/member/`. If you accidentally use it
+(for example during testing) with a different cluster view, then you can corrupt
+it. This can happen if you use it with more than one different hostname.
 
-### Why do resources have both a `Cmp` method and an `IFF` (on the UID) method?
+The solution is to avoid making this mistake, and if there is no important data
+saved, you can remove the etcd member dir and start over.
 
-The `Cmp()` methods are for determining if two resources are effectively the
-same, which is used to make graph change delta's efficient. This is when we want
-to change from the current running graph to a new graph, but preserve the common
-vertices. Since we want to make this process efficient, we only update the parts
-that are different, and leave everything else alone. This `Cmp()` method can
-tell us if two resources are the same. In case it is not obvious, `cmp` is an
-abbrev. for compare.
+### On running `make` to build a new version, it errors with: `Text file busy`.
 
-The `IFF()` method is part of the whole UID system, which is for discerning if a
-resource meets the requirements another expects for an automatic edge. This is
-because the automatic edge system assumes a unified UID pattern to test for
-equality. In the future it might be helpful or sane to merge the two similar
-comparison functions although for now they are separate because they are
-actually answer different questions.
+If you get an error like:
+
+```
+cp: cannot create regular file 'mgmt': Text file busy
+```
+
+This can happen if you ran `make build` (or just `make`) when there was already
+an instance of mgmt running, or if a related file locking issue occurred. To
+solve this, shutdown and running mgmt process, run `rm mgmt` to remove the file,
+and then get a new one by running `make` again.
 
 ### Does this support Windows? OSX? GNU Hurd?
 
@@ -193,7 +294,7 @@ serious automation workloads. Support for non-Linux operating systems isn't a
 high priority of mine, but we're happy to accept patches for missing features
 or resources that you think would make sense on your favourite platform.
 
-### Why aren't you using `glide` or `godep` for dependency management?
+### Why aren't you using `glide`, `godep` or `go mod` for dependency management?
 
 Vendoring dependencies means that as the git master branch of each dependency
 marches on, you're left behind using an old version. As a result, bug fixes and
@@ -262,9 +363,8 @@ which definitely existed before the band did.
 ### You didn't answer my question, or I have a question!
 
 It's best to ask on [IRC](https://webchat.freenode.net/?channels=#mgmtconfig)
-to see if someone can help you. Once we get a big enough community going, we'll
-add a mailing list. If you don't get any response from the above, you can
-contact me through my [technical blog](https://purpleidea.com/contact/)
-and I'll do my best to help. If you have a good question, please add it as a
-patch to this documentation. I'll merge your question, and add a patch with the
-answer!
+to see if someone can help you. If you don't get a response from IRC, you can
+contact me through my [technical blog](https://purpleidea.com/contact/) and I'll
+do my best to help. If you have a good question, please add it as a patch to
+this documentation. I'll merge your question, and add a patch with the answer!
+For news and updates, subscribe to the [mailing list](https://www.redhat.com/mailman/listinfo/mgmtconfig-list).

docs/function-guide.md

@@ -219,7 +219,7 @@ Init(init *interfaces.Init) error
 
 This is called to initialize the function. If something goes wrong, it should
 return an error. It is passed a struct that contains all the important
-information and poiinters that it might need to work with throughout its
+information and pointers that it might need to work with throughout its
 lifetime. As a result, it will need to save a copy to that pointer for future
 use in the other methods.
 
@@ -377,9 +377,9 @@ might be different ways you would want to call `printf`, such as:
 `printf("the %s is %d", "answer", 42)` or `printf("3 * 2 = %d", 3 * 2)`. Since
 you couldn't implement the infinite number of possible signatures, this API lets
 you write code which can be coerced into different forms. This makes
-implementing what would appear to be generic or polymorphic, instead something
-that is actually static and that still has the static type safety properties
-that were guaranteed by the mgmt language.
+implementing what would appear to be generic or polymorphic, instead of
+something that is actually static and that still has the static type safety
+properties that were guaranteed by the mgmt language.
 
 Since this is an advanced topic, it is not described in full at this time. For
 more information please have a look at the source code comments, some of the

docs/language-guide.md

@@ -511,6 +511,9 @@ without making any changes. The `ExprVar` node naturally consumes scope's and
 the `StmtProg` node cleverly passes the scope through in the order expected for
 the out-of-order bind logic to work.
 
+This step typically calls the ordering algorithm to determine the correct order
+of statements in a program.
+
 #### Type unification
 
 Each expression must have a known type. The unpleasant option is to force the

docs/on-the-web.md

@@ -44,3 +44,11 @@ if we missed something that you think is relevant!
 | James Shubin | blog | [Mgmt Configuration Language](https://purpleidea.com/blog/2018/02/05/mgmt-configuration-language/) |
 | James Shubin | video | [Recording from CfgMgmtCamp.eu 2018](https://www.youtube.com/watch?v=NxObmwZDyrI) |
 | Jonathan Gold | blog | [Go Netlink and Select](https://jonathangold.ca/blog/go-netlink-and-select/) |
+| James Shubin | video | [Recording from DevOpsDays Montreal 2018](https://www.youtube.com/watch?v=1i38c5cooHo) |
+| James Shubin | video | [Recording from FOSDEM Minimalistic Languages Devroom 2019](https://video.fosdem.org/2019/K.4.201/mgmtconfig.webm) |
+| James Shubin | video | [Recording from FOSDEM Infra Management Devroom 2019](https://video.fosdem.org/2019/UB2.252A/mgmt.webm) |
+| James Shubin | video | [Recording from FOSDEM Graph Processing Devroom 2019](https://video.fosdem.org/2019/H.1308/graph_mgmt_config.webm) |
+| James Shubin | video | [Recording from FOSDEM Virtualization Devroom 2019](https://video.fosdem.org/2019/H.2213/vai_real_time_virtualization_automation.webm) |
+| James Shubin | video | [Recording from FOSDEM Containers Devroom 2019](https://video.fosdem.org/2019/UA2.114/containers_mgmt.webm) |
+| James Shubin | video | [Recording from FOSDEM Monitoring Devroom 2019](https://video.fosdem.org/2019/UB2.252A/real_time_merging_of_config_management_and_monitoring.webm) |
+| James Shubin | blog | [Mgmt Configuration Language: Class and Include](https://purpleidea.com/blog/2019/07/26/class-and-include-in-mgmt/) |

docs/puppet-guide.md

@@ -173,7 +173,7 @@ useful when you are in the process of replacing Puppet with mgmt. You
 can translate your custom modules into mgmt's language one by one,
 and let mgmt run the current mix.
 
-Instead of the usual `--puppet`, `--puppet-conf`, and `--lang` for mcl,
+Instead of the usual `--puppet-conf` flag and argv for `puppet` and `mcl` input,
 you need to use alternative flags to make this work:
 
 * `--lp-lang` to specify the mcl input

docs/quick-start-guide.md

@@ -2,65 +2,108 @@
 
 ## Introduction
 
-This guide is intended for developers. Once `mgmt` is minimally viable, we'll
-publish a quick start guide for users too. If you're brand new to `mgmt`, it's
-probably a good idea to start by reading the
-[introductory article](https://purpleidea.com/blog/2016/01/18/next-generation-configuration-mgmt/)
-or to watch an [introductory video](https://www.youtube.com/watch?v=LkEtBVLfygE&html5=1).
-Once you're familiar with the general idea, please start hacking...
+This guide is intended for users and developers. If you're brand new to `mgmt`,
+it's probably a good idea to start by reading an
+[introductory article about the engine](https://purpleidea.com/blog/2016/01/18/next-generation-configuration-mgmt/)
+and an [introductory article about the language](https://purpleidea.com/blog/2018/02/05/mgmt-configuration-language/).
+[There are other articles and videos available](on-the-web.md) if you'd like to
+learn more or prefer different formats. Once you're familiar with the general
+idea, or if you prefer a hands-on approach, please start hacking...
 
-## Quick start
+## Getting mgmt
 
-### Installing golang
+You can either build `mgmt` from source, or you can download a pre-built
+release. There are also some distro repositories available, but they may not be
+up to date. A pre-built release is the fastest option if there's one that's
+available for your platform. If you are developing or testing a new patch to
+`mgmt`, or there is not a release available for your platform, then you'll have
+to build your own.
 
-* You need golang version 1.10 or greater installed.
+### Downloading a pre-built release:
+
+The latest releases can be found [here](https://github.com/purpleidea/mgmt/releases/).
+An alternate mirror is available [here](https://dl.fedoraproject.org/pub/alt/purpleidea/mgmt/releases/).
+
+Make sure to verify the signatures of all packages before you use them. The
+signing key can be downloaded from [https://purpleidea.com/contact/#pgp-key](https://purpleidea.com/contact/#pgp-key)
+to verify the release.
+
+If you've decided to install a pre-build release, you can skip to the
+[Running mgmt](#running-mgmt) section below!
+
+### Building a release:
+
+You'll need some dependencies, including `golang`, and some associated tools.
+
+#### Installing golang
+
+* You need golang version 1.11 or greater installed.
 	* To install on rpm style systems: `sudo dnf install golang`
 	* To install on apt style systems: `sudo apt install golang`
 	* To install on macOS systems install [Homebrew](https://brew.sh)
 	and run: `brew install go`
 * You can run `go version` to check the golang version.
-* If your distro is tool old, you may need to [download](https://golang.org/dl/)
+* If your distro is too old, you may need to [download](https://golang.org/dl/)
 a newer golang version.
 
-### Setting up golang
+#### Setting up golang
 
-* If you do not have a GOPATH yet, create one and export it:
+* You can skip this step, as your installation will default to using `~/go/`,
+but if you do not have a `GOPATH` yet and want one in a custom location, create
+one and export it:
 
-```
+```shell
 mkdir $HOME/gopath
 export GOPATH=$HOME/gopath
 ```
 
 * You might also want to add the GOPATH to your `~/.bashrc` or `~/.profile`.
-* For more information you can read the [GOPATH documentation](https://golang.org/cmd/go/#hdr-GOPATH_environment_variable).
+* For more information you can read the
+[GOPATH documentation](https://golang.org/cmd/go/#hdr-GOPATH_environment_variable).
 
-### Getting the mgmt code and dependencies
+#### Getting the mgmt code and associated dependencies
 
-* Download the `mgmt` code into the GOPATH, and switch to that directory:
+* Download the `mgmt` code into the `GOPATH`, and switch to that directory:
 
-```
-mkdir -p $GOPATH/src/github.com/purpleidea/
-cd $GOPATH/src/github.com/purpleidea/
+```shell
+[ -z "$GOPATH" ] && mkdir ~/go/ || mkdir -p $GOPATH/src/github.com/purpleidea/
+cd $GOPATH/src/github.com/purpleidea/ || cd ~/go/
 git clone --recursive https://github.com/purpleidea/mgmt/
-cd $GOPATH/src/github.com/purpleidea/mgmt
+cd $GOPATH/src/github.com/purpleidea/mgmt/ || cd ~/go/src/github.com/purpleidea/mgmt/
 ```
 
-* Add $GOPATH/bin to $PATH
+* Add `$GOPATH/bin` to `$PATH`
 
-```
+```shell
 export PATH=$PATH:$GOPATH/bin
 ```
 
 * Run `make deps` to install system and golang dependencies. Take a look at
-`misc/make-deps.sh` for details.
-* Run `make build` to get a freshly built `mgmt` binary.
+`misc/make-deps.sh` if you want to see the details of what it does.
 
-### Running mgmt
+#### Building mgmt
 
-* Run `time ./mgmt run --tmp-prefix lang --lang examples/lang/hello0.mcl` to try
-out a very simple example!
+* Now run `make` to get a freshly built `mgmt` binary. If this succeeds, you can
+proceed to the [Running mgmt](#running-mgmt) section below!
+
+### Installing a distro release
+
+Installation of `mgmt` from distribution packages currently needs improvement.
+They are not always up-to-date with git master and as such are not recommended.
+At the moment we have:
+* [COPR](https://copr.fedoraproject.org/coprs/purpleidea/mgmt/) (currently dead)
+* [Arch](https://aur.archlinux.org/packages/mgmt/) (currently stale)
+
+Please contribute more and help improve these! We'd especially like to see a
+Debian package!
+
+## Running mgmt
+
+* Run `mgmt run --tmp-prefix lang examples/lang/hello0.mcl` to try out a very
+simple example! If you built it from source, you'll need to use `./mgmt` from
+the project directory.
 * Look in that example file that you ran to see if you can figure out what it
-did!
+did! You can press `^C` to exit `mgmt`.
 * Have fun hacking on our future technology and get involved to shape the
 project!
 
@@ -68,118 +111,3 @@ project!
 
 Please look in the [examples/lang/](../examples/lang/) folder for some more
 examples!
-
-## Vagrant
-
-If you would like to avoid doing the above steps manually, we have prepared a
-[Vagrant](https://www.vagrantup.com/) environment for your convenience. From the
-project directory, run a `vagrant up`, and then a `vagrant status`. From there,
-you can `vagrant ssh` into the `mgmt` machine. The MOTD will explain the rest.
-
-## Using Docker
-
-Alternatively, you can check out the [docker-guide](docs/docker-guide.md) in
-order to develop or deploy using docker.
-
-## Information about dependencies
-
-Software projects have a few different kinds of dependencies. There are _build_
-dependencies, _runtime_ dependencies, and additionally, a few extra dependencies
-required for running the _test_ suite.
-
-### Build
-
-* `golang` 1.10 or higher (required, available in some distros and distributed
-as a binary officially by [golang.org](https://golang.org/dl/))
-
-### Runtime
-
-A relatively modern GNU/Linux system should be able to run `mgmt` without any
-problems. Since `mgmt` runs as a single statically compiled binary, all of the
-library dependencies are included. It is expected, that certain advanced
-resources require host specific facilities to work. These requirements are
-listed below:
-
-| Resource | Dependency        | Version                     | Check version with                                        |
-|----------|-------------------|-----------------------------|-----------------------------------------------------------|
-| augeas   | augeas-devel      | `augeas 1.6` or greater     | `dnf info augeas-devel` or `apt-cache show libaugeas-dev` |
-| file     | inotify           | `Linux 2.6.27` or greater   | `uname -a`                                                |
-| hostname | systemd-hostnamed | `systemd 25` or greater     | `systemctl --version`                                     |
-| nspawn   | systemd-nspawn    | `systemd ???` or greater    | `systemctl --version`                                     |
-| pkg      | packagekitd       | `packagekit 1.x` or greater | `pkcon --version`                                         |
-| svc      | systemd           | `systemd ???` or greater    | `systemctl --version`                                     |
-| virt     | libvirt-devel     | `libvirt 1.2.0` or greater  | `dnf info libvirt-devel` or `apt-cache show libvirt-dev`  |
-| virt     | libvirtd          | `libvirt 1.2.0` or greater  | `libvirtd --version`                                      |
-
-For building a visual representation of the graph, `graphviz` is required.
-
-To build `mgmt` without augeas support please run:
-`GOTAGS='noaugeas' make build`
-
-To build `mgmt` without libvirt support please run:
-`GOTAGS='novirt' make build`
-
-To build `mgmt` without docker support please run:
-`GOTAGS='nodocker' make build`
-
-To build `mgmt` without augeas, libvirt or docker support please run:
-`GOTAGS='noaugeas novirt nodocker' make build`
-
-## Binary Package Installation
-
-Installation of `mgmt` from distribution packages currently needs improvement.
-They are not always up-to-date with git master and as such are not recommended.
-At the moment we have:
-* [COPR](https://copr.fedoraproject.org/coprs/purpleidea/mgmt/)
-* [Arch](https://aur.archlinux.org/packages/mgmt/)
-
-Please contribute more! We'd especially like to see a Debian package!
-
-## OSX/macOS/Darwin development
-
-Developing and running `mgmt` on macOS is currently not supported (but not
-discouraged either). Meaning it might work but in the case it doesn't you would
-have to provide your own patches to fix problems (the project maintainer and
-community are glad to assist where needed).
-
-There are currently some issues that make `mgmt` less suitable to run for provisioning
-macOS. But as a client to provision remote servers it should run fine.
-
-Since the primary supported systems are Linux and these are the environments
-tested for it is wise to run these suites during macOS development as well. To
-ease this Docker can be leveraged ([Docker for Mac](https://docs.docker.com/docker-for-mac/)).
-
-Before running any of the commands below create the development Docker image:
-
-```
-docker/scripts/build-development
-```
-
-This image requires updating every time dependencies (`make-deps.sh`) change.
-
-Then to run the test suite:
-
-```
-docker run --rm -ti \
-	-v $PWD:/go/src/github.com/purpleidea/mgmt/ \
-	-w /go/src/github.com/purpleidea/mgmt/ \
-	purpleidea/mgmt:development \
-	make test
-```
-
-For convenience this command is wrapped in `docker/scripts/exec-development`.
-
-Basically any command can be executed this way. Because the repository source is
-mounted into the Docker container invocation will be quick and allow rapid
-testing, example:
-
-```
-docker/scripts/exec-development test/test-shell.sh load0.sh
-```
-
-Other examples:
-
-```
-docker/scripts/exec-development make build
-docker/scripts/exec-development ./mgmt run --tmp-prefix lang --lang examples/lang/load0.mcl
-```

docs/resource-guide.md

@@ -96,8 +96,10 @@ Default() engine.Res
 ```
 
 This returns a populated resource struct as a `Res`. It shouldn't populate any
-values which already have the correct default as the golang zero value. In
+values which already get a good default as the respective golang zero value. In
 general it is preferable if the zero values make for the correct defaults.
+(This is to say, resources are designed to behave safely and intuitively
+when parameters take a zero value, whenever this is possible.)
 
 #### Example
 
@@ -754,6 +756,23 @@ Feel free to use this pattern if you're convinced it's necessary. Alternatively,
 if you think I got the `Res` API wrong and you have an improvement, please let
 us know!
 
+### Why do resources have both a `Cmp` method and an `IFF` (on the UID) method?
+
+The `Cmp()` methods are for determining if two resources are effectively the
+same, which is used to make graph change delta's efficient. This is when we want
+to change from the current running graph to a new graph, but preserve the common
+vertices. Since we want to make this process efficient, we only update the parts
+that are different, and leave everything else alone. This `Cmp()` method can
+tell us if two resources are the same. In case it is not obvious, `cmp` is an
+abbrev. for compare.
+
+The `IFF()` method is part of the whole UID system, which is for discerning if a
+resource meets the requirements another expects for an automatic edge. This is
+because the automatic edge system assumes a unified UID pattern to test for
+equality. In the future it might be helpful or sane to merge the two similar
+comparison functions although for now they are separate because they are
+actually answer different questions.
+
 ### What new resource primitives need writing?
 
 There are still many ideas for new resources that haven't been written yet. If

docs/resources.md

@@ -69,8 +69,8 @@ identified by a trailing slash in their path name. File have no such slash.
 It has the following properties:
 
 * `path`: absolute file path (directories have a trailing slash here)
+* `state`: either `exists`, `absent`, or undefined
 * `content`: raw file content
-* `state`: either `exists` (the default value) or `absent`
 * `mode`: octal unix file permissions
 * `owner`: username or uid for the file owner
 * `group`: group name or gid for the file group
@@ -79,6 +79,16 @@ It has the following properties:
 
 The path property specifies the file or directory that we are managing.
 
+### State
+
+The state property describes the action we'd like to apply for the resource. The
+possible values are: `exists` and `absent`. If you do not specify either of
+these, it is undefined. Without specifying this value as `exists`, another param
+cannot cause a file to get implicitly created. When specifying this value as
+`absent`, you should not specify any other params that would normally change the
+file. For example, if you specify `content` and this param is `absent`, then you
+will get an engine validation error.
+
 ### Content
 
 The content property is a string that specifies the desired file contents.
@@ -88,11 +98,6 @@ The content property is a string that specifies the desired file contents.
 The source property points to a source file or directory path that we wish to
 copy over and use as the desired contents for our resource.
 
-### State
-
-The state property describes the action we'd like to apply for the resource. The
-possible values are: `exists` and `absent`.
-
 ### Recurse
 
 The recurse property limits whether file resource operations should recurse into

engine/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/autoedge_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/cmp.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -152,6 +152,18 @@ func ResCmp(r1, r2 Res) error {
 		}
 	}
 
+	// compare meta params for resources with reversible traits
+	r1v, ok1 := r1.(ReversibleRes)
+	r2v, ok2 := r2.(ReversibleRes)
+	if ok1 != ok2 {
+		return fmt.Errorf("reversible differs") // they must be different (optional)
+	}
+	if ok1 && ok2 {
+		if r1v.ReversibleMeta().Cmp(r2v.ReversibleMeta()) != nil {
+			return fmt.Errorf("reversible differs")
+		}
+	}
+
 	return nil
 }
 
@@ -280,6 +292,18 @@ func AdaptCmp(r1, r2 CompatibleRes) error {
 		}
 	}
 
+	// compare meta params for resources with reversible traits
+	r1v, ok1 := r1.(ReversibleRes)
+	r2v, ok2 := r2.(ReversibleRes)
+	if ok1 != ok2 {
+		return fmt.Errorf("reversible differs") // they must be different (optional)
+	}
+	if ok1 && ok2 {
+		if r1v.ReversibleMeta().Cmp(r2v.ReversibleMeta()) != nil {
+			return fmt.Errorf("reversible differs")
+		}
+	}
+
 	return nil
 }
 

engine/copy.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -20,7 +20,7 @@ package engine
 import (
 	"fmt"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // ResCopy copies a resource. This is the main entry point for copying a
@@ -106,6 +106,16 @@ func ResCopy(r CopyableRes) (CopyableRes, error) {
 		}
 	}
 
+	// copy meta params for resources with reversible traits
+	if x, ok := r.(ReversibleRes); ok {
+		dst, ok := res.(ReversibleRes)
+		if !ok {
+			// programming error
+			panic("reversible interfaces are illogical")
+		}
+		dst.SetReversibleMeta(x.ReversibleMeta()) // no need to copy atm
+	}
+
 	return res, nil
 }
 

engine/edge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/error.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/fs.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/graph/actions.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,9 +25,8 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 	"golang.org/x/time/rate"
 )
 
@@ -377,8 +376,8 @@ Loop:
 			// we then save so we can return it to the caller of us.
 			if err != nil {
 				failed = true
-				close(obj.state[vertex].watchDone)    // causes doneChan to close
-				reterr = multierr.Append(reterr, err) // permanent failure
+				close(obj.state[vertex].watchDone)   // causes doneChan to close
+				reterr = errwrap.Append(reterr, err) // permanent failure
 				continue
 			}
 			if obj.Debug {
@@ -458,8 +457,8 @@ Loop:
 					}
 					if e != nil {
 						failed = true
-						close(obj.state[vertex].limitDone)  // causes doneChan to close
-						reterr = multierr.Append(reterr, e) // permanent failure
+						close(obj.state[vertex].limitDone) // causes doneChan to close
+						reterr = errwrap.Append(reterr, e) // permanent failure
 						break LimitWait
 					}
 					if obj.Debug {
@@ -498,8 +497,8 @@ Loop:
 						}
 						if e != nil {
 							failed = true
-							close(obj.state[vertex].limitDone)  // causes doneChan to close
-							reterr = multierr.Append(reterr, e) // permanent failure
+							close(obj.state[vertex].limitDone) // causes doneChan to close
+							reterr = errwrap.Append(reterr, e) // permanent failure
 							break RetryWait
 						}
 						if obj.Debug {
@@ -546,8 +545,8 @@ Loop:
 			// this dies. If Process fails permanently, we ask it
 			// to exit right here... (It happens when we loop...)
 			failed = true
-			close(obj.state[vertex].processDone)  // causes doneChan to close
-			reterr = multierr.Append(reterr, err) // permanent failure
+			close(obj.state[vertex].processDone) // causes doneChan to close
+			reterr = errwrap.Append(reterr, err) // permanent failure
 			continue
 
 		} // retry loop

engine/graph/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/graph/autoedge/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -22,9 +22,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
-
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // AutoEdge adds the automatic edges to the graph.
@@ -49,7 +47,7 @@ func AutoEdge(graph *pgraph.Graph, debug bool, logf func(format string, v ...int
 	for _, res := range sorted { // for each vertexes autoedges
 		autoEdgeObj, e := res.AutoEdges()
 		if e != nil {
-			err = multierr.Append(err, e) // collect all errors
+			err = errwrap.Append(err, e) // collect all errors
 			continue
 		}
 		if autoEdgeObj == nil {
@@ -91,6 +89,9 @@ func AutoEdge(graph *pgraph.Graph, debug bool, logf func(format string, v ...int
 			}
 		}
 	}
+
+	// It would be great to ensure we didn't add any loops here, but instead
+	// of checking now, we'll move the check into the main loop.
 	return nil
 }
 

engine/graph/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -24,8 +24,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/graph/autogroup"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // AutoGroup runs the auto grouping on the loaded graph.

engine/graph/autogroup/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -22,8 +22,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // AutoGroup is the mechanical auto group "runner" that runs the interface spec.
@@ -67,5 +66,8 @@ func AutoGroup(ag engine.AutoGrouper, g *pgraph.Graph, debug bool, logf func(for
 		}
 	}
 
+	// It would be great to ensure we didn't add any loops here, but instead
+	// of checking now, we'll move the check into the main loop.
+
 	return nil
 }

engine/graph/autogroup/autogroup_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -31,8 +31,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {

engine/graph/autogroup/base.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/graph/autogroup/nonreachability.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -19,8 +19,7 @@ package autogroup
 
 import (
 	"github.com/purpleidea/mgmt/pgraph"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // NonReachabilityGrouper is the most straight-forward algorithm for grouping.

engine/graph/autogroup/util.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -18,9 +18,11 @@
 package autogroup
 
 import (
-	"github.com/purpleidea/mgmt/pgraph"
+	"fmt"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/engine"
+	"github.com/purpleidea/mgmt/pgraph"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // VertexMerge merges v2 into v1 by reattaching the edges where appropriate,
@@ -113,8 +115,17 @@ func VertexMerge(g *pgraph.Graph, v1, v2 pgraph.Vertex, vertexMergeFn func(pgrap
 			// note: This branch isn't used if the vertexMergeFn
 			// decides to just merge logically on its own instead
 			// of actually returning something that we then merge.
-			v1 = v // TODO: ineffassign?
+			v1 = v // XXX: ineffassign?
 			//*v1 = *v
+
+			// Ensure that everything still validates. (For safety!)
+			r, ok := v1.(engine.Res) // TODO: v ?
+			if !ok {
+				return fmt.Errorf("not a Res")
+			}
+			if err := engine.Validate(r); err != nil {
+				return errwrap.Wrapf(err, "the Res did not Validate")
+			}
 		}
 	}
 	g.DeleteVertex(v2) // remove grouped vertex

engine/graph/engine.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,11 +25,16 @@ import (
 
 	"github.com/purpleidea/mgmt/converger"
 	"github.com/purpleidea/mgmt/engine"
+	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/pgraph"
+	"github.com/purpleidea/mgmt/util/errwrap"
 	"github.com/purpleidea/mgmt/util/semaphore"
+)
 
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
+const (
+	// StateDir is the name of the sub directory where all the local
+	// resource state is stored.
+	StateDir = "state"
 )
 
 // Engine encapsulates a generic graph and manages its operations.
@@ -50,6 +55,7 @@ type Engine struct {
 	nextGraph *pgraph.Graph
 	state     map[pgraph.Vertex]*State
 	waits     map[pgraph.Vertex]*sync.WaitGroup // wg for the Worker func
+	wlock     *sync.Mutex                       // lock around waits map
 
 	slock *sync.Mutex // semaphore lock
 	semas map[string]*semaphore.Semaphore
@@ -64,6 +70,13 @@ type Engine struct {
 // If the struct does not validate, or it cannot initialize, then this errors.
 // Initially it will contain an empty graph.
 func (obj *Engine) Init() error {
+	if obj.Program == "" {
+		return fmt.Errorf("the Program is empty")
+	}
+	if obj.Hostname == "" {
+		return fmt.Errorf("the Hostname is empty")
+	}
+
 	var err error
 	if obj.graph, err = pgraph.NewGraph("graph"); err != nil {
 		return err
@@ -78,6 +91,7 @@ func (obj *Engine) Init() error {
 
 	obj.state = make(map[pgraph.Vertex]*State)
 	obj.waits = make(map[pgraph.Vertex]*sync.WaitGroup)
+	obj.wlock = &sync.Mutex{}
 
 	obj.slock = &sync.Mutex{}
 	obj.semas = make(map[string]*semaphore.Semaphore)
@@ -167,9 +181,9 @@ func (obj *Engine) Commit() error {
 			return errwrap.Wrapf(err, "the Res did not Validate")
 		}
 
-		// FIXME: is res.Name() sufficiently unique to use as a UID here?
-		pathUID := fmt.Sprintf("%s-%s", res.Kind(), res.Name())
-		statePrefix := fmt.Sprintf("%s/", path.Join(obj.Prefix, "state", pathUID))
+		pathUID := engineUtil.ResPathUID(res)
+		statePrefix := fmt.Sprintf("%s/", path.Join(obj.statePrefix(), pathUID))
+
 		// don't create this unless it *will* be used
 		//if err := os.MkdirAll(statePrefix, 0770); err != nil {
 		//	return errwrap.Wrapf(err, "can't create state prefix")
@@ -199,10 +213,19 @@ func (obj *Engine) Commit() error {
 		fn := func() error {
 			// start the Worker
 			obj.wg.Add(1)
+			obj.wlock.Lock()
 			obj.waits[vertex].Add(1)
+			obj.wlock.Unlock()
 			go func(v pgraph.Vertex) {
 				defer obj.wg.Done()
-				defer obj.waits[v].Done()
+				defer func() {
+					// we need this lock, because this go
+					// routine could run when the next fn
+					// function above here is running...
+					obj.wlock.Lock()
+					obj.waits[v].Done()
+					obj.wlock.Unlock()
+				}()
 
 				obj.Logf("Worker(%s)", v)
 				// contains the Watch and CheckApply loops
@@ -380,20 +403,16 @@ func (obj *Engine) Pause(fastPause bool) error {
 
 // Close triggers a shutdown. Engine must be already paused before this is run.
 func (obj *Engine) Close() error {
-	var reterr error
-
-	emptyGraph, err := pgraph.NewGraph("empty")
-	if err != nil {
-		reterr = multierr.Append(reterr, err) // list of errors
-	}
+	emptyGraph, reterr := pgraph.NewGraph("empty")
 
 	// this is a graph switch (graph sync) that switches to an empty graph!
 	if err := obj.Load(emptyGraph); err != nil { // copy in empty graph
-		reterr = multierr.Append(reterr, err)
+		reterr = errwrap.Append(reterr, err)
 	}
+	// FIXME: Do we want to run commit if Load failed? Does this even work?
 	// the commit will cause the graph sync to shut things down cleverly...
 	if err := obj.Commit(); err != nil {
-		reterr = multierr.Append(reterr, err)
+		reterr = errwrap.Append(reterr, err)
 	}
 
 	obj.wg.Wait() // for now, this doesn't need to be a separate Wait() method
@@ -404,3 +423,8 @@ func (obj *Engine) Close() error {
 func (obj *Engine) Graph() *pgraph.Graph {
 	return obj.graph
 }
+
+// statePrefix returns the dir where all the resource state is stored locally.
+func (obj *Engine) statePrefix() string {
+	return fmt.Sprintf("%s/", path.Join(obj.Prefix, StateDir))
+}

engine/graph/graph_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -23,13 +23,13 @@ import (
 	"fmt"
 	"testing"
 
-	multierr "github.com/hashicorp/go-multierror"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func TestMultiErr(t *testing.T) {
 	var err error
 	e := fmt.Errorf("some error")
-	err = multierr.Append(err, e) // build an error from a nil base
+	err = errwrap.Append(err, e) // build an error from a nil base
 	// ensure that this lib allows us to append to a nil
 	if err == nil {
 		t.Errorf("missing error")

engine/graph/refresh.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/graph/reverse.go

@@ -0,0 +1,295 @@
+// Mgmt
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package graph
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path"
+	"sort"
+
+	"github.com/purpleidea/mgmt/engine"
+	engineUtil "github.com/purpleidea/mgmt/engine/util"
+	"github.com/purpleidea/mgmt/pgraph"
+	"github.com/purpleidea/mgmt/util/errwrap"
+)
+
+const (
+	// ReverseFile is the file name in the resource state dir where any
+	// reversal information is stored.
+	ReverseFile = "reverse"
+
+	// ReversePerm is the permissions mode used to create the ReverseFile.
+	ReversePerm = 0600
+)
+
+// Reversals adds the reversals onto the loaded graph. This should happen last,
+// and before Commit.
+func (obj *Engine) Reversals() error {
+	if obj.nextGraph == nil {
+		return fmt.Errorf("there is no active graph to add reversals to")
+	}
+
+	// Initially get all of the reversals to seek out all possible errors.
+	// XXX: The engine needs to know where data might have been stored if we
+	// XXX: want to potentially allow alternate read/write paths, like etcd.
+	// XXX: In this scenario, we'd have to store a token somewhere to let us
+	// XXX: know to look elsewhere for the special ReversalList read method.
+	data, err := obj.ReversalList() // (map[string]string, error)
+	if err != nil {
+		return errwrap.Wrapf(err, "the reversals had errors")
+	}
+
+	if len(data) == 0 {
+		return nil // end early
+	}
+
+	resMatch := func(r1, r2 engine.Res) bool { // simple match on UID only!
+		if r1.Kind() != r2.Kind() {
+			return false
+		}
+		if r1.Name() != r2.Name() {
+			return false
+		}
+		return true
+	}
+	resInList := func(needle engine.Res, haystack []engine.Res) bool {
+		for _, res := range haystack {
+			if resMatch(needle, res) {
+				return true
+			}
+		}
+		return false
+	}
+
+	if obj.Debug {
+		obj.Logf("decoding %d reversals...", len(data))
+	}
+	resources := []engine.Res{}
+
+	// do this in a sorted order so that it errors deterministically
+	sorted := []string{}
+	for key := range data {
+		sorted = append(sorted, key)
+	}
+	sort.Strings(sorted)
+	for _, key := range sorted {
+		val := data[key]
+		// XXX: replace this ResToB64 method with one that stores it in
+		// a human readable format, in case someone wants to hack and
+		// edit it manually.
+		// XXX: we probably want this to be YAML, it works with the diff
+		// too...
+		r, err := engineUtil.B64ToRes(val)
+		if err != nil {
+			return errwrap.Wrapf(err, "error decoding res with UID: `%s`", key)
+		}
+
+		res, ok := r.(engine.ReversibleRes)
+		if !ok {
+			// this requirement is here to keep things simpler...
+			return errwrap.Wrapf(err, "decoded res with UID: `%s` was not reversible", key)
+		}
+
+		matchFn := func(vertex pgraph.Vertex) (bool, error) {
+			r, ok := vertex.(engine.Res)
+			if !ok {
+				return false, fmt.Errorf("not a Res")
+			}
+			if !resMatch(r, res) {
+				return false, nil
+			}
+			return true, nil
+		}
+
+		// FIXME: not efficient, we could build a cache-map first
+		vertex, err := obj.nextGraph.VertexMatchFn(matchFn) // (Vertex, error)
+		if err != nil {
+			return errwrap.Wrapf(err, "error searching graph for match")
+		}
+		if vertex != nil { // found one!
+			continue // it doesn't need reversing yet
+		}
+
+		// TODO: check for (incompatible?) duplicates instead
+		if resInList(res, resources) { // we've already got this one...
+			continue
+		}
+
+		// We set this in two different places to be safe. It ensures
+		// that we erase the reversal state file after we've used it.
+		res.ReversibleMeta().Reversal = true // set this for later...
+
+		resources = append(resources, res)
+	}
+
+	if len(resources) == 0 {
+		return nil // end early
+	}
+
+	// Now that we've passed the chance of any errors, we modify the graph.
+	obj.Logf("adding %d reversals...", len(resources))
+	for _, res := range resources {
+		obj.nextGraph.AddVertex(res)
+	}
+	// TODO: Do we want a way for stored reversals to add edges too?
+
+	// It would be great to ensure we didn't add any loops here, but instead
+	// of checking now, we'll move the check into the main loop.
+	return nil
+}
+
+// ReversalList returns all the available pending reversal data on this host. It
+// can then be decoded by whatever method is appropriate for.
+func (obj *Engine) ReversalList() (map[string]string, error) {
+	result := make(map[string]string) // some key to contents
+
+	dir := obj.statePrefix() // loop through this dir...
+	files, err := ioutil.ReadDir(dir)
+	if err != nil && !os.IsNotExist(err) {
+		return nil, errwrap.Wrapf(err, "error reading list of state dirs")
+	} else if err != nil {
+		return result, nil // nothing found, no state dir exists yet
+	}
+
+	for _, x := range files {
+		key := x.Name() // some uid for the resource
+		file := path.Join(dir, key, ReverseFile)
+		content, err := ioutil.ReadFile(file)
+		if err != nil && !os.IsNotExist(err) {
+			return nil, errwrap.Wrapf(err, "could not read reverse file: %s", file)
+		} else if err != nil {
+			continue // file does not exist, skip
+		}
+
+		// file exists!
+		str := string(content)
+		result[key] = str // save
+	}
+
+	return result, nil
+}
+
+// ReversalInit performs the reversal initialization steps if necessary for this
+// resource.
+func (obj *State) ReversalInit() error {
+	res, ok := obj.Vertex.(engine.ReversibleRes)
+	if !ok {
+		return nil // nothing to do
+	}
+
+	if res.ReversibleMeta().Disabled {
+		return nil // nothing to do, reversal isn't enabled
+	}
+
+	// If the reversal is enabled, but we are the result of a previous
+	// reversal, then this will overwrite that older reversal request, and
+	// our resource should be designed to deal with that. This happens if we
+	// return a reversible resource as the reverse of a resource that was
+	// reversed. It's probably fairly rare.
+	if res.ReversibleMeta().Reversal {
+		obj.Logf("triangle reversal") // warn!
+	}
+
+	r, err := res.Reversed()
+	if err != nil {
+		return errwrap.Wrapf(err, "could not reverse: %s", res.String())
+	}
+	if r == nil {
+		return nil // this can't be reversed, or isn't implemented here
+	}
+
+	// We set this in two different places to be safe. It ensures that we
+	// erase the reversal state file after we've used it.
+	r.ReversibleMeta().Reversal = true // set this for later...
+
+	// XXX: replace this ResToB64 method with one that stores it in a human
+	// readable format, in case someone wants to hack and edit it manually.
+	// XXX: we probably want this to be YAML, it works with the diff too...
+	str, err := engineUtil.ResToB64(r)
+	if err != nil {
+		return errwrap.Wrapf(err, "could not encode: %s", res.String())
+	}
+
+	// TODO: put this method on traits.Reversible as part of the interface?
+	return obj.ReversalWrite(str, res.ReversibleMeta().Overwrite) // Store!
+}
+
+// ReversalClose performs the reversal shutdown steps if necessary for this
+// resource.
+func (obj *State) ReversalClose() error {
+	res, ok := obj.Vertex.(engine.ReversibleRes)
+	if !ok {
+		return nil // nothing to do
+	}
+
+	// Don't check res.ReversibleMeta().Disabled because we're removing the
+	// previous one. That value only applies if we're doing a new reversal.
+
+	if !res.ReversibleMeta().Reversal {
+		return nil // nothing to erase, we're not a reversal resource
+	}
+
+	if !obj.isStateOK { // did we successfully reverse?
+		obj.Logf("did not complete reversal") // warn
+		return nil
+	}
+
+	// TODO: put this method on traits.Reversible as part of the interface?
+	return obj.ReversalDelete() // Erase our reversal instructions.
+}
+
+// ReversalWrite stores the reversal state information for this resource.
+func (obj *State) ReversalWrite(str string, overwrite bool) error {
+	dir, err := obj.varDir("") // private version
+	if err != nil {
+		return errwrap.Wrapf(err, "could not get VarDir for reverse")
+	}
+	file := path.Join(dir, ReverseFile) // return a unique file
+
+	content, err := ioutil.ReadFile(file)
+	if err != nil && !os.IsNotExist(err) {
+		return errwrap.Wrapf(err, "could not read reverse file: %s", file)
+	}
+
+	// file exists and we shouldn't overwrite if different
+	if err == nil && !overwrite {
+		// compare to existing file
+		oldStr := string(content)
+		if str != oldStr {
+			obj.Logf("existing, pending, reversible resource exists")
+			//obj.Logf("diff:")
+			//obj.Logf("") // TODO: print the diff w/o and secret values
+			return fmt.Errorf("existing, pending, reversible resource exists")
+		}
+	}
+
+	return ioutil.WriteFile(file, []byte(str), ReversePerm)
+}
+
+// ReversalDelete removes the reversal state information for this resource.
+func (obj *State) ReversalDelete() error {
+	dir, err := obj.varDir("") // private version
+	if err != nil {
+		return errwrap.Wrapf(err, "could not get VarDir for reverse")
+	}
+	file := path.Join(dir, ReverseFile) // return a unique file
+
+	return errwrap.Wrapf(os.Remove(file), "could not remove reverse state file")
+}

engine/graph/semaphore.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -23,9 +23,8 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/purpleidea/mgmt/util/errwrap"
 	"github.com/purpleidea/mgmt/util/semaphore"
-
-	multierr "github.com/hashicorp/go-multierror"
 )
 
 // SemaSep is the trailing separator to split the semaphore id from the size.
@@ -46,9 +45,8 @@ func (obj *Engine) semaLock(semas []string) error {
 		}
 		obj.slock.Unlock()
 
-		if err := sema.P(1); err != nil { // lock!
-			reterr = multierr.Append(reterr, err) // list of errors
-		}
+		err := sema.P(1)                     // lock!
+		reterr = errwrap.Append(reterr, err) // list of errors
 	}
 	return reterr
 }
@@ -65,9 +63,8 @@ func (obj *Engine) semaUnlock(semas []string) error {
 			panic(fmt.Sprintf("graph: sema: %s does not exist", id))
 		}
 
-		if err := sema.V(1); err != nil { // unlock!
-			reterr = multierr.Append(reterr, err) // list of errors
-		}
+		err := sema.V(1)                     // unlock!
+		reterr = errwrap.Append(reterr, err) // list of errors
 	}
 	return reterr
 }

engine/graph/semaphore_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/graph/sendrecv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -22,9 +22,8 @@ import (
 	"reflect"
 
 	"github.com/purpleidea/mgmt/engine"
-
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
+	engineUtil "github.com/purpleidea/mgmt/engine/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // SendRecv pulls in the sent values into the receive slots. It is called by the
@@ -47,16 +46,51 @@ func (obj *Engine) SendRecv(res engine.RecvableRes) (map[string]bool, error) {
 			st = v.Res.Sent()
 		}
 
+		if st == nil {
+			e := fmt.Errorf("received nil value from: %s", v.Res)
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
+		if e := engineUtil.StructFieldCompat(st, v.Key, res, k); e != nil {
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
 		// send
+		m1, e := engineUtil.StructTagToFieldName(st)
+		if e != nil {
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+		key1, exists := m1[v.Key]
+		if !exists {
+			e := fmt.Errorf("requested key of `%s` not found in send struct", v.Key)
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
 		obj1 := reflect.Indirect(reflect.ValueOf(st))
 		type1 := obj1.Type()
-		value1 := obj1.FieldByName(v.Key)
+		value1 := obj1.FieldByName(key1)
 		kind1 := value1.Kind()
 
 		// recv
+		m2, e := engineUtil.StructTagToFieldName(res)
+		if e != nil {
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+		key2, exists := m2[k]
+		if !exists {
+			e := fmt.Errorf("requested key of `%s` not found in recv struct", k)
+			err = errwrap.Append(err, e) // list of errors
+			continue
+		}
+
 		obj2 := reflect.Indirect(reflect.ValueOf(res)) // pass in full struct
 		type2 := obj2.Type()
-		value2 := obj2.FieldByName(k)
+		value2 := obj2.FieldByName(key2)
 		kind2 := value2.Kind()
 
 		if obj.Debug {
@@ -67,7 +101,7 @@ func (obj *Engine) SendRecv(res engine.RecvableRes) (map[string]bool, error) {
 		// i think we probably want the same kind, at least for now...
 		if kind1 != kind2 {
 			e := fmt.Errorf("kind mismatch between %s: %s and %s: %s", v.Res, kind1, res, kind2)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 
@@ -75,21 +109,21 @@ func (obj *Engine) SendRecv(res engine.RecvableRes) (map[string]bool, error) {
 		// FIXME: do we want to relax this for string -> *string ?
 		if e := TypeCmp(value1, value2); e != nil {
 			e := errwrap.Wrapf(e, "type mismatch between %s and %s", v.Res, res)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 
 		// if we can't set, then well this is pointless!
 		if !value2.CanSet() {
 			e := fmt.Errorf("can't set %s.%s", res, k)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 
 		// if we can't interface, we can't compare...
 		if !value1.CanInterface() || !value2.CanInterface() {
 			e := fmt.Errorf("can't interface %s.%s", res, k)
-			err = multierr.Append(err, e) // list of errors
+			err = errwrap.Append(err, e) // list of errors
 			continue
 		}
 

engine/graph/state.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -26,8 +26,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/pgraph"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // State stores some state about the resource it is mapped to.
@@ -204,6 +203,12 @@ func (obj *State) Init() error {
 	if obj.Debug {
 		obj.Logf("Init(%s)", res)
 	}
+
+	// write the reverse request to the disk...
+	if err := obj.ReversalInit(); err != nil {
+		return err // TODO: test this code path...
+	}
+
 	err := res.Init(obj.init)
 	if obj.Debug {
 		obj.Logf("Init(%s): Return(%+v)", res, err)
@@ -237,12 +242,23 @@ func (obj *State) Close() error {
 	if obj.Debug {
 		obj.Logf("Close(%s)", res)
 	}
-	err := res.Close()
-	if obj.Debug {
-		obj.Logf("Close(%s): Return(%+v)", res, err)
+
+	var reverr error
+	// clear the reverse request from the disk...
+	if err := obj.ReversalClose(); err != nil {
+		// TODO: test this code path...
+		// TODO: should this be an error or a warning?
+		reverr = err
 	}
 
-	return err
+	reterr := res.Close()
+	if obj.Debug {
+		obj.Logf("Close(%s): Return(%+v)", res, reterr)
+	}
+
+	reterr = errwrap.Append(reterr, reverr)
+
+	return reterr
 }
 
 // Poke sends a notification on the poke channel. This channel is used to notify

engine/graph/vardir.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -22,7 +22,7 @@ import (
 	"os"
 	"path"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // varDir returns the path to a working directory for the resource. It will try

engine/metaparams.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -22,8 +22,8 @@ import (
 	"strconv"
 
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
-	errwrap "github.com/pkg/errors"
 	"golang.org/x/time/rate"
 )
 

engine/metaparams_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/refresh.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -21,7 +21,8 @@ import (
 	"encoding/gob"
 	"fmt"
 
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
+
 	"gopkg.in/yaml.v2"
 )
 

engine/resources/augeas.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -27,8 +27,8 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
-	errwrap "github.com/pkg/errors"
 	// FIXME: we vendor go/augeas because master requires augeas 1.6.0
 	// and libaugeas-dev-1.6.0 is not yet available in a PPA.
 	"honnef.co/go/augeas"

engine/resources/aws_ec2.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -34,6 +34,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
@@ -42,8 +43,6 @@ import (
 	cwe "github.com/aws/aws-sdk-go/service/cloudwatchevents"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/sns"
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -393,17 +392,14 @@ func (obj *AwsEc2Res) Close() error {
 	// clean up sns objects created by Init/snsWatch
 	if obj.snsClient != nil {
 		// delete the topic and associated subscriptions
-		if err := obj.snsDeleteTopic(obj.snsTopicArn); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		e1 := obj.snsDeleteTopic(obj.snsTopicArn)
+		errList = errwrap.Append(errList, e1)
 		// remove the target
-		if err := obj.cweRemoveTarget(CweTargetID, CweRuleName); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		e2 := obj.cweRemoveTarget(CweTargetID, CweRuleName)
+		errList = errwrap.Append(errList, e2)
 		// delete the cloudwatch rule
-		if err := obj.cweDeleteRule(CweRuleName); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		e3 := obj.cweDeleteRule(CweRuleName)
+		errList = errwrap.Append(errList, e3)
 	}
 
 	return errList
@@ -608,7 +604,7 @@ func (obj *AwsEc2Res) snsWatch() error {
 }
 
 // CheckApply method for AwsEc2 resource.
-func (obj *AwsEc2Res) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *AwsEc2Res) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
 	// find the instance we need to check
@@ -755,45 +751,37 @@ func (obj *AwsEc2Res) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *AwsEc2Res) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *AwsEc2Res) Compare(r engine.Res) bool {
 	// we can only compare AwsEc2Res to others of the same resource kind
 	res, ok := r.(*AwsEc2Res)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if obj.Region != res.Region {
-		return false
+		return fmt.Errorf("the Region differs")
 	}
 	if obj.Type != res.Type {
-		return false
+		return fmt.Errorf("the Type differs")
 	}
 	if obj.ImageID != res.ImageID {
-		return false
+		return fmt.Errorf("the ImageID differs")
 	}
 	if obj.WatchEndpoint != res.WatchEndpoint {
-		return false
+		return fmt.Errorf("the WatchEndpoint differs")
 	}
 	if obj.WatchListenAddr != res.WatchListenAddr {
-		return false
+		return fmt.Errorf("the WatchListenAddr differs")
 	}
 	if obj.ErrorOnMalformedPost != res.ErrorOnMalformedPost {
-		return false
+		return fmt.Errorf("the ErrorOnMalformedPost differs")
 	}
 	if obj.UserData != res.UserData {
-		return false
+		return fmt.Errorf("the UserData differs")
 	}
-	return true
+	return nil
 }
 
 func (obj *AwsEc2Res) prependName() string {
@@ -1029,7 +1017,7 @@ func (obj *AwsEc2Res) snsMakeTopic() (string, error) {
 	}
 	obj.init.Logf("Created SNS Topic")
 	if topic.TopicArn == nil {
-		return "", fmt.Errorf("TopicArn is nil")
+		return "", fmt.Errorf("the TopicArn is nil")
 	}
 	return *topic.TopicArn, nil
 }

engine/resources/config_etcd.go

@@ -0,0 +1,250 @@
+// Mgmt
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
+// Written by James Shubin <james@shubin.ca> and the project contributors
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package resources
+
+import (
+	"context"
+	"fmt"
+	"sync"
+	"time"
+
+	"github.com/purpleidea/mgmt/engine"
+	"github.com/purpleidea/mgmt/engine/traits"
+	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
+)
+
+func init() {
+	engine.RegisterResource("config:etcd", func() engine.Res { return &ConfigEtcdRes{} })
+}
+
+const (
+	sizeCheckApplyTimeout = 5 * time.Second
+)
+
+// ConfigEtcdRes is a resource that sets mgmt's etcd configuration.
+type ConfigEtcdRes struct {
+	traits.Base // add the base methods without re-implementation
+
+	init *engine.Init
+
+	// IdealClusterSize is the requested minimum size of the cluster. If you
+	// set this to zero, it will cause a cluster wide shutdown if
+	// AllowSizeShutdown is true. If it's not true, then it will cause a
+	// validation error.
+	IdealClusterSize uint16 `lang:"idealclustersize"`
+	// AllowSizeShutdown is a required safety flag that you must set to true
+	// if you want to allow causing a cluster shutdown by setting
+	// IdealClusterSize to zero.
+	AllowSizeShutdown bool `lang:"allow_size_shutdown"`
+
+	// sizeFlag determines whether sizeCheckApply already ran or not.
+	sizeFlag bool
+
+	interruptChan chan struct{}
+	wg            *sync.WaitGroup
+}
+
+// Default returns some sensible defaults for this resource.
+func (obj *ConfigEtcdRes) Default() engine.Res {
+	return &ConfigEtcdRes{}
+}
+
+// Validate if the params passed in are valid data.
+func (obj *ConfigEtcdRes) Validate() error {
+	if obj.IdealClusterSize < 0 {
+		return fmt.Errorf("the IdealClusterSize param must be positive")
+	}
+
+	if obj.IdealClusterSize == 0 && !obj.AllowSizeShutdown {
+		return fmt.Errorf("the IdealClusterSize can't be zero if AllowSizeShutdown is false")
+	}
+
+	return nil
+}
+
+// Init runs some startup code for this resource.
+func (obj *ConfigEtcdRes) Init(init *engine.Init) error {
+	obj.init = init // save for later
+
+	obj.interruptChan = make(chan struct{})
+	obj.wg = &sync.WaitGroup{}
+
+	return nil
+}
+
+// Close is run by the engine to clean up after the resource is done.
+func (obj *ConfigEtcdRes) Close() error {
+	obj.wg.Wait() // bonus
+	return nil
+}
+
+// Watch is the primary listener for this resource and it outputs events.
+func (obj *ConfigEtcdRes) Watch() error {
+	obj.wg.Add(1)
+	defer obj.wg.Done()
+	// FIXME: add timeout to context
+	// The obj.init.Done channel is closed by the engine to signal shutdown.
+	ctx, cancel := util.ContextWithCloser(context.Background(), obj.init.Done)
+	defer cancel()
+	ch, err := obj.init.World.IdealClusterSizeWatch(util.CtxWithWg(ctx, obj.wg))
+	if err != nil {
+		return errwrap.Wrapf(err, "could not watch ideal cluster size")
+	}
+
+	obj.init.Running() // when started, notify engine that we're running
+
+Loop:
+	for {
+		select {
+		case event, ok := <-ch:
+			if !ok {
+				break Loop
+			}
+			if obj.init.Debug {
+				obj.init.Logf("event: %+v", event)
+			}
+			// pass through and send an event
+
+		case <-obj.init.Done: // closed by the engine to signal shutdown
+		}
+
+		obj.init.Event() // notify engine of an event (this can block)
+	}
+
+	return nil
+}
+
+// sizeCheckApply sets the IdealClusterSize parameter. If it sees a value change
+// to zero, then it *won't* try and change it away from zero, because it assumes
+// that someone has requested a shutdown. If the value is seen on first startup,
+// then it will change it, because it might be a zero from the previous cluster.
+func (obj *ConfigEtcdRes) sizeCheckApply(apply bool) (bool, error) {
+	wg := &sync.WaitGroup{}
+	defer wg.Wait() // this must be above the defer cancel() call
+	ctx, cancel := context.WithTimeout(context.Background(), sizeCheckApplyTimeout)
+	defer cancel()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-obj.interruptChan:
+			cancel()
+		case <-ctx.Done():
+			// let this exit
+		}
+	}()
+
+	val, err := obj.init.World.IdealClusterSizeGet(ctx)
+	if err != nil {
+		return false, errwrap.Wrapf(err, "could not get ideal cluster size")
+	}
+
+	// if we got a value of zero, and we've already run before, then it's ok
+	if obj.IdealClusterSize != 0 && val == 0 && obj.sizeFlag {
+		obj.init.Logf("impending cluster shutdown, not setting ideal cluster size")
+		return true, nil // impending shutdown, don't try and cancel it.
+	}
+	obj.sizeFlag = true
+
+	// must be done after setting the above flag
+	if obj.IdealClusterSize == val { // state is correct
+		return true, nil
+	}
+
+	if !apply {
+		return false, nil
+	}
+
+	// set!
+	// This is run as a transaction so we detect if we needed to change it.
+	changed, err := obj.init.World.IdealClusterSizeSet(ctx, obj.IdealClusterSize)
+	if err != nil {
+		return false, errwrap.Wrapf(err, "could not set ideal cluster size")
+	}
+	if !changed {
+		return true, nil // we lost a race, which means no change needed
+	}
+	obj.init.Logf("set dynamic cluster size to: %d", obj.IdealClusterSize)
+
+	return false, nil
+}
+
+// CheckApply method for Noop resource. Does nothing, returns happy!
+func (obj *ConfigEtcdRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
+
+	if c, err := obj.sizeCheckApply(apply); err != nil {
+		return false, err
+	} else if !c {
+		checkOK = false
+	}
+
+	// TODO: add more config settings management here...
+	//if c, err := obj.TODOCheckApply(apply); err != nil {
+	//	return false, err
+	//} else if !c {
+	//	checkOK = false
+	//}
+
+	return checkOK, nil // w00t
+}
+
+// Cmp compares two resources and returns an error if they are not equivalent.
+func (obj *ConfigEtcdRes) Cmp(r engine.Res) error {
+	// we can only compare ConfigEtcdRes to others of the same resource kind
+	res, ok := r.(*ConfigEtcdRes)
+	if !ok {
+		return fmt.Errorf("not a %s", obj.Kind())
+	}
+
+	if obj.IdealClusterSize != res.IdealClusterSize {
+		return fmt.Errorf("the IdealClusterSize param differs")
+	}
+	if obj.AllowSizeShutdown != res.AllowSizeShutdown {
+		return fmt.Errorf("the AllowSizeShutdown param differs")
+	}
+
+	return nil
+}
+
+// Interrupt is called to ask the execution of this resource to end early.
+func (obj *ConfigEtcdRes) Interrupt() error {
+	close(obj.interruptChan)
+	return nil
+}
+
+// UnmarshalYAML is the custom unmarshal handler for this struct.
+// It is primarily useful for setting the defaults.
+func (obj *ConfigEtcdRes) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	type rawRes ConfigEtcdRes // indirection to avoid infinite recursion
+
+	def := obj.Default()            // get the default
+	res, ok := def.(*ConfigEtcdRes) // put in the right format
+	if !ok {
+		return fmt.Errorf("could not convert to ConfigEtcdRes")
+	}
+	raw := rawRes(*res) // convert; the defaults go here
+
+	if err := unmarshal(&raw); err != nil {
+		return err
+	}
+
+	*obj = ConfigEtcdRes(raw) // restore from indirection with type conversion!
+	return nil
+}

engine/resources/cron.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -31,12 +31,12 @@ import (
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	sdbus "github.com/coreos/go-systemd/dbus"
 	"github.com/coreos/go-systemd/unit"
 	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -310,28 +310,29 @@ func (obj *CronRes) Watch() error {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *CronRes) CheckApply(apply bool) (checkOK bool, err error) {
-	ok := true
+func (obj *CronRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
 	// use the embedded file resource to apply the correct state
 	if c, err := obj.file.CheckApply(apply); err != nil {
 		return false, errwrap.Wrapf(err, "nested file failed")
 	} else if !c {
-		ok = false
+		checkOK = false
 	}
 	// check timer state and apply the defined state if needed
 	if c, err := obj.unitCheckApply(apply); err != nil {
 		return false, errwrap.Wrapf(err, "unitCheckApply error")
 	} else if !c {
-		ok = false
+		checkOK = false
 	}
-	return ok, nil
+	return checkOK, nil
 }
 
 // unitCheckApply checks the state of the systemd-timer unit and, if apply is
 // true, applies the defined state.
-func (obj *CronRes) unitCheckApply(apply bool) (checkOK bool, err error) {
+func (obj *CronRes) unitCheckApply(apply bool) (bool, error) {
 	var conn *sdbus.Conn
 	var godbusConn *dbus.Conn
+	var err error
 
 	// this resource depends on systemd to ensure that it's running
 	if !systemdUtil.IsRunningSystemd() {

engine/resources/docker_container.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -30,13 +30,13 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/client"
 	"github.com/docker/go-connections/nat"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -201,7 +201,7 @@ func (obj *DockerContainerRes) Watch() error {
 }
 
 // CheckApply method for Docker resource.
-func (obj *DockerContainerRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *DockerContainerRes) CheckApply(apply bool) (bool, error) {
 	var id string
 	var destroy bool
 

engine/resources/docker_container_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/exec.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -27,13 +27,12 @@ import (
 	"strings"
 	"sync"
 	"syscall"
+	"time"
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
-	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -44,26 +43,60 @@ func init() {
 type ExecRes struct {
 	traits.Base // add the base methods without re-implementation
 	traits.Edgeable
+	traits.Sendable
 
 	init *engine.Init
 
-	Cmd        string  `yaml:"cmd"`        // the command to run
-	Cwd        string  `yaml:"cwd"`        // the dir to run the command in (empty means use `pwd` of command)
-	Shell      string  `yaml:"shell"`      // the (optional) shell to use to run the cmd
-	Timeout    int     `yaml:"timeout"`    // the cmd timeout in seconds
-	WatchCmd   string  `yaml:"watchcmd"`   // the watch command to run
-	WatchCwd   string  `yaml:"watchcwd"`   // the dir to run the watch command in (empty means use `pwd` of command)
-	WatchShell string  `yaml:"watchshell"` // the (optional) shell to use to run the watch cmd
-	IfCmd      string  `yaml:"ifcmd"`      // the if command to run
-	IfCwd      string  `yaml:"ifcwd"`      // the dir to run the if command in (empty means use `pwd` of command)
-	IfShell    string  `yaml:"ifshell"`    // the (optional) shell to use to run the if cmd
-	User       string  `yaml:"user"`       // the (optional) user to use to execute the command
-	Group      string  `yaml:"group"`      // the (optional) group to use to execute the command
-	Output     *string // all cmd output, read only, do not set!
-	Stdout     *string // the cmd stdout, read only, do not set!
-	Stderr     *string // the cmd stderr, read only, do not set!
+	// Cmd is the command to run. If this is not specified, we use the name.
+	Cmd string `yaml:"cmd"`
+	// Args is a list of args to pass to Cmd. This can be used *instead* of
+	// passing the full command and args as a single string to Cmd. It can
+	// only be used when a Shell is *not* specified. The advantage of this
+	// is that you don't have to worry about escape characters.
+	Args []string `yaml:"args"`
+	// Cmd is the dir to run the command in. If empty, then this will use
+	// the working directory of the calling process. (This process is mgmt,
+	// not the process being run here.)
+	Cwd string `yaml:"cwd"`
+	// Shell is the (optional) shell to use to run the cmd. If you specify
+	// this, then you can't use the Args parameter.
+	Shell string `yaml:"shell"`
+	// Timeout is the number of seconds to wait before sending a Kill to the
+	// running command. If the Kill is received before the process exits,
+	// then this be treated as an error.
+	Timeout uint64 `yaml:"timeout"`
 
-	wg *sync.WaitGroup
+	// Watch is the command to run to detect event changes. Each line of
+	// output from this command is treated as an event.
+	WatchCmd string `yaml:"watchcmd"`
+	// WatchCwd is the Cwd for the WatchCmd. See the docs for Cwd.
+	WatchCwd string `yaml:"watchcwd"`
+	// WatchShell is the Shell for the WatchCmd. See the docs for Shell.
+	WatchShell string `yaml:"watchshell"`
+
+	// IfCmd is the command that runs to guard against running the Cmd. If
+	// this command succeeds, then Cmd *will* be run. If this command
+	// returns a non-zero result, then the Cmd will not be run. Any error
+	// scenario or timeout will cause the resource to error.
+	IfCmd string `yaml:"ifcmd"`
+	// IfCwd is the Cwd for the IfCmd. See the docs for Cwd.
+	IfCwd string `yaml:"ifcwd"`
+	// IfShell is the Shell for the IfCmd. See the docs for Shell.
+	IfShell string `yaml:"ifshell"`
+
+	// User is the (optional) user to use to execute the command. It is used
+	// for any command being run.
+	User string `yaml:"user"`
+	// Group is the (optional) group to use to execute the command. It is
+	// used for any command being run.
+	Group string `yaml:"group"`
+
+	output *string // all cmd output, read only, do not set!
+	stdout *string // the cmd stdout, read only, do not set!
+	stderr *string // the cmd stderr, read only, do not set!
+
+	interruptChan chan struct{}
+	wg            *sync.WaitGroup
 }
 
 // Default returns some sensible defaults for this resource.
@@ -71,10 +104,27 @@ func (obj *ExecRes) Default() engine.Res {
 	return &ExecRes{}
 }
 
+// getCmd returns the actual command to run. When Cmd is not specified, we use
+// the Name.
+func (obj *ExecRes) getCmd() string {
+	if obj.Cmd != "" {
+		return obj.Cmd
+	}
+	return obj.Name()
+}
+
 // Validate if the params passed in are valid data.
 func (obj *ExecRes) Validate() error {
-	if obj.Cmd == "" { // this is the only thing that is really required
-		return fmt.Errorf("command can't be empty")
+	if obj.getCmd() == "" { // this is the only thing that is really required
+		return fmt.Errorf("the Cmd can't be empty")
+	}
+
+	split := strings.Fields(obj.getCmd())
+	if len(obj.Args) > 0 && obj.Shell != "" {
+		return fmt.Errorf("the Args param can't be used with a Shell")
+	}
+	if len(obj.Args) > 0 && len(split) > 1 {
+		return fmt.Errorf("the Args param can't be used when Cmd has args")
 	}
 
 	// check that, if an user or a group is set, we're running as root
@@ -84,7 +134,7 @@ func (obj *ExecRes) Validate() error {
 			return errwrap.Wrapf(err, "error looking up current user")
 		}
 		if currentUser.Uid != "0" {
-			return errwrap.Errorf("running as root is required if you want to use exec with a different user/group")
+			return fmt.Errorf("running as root is required if you want to use exec with a different user/group")
 		}
 	}
 
@@ -95,6 +145,7 @@ func (obj *ExecRes) Validate() error {
 func (obj *ExecRes) Init(init *engine.Init) error {
 	obj.init = init // save for later
 
+	obj.interruptChan = make(chan struct{})
 	obj.wg = &sync.WaitGroup{}
 
 	return nil
@@ -107,7 +158,7 @@ func (obj *ExecRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *ExecRes) Watch() error {
-	ioChan := make(chan *bufioOutput)
+	ioChan := make(chan *cmdOutput)
 	defer obj.wg.Wait()
 
 	if obj.WatchCmd != "" {
@@ -125,7 +176,10 @@ func (obj *ExecRes) Watch() error {
 			cmdName = obj.WatchShell // usually bash, or sh
 			cmdArgs = []string{"-c", obj.WatchCmd}
 		}
-		cmd := exec.Command(cmdName, cmdArgs...)
+
+		ctx, cancel := context.WithCancel(context.Background())
+		defer cancel()
+		cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
 		cmd.Dir = obj.WatchCwd // run program in pwd if ""
 		// ignore signals sent to parent process (we're in our own group)
 		cmd.SysProcAttr = &syscall.SysProcAttr{
@@ -139,25 +193,9 @@ func (obj *ExecRes) Watch() error {
 			return errwrap.Wrapf(err, "error while setting credential")
 		}
 
-		cmdReader, err := cmd.StdoutPipe()
-		if err != nil {
-			return errwrap.Wrapf(err, "error creating StdoutPipe for Cmd")
+		if ioChan, err = obj.cmdOutputRunner(ctx, cmd); err != nil {
+			return errwrap.Wrapf(err, "error starting WatchCmd")
 		}
-		scanner := bufio.NewScanner(cmdReader)
-
-		defer cmd.Wait() // wait for the command to exit before return!
-		defer func() {
-			// FIXME: without wrapping this in this func it panic's
-			// when running certain graphs... why?
-			cmd.Process.Kill() // shutdown the Watch command on exit
-		}()
-		if err := cmd.Start(); err != nil {
-			return errwrap.Wrapf(err, "error starting Cmd")
-		}
-
-		ctx, cancel := context.WithCancel(context.Background())
-		defer cancel() // unblock and cleanup
-		ioChan = obj.bufioChanScanner(ctx, scanner)
 	}
 
 	obj.init.Running() // when started, notify engine that we're running
@@ -172,12 +210,32 @@ func (obj *ExecRes) Watch() error {
 				return fmt.Errorf("reached EOF")
 			}
 			if err := data.err; err != nil {
-				// error reading input?
-				return errwrap.Wrapf(err, "unknown error")
+				// error reading input or cmd failure
+				exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
+				if !ok {
+					// command failed in some bad way
+					return errwrap.Wrapf(err, "unknown error")
+				}
+				pStateSys := exitErr.Sys() // (*os.ProcessState) Sys
+				wStatus, ok := pStateSys.(syscall.WaitStatus)
+				if !ok {
+					return errwrap.Wrapf(err, "error running cmd")
+				}
+				exitStatus := wStatus.ExitStatus()
+				obj.init.Logf("watchcmd exited with: %d", exitStatus)
+				if exitStatus != 0 {
+					return errwrap.Wrapf(err, "unexpected exit status of zero")
+				}
+				return err // i'm not sure if this could happen
 			}
 
 			// each time we get a line of output, we loop!
-			obj.init.Logf("watch output: %s", data.text)
+			if s := data.text; s == "" {
+				obj.init.Logf("watch output is empty!")
+			} else {
+				obj.init.Logf("watch output is:")
+				obj.init.Logf(s)
+			}
 			if data.text != "" {
 				send = true
 			}
@@ -231,11 +289,42 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 			return false, errwrap.Wrapf(err, "error while setting credential")
 		}
 
+		var out splitWriter
+		out.Init()
+		cmd.Stdout = out.Stdout
+		cmd.Stderr = out.Stderr
+
 		if err := cmd.Run(); err != nil {
-			// TODO: check exit value
+			exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
+			if !ok {
+				// command failed in some bad way
+				return false, err
+			}
+			pStateSys := exitErr.Sys() // (*os.ProcessState) Sys
+			wStatus, ok := pStateSys.(syscall.WaitStatus)
+			if !ok {
+				return false, errwrap.Wrapf(err, "error running cmd")
+			}
+			exitStatus := wStatus.ExitStatus()
+			if exitStatus == 0 {
+				return false, fmt.Errorf("unexpected exit status of zero")
+			}
+
+			obj.init.Logf("ifcmd exited with: %d", exitStatus)
+			if s := out.String(); s == "" {
+				obj.init.Logf("ifcmd output is empty!")
+			} else {
+				obj.init.Logf("ifcmd output is:")
+				obj.init.Logf(s)
+			}
 			return true, nil // don't run
 		}
-
+		if s := out.String(); s == "" {
+			obj.init.Logf("ifcmd output is empty!")
+		} else {
+			obj.init.Logf("ifcmd output is:")
+			obj.init.Logf(s)
+		}
 	}
 
 	// state is not okay, no work done, exit, but without error
@@ -251,16 +340,33 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		// call without a shell
 		// FIXME: are there still whitespace splitting issues?
 		// TODO: we could make the split character user selectable...!
-		split := strings.Fields(obj.Cmd)
+		split := strings.Fields(obj.getCmd())
 		cmdName = split[0]
 		//d, _ := os.Getwd() // TODO: how does this ever error ?
 		//cmdName = path.Join(d, cmdName)
 		cmdArgs = split[1:]
+		if len(obj.Args) > 0 {
+			if len(split) != 1 { // should not happen
+				return false, fmt.Errorf("validation error")
+			}
+			cmdArgs = obj.Args
+		}
 	} else {
 		cmdName = obj.Shell // usually bash, or sh
-		cmdArgs = []string{"-c", obj.Cmd}
+		cmdArgs = []string{"-c", obj.getCmd()}
 	}
-	cmd := exec.Command(cmdName, cmdArgs...)
+
+	wg := &sync.WaitGroup{}
+	defer wg.Wait() // this must be above the defer cancel() call
+	var ctx context.Context
+	var cancel context.CancelFunc
+	if obj.Timeout > 0 { // cmd.Process.Kill() is called on timeout
+		ctx, cancel = context.WithTimeout(context.Background(), time.Duration(obj.Timeout)*time.Second)
+	} else { // zero timeout means no timer
+		ctx, cancel = context.WithCancel(context.Background())
+	}
+	defer cancel()
+	cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
 	cmd.Dir = obj.Cwd // run program in pwd if ""
 	// ignore signals sent to parent process (we're in our own group)
 	cmd.SysProcAttr = &syscall.SysProcAttr{
@@ -285,35 +391,32 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		return false, errwrap.Wrapf(err, "error starting cmd")
 	}
 
-	timeout := obj.Timeout
-	if timeout == 0 { // zero timeout means no timer, so disable it
-		timeout = -1
-	}
-	done := make(chan error)
-	go func() { done <- cmd.Wait() }()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-obj.interruptChan:
+			cancel()
+		case <-ctx.Done():
+			// let this exit
+		}
+	}()
 
-	select {
-	case e := <-done:
-		err = e // store
-
-	case <-util.TimeAfterOrBlock(timeout):
-		cmd.Process.Kill() // TODO: check error?
-		return false, fmt.Errorf("timeout for cmd")
-	}
+	err = cmd.Wait() // we can unblock this with the timeout
 
 	// save in memory for send/recv
 	// we use pointers to strings to indicate if used or not
 	if out.Stdout.Activity || out.Stderr.Activity {
 		str := out.String()
-		obj.Output = &str
+		obj.output = &str
 	}
 	if out.Stdout.Activity {
 		str := out.Stdout.String()
-		obj.Stdout = &str
+		obj.stdout = &str
 	}
 	if out.Stderr.Activity {
 		str := out.Stderr.String()
-		obj.Stderr = &str
+		obj.stderr = &str
 	}
 
 	// process the err result from cmd, we process non-zero exits here too!
@@ -324,7 +427,18 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 		if !ok {
 			return false, errwrap.Wrapf(err, "error running cmd")
 		}
-		return false, fmt.Errorf("cmd error, exit status: %d", wStatus.ExitStatus())
+		exitStatus := wStatus.ExitStatus()
+		if !wStatus.Signaled() { // not a timeout or cancel (no signal)
+			return false, errwrap.Wrapf(err, "cmd error, exit status: %d", exitStatus)
+		}
+		sig := wStatus.Signal()
+
+		// we get this on timeout, because ctx calls cmd.Process.Kill()
+		if sig == syscall.SIGKILL {
+			return false, errwrap.Wrapf(err, "cmd timeout, exit status: %d", exitStatus)
+		}
+
+		return false, fmt.Errorf("unknown cmd error, signal: %s, exit status: %d", sig, exitStatus)
 
 	} else if err != nil {
 		return false, errwrap.Wrapf(err, "general cmd error")
@@ -334,11 +448,18 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 	// would be nice, but it would require terminal log output that doesn't
 	// interleave all the parallel parts which would mix it all up...
 	if s := out.String(); s == "" {
-		obj.init.Logf("Command output is empty!")
-
+		obj.init.Logf("command output is empty!")
 	} else {
-		obj.init.Logf("Command output is:")
-		obj.init.Logf(out.String())
+		obj.init.Logf("command output is:")
+		obj.init.Logf(s)
+	}
+
+	if err := obj.init.Send(&ExecSends{
+		Output: obj.output,
+		Stdout: obj.stdout,
+		Stderr: obj.stderr,
+	}); err != nil {
+		return false, err
 	}
 
 	// The state tracking is for exec resources that can't "detect" their
@@ -351,58 +472,67 @@ func (obj *ExecRes) CheckApply(apply bool) (bool, error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *ExecRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *ExecRes) Compare(r engine.Res) bool {
 	// we can only compare ExecRes to others of the same resource kind
 	res, ok := r.(*ExecRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.Cmd != res.Cmd {
-		return false
+		return fmt.Errorf("the Cmd differs")
+	}
+	if len(obj.Args) != len(res.Args) {
+		return fmt.Errorf("the Args differ")
+	}
+	for i, a := range obj.Args {
+		if a != res.Args[i] {
+			return fmt.Errorf("the Args differ at index: %d", i)
+		}
 	}
 	if obj.Cwd != res.Cwd {
-		return false
+		return fmt.Errorf("the Cwd differs")
 	}
 	if obj.Shell != res.Shell {
-		return false
+		return fmt.Errorf("the Shell differs")
 	}
 	if obj.Timeout != res.Timeout {
-		return false
-	}
-	if obj.WatchCmd != res.WatchCmd {
-		return false
-	}
-	if obj.WatchCwd != res.WatchCwd {
-		return false
-	}
-	if obj.WatchShell != res.WatchShell {
-		return false
-	}
-	if obj.IfCmd != res.IfCmd {
-		return false
-	}
-	if obj.IfCwd != res.IfCwd {
-		return false
-	}
-	if obj.IfShell != res.IfShell {
-		return false
-	}
-	if obj.User != res.User {
-		return false
-	}
-	if obj.Group != res.Group {
-		return false
+		return fmt.Errorf("the Timeout differs")
 	}
 
-	return true
+	if obj.WatchCmd != res.WatchCmd {
+		return fmt.Errorf("the WatchCmd differs")
+	}
+	if obj.WatchCwd != res.WatchCwd {
+		return fmt.Errorf("the WatchCwd differs")
+	}
+	if obj.WatchShell != res.WatchShell {
+		return fmt.Errorf("the WatchShell differs")
+	}
+
+	if obj.IfCmd != res.IfCmd {
+		return fmt.Errorf("the IfCmd differs")
+	}
+	if obj.IfCwd != res.IfCwd {
+		return fmt.Errorf("the IfCwd differs")
+	}
+	if obj.IfShell != res.IfShell {
+		return fmt.Errorf("the IfShell differs")
+	}
+
+	if obj.User != res.User {
+		return fmt.Errorf("the User differs")
+	}
+	if obj.Group != res.Group {
+		return fmt.Errorf("the Group differs")
+	}
+
+	return nil
+}
+
+// Interrupt is called to ask the execution of this resource to end early.
+func (obj *ExecRes) Interrupt() error {
+	close(obj.interruptChan)
+	return nil
 }
 
 // ExecUID is the UID struct for ExecRes.
@@ -453,13 +583,32 @@ func (obj *ExecRes) AutoEdges() (engine.AutoEdge, error) {
 func (obj *ExecRes) UIDs() []engine.ResUID {
 	x := &ExecUID{
 		BaseUID: engine.BaseUID{Name: obj.Name(), Kind: obj.Kind()},
-		Cmd:     obj.Cmd,
+		Cmd:     obj.getCmd(),
 		IfCmd:   obj.IfCmd,
 		// TODO: add more params here
 	}
 	return []engine.ResUID{x}
 }
 
+// ExecSends is the struct of data which is sent after a successful Apply.
+type ExecSends struct {
+	// Output is the combined stdout and stderr of the command.
+	Output *string `lang:"output"`
+	// Stdout is the stdout of the command.
+	Stdout *string `lang:"stdout"`
+	// Stderr is the stderr of the command.
+	Stderr *string `lang:"stderr"`
+}
+
+// Sends represents the default struct of values we can send using Send/Recv.
+func (obj *ExecRes) Sends() interface{} {
+	return &ExecSends{
+		Output: nil,
+		Stdout: nil,
+		Stderr: nil,
+	}
+}
+
 // UnmarshalYAML is the custom unmarshal handler for this struct.
 // It is primarily useful for setting the defaults.
 func (obj *ExecRes) UnmarshalYAML(unmarshal func(interface{}) error) error {
@@ -516,7 +665,7 @@ func (obj *ExecRes) cmdFiles() []string {
 	var paths []string
 	if obj.Shell != "" {
 		paths = append(paths, obj.Shell)
-	} else if cmdSplit := strings.Fields(obj.Cmd); len(cmdSplit) > 0 {
+	} else if cmdSplit := strings.Fields(obj.getCmd()); len(cmdSplit) > 0 {
 		paths = append(paths, cmdSplit[0])
 	}
 	if obj.WatchShell != "" {
@@ -532,36 +681,56 @@ func (obj *ExecRes) cmdFiles() []string {
 	return paths
 }
 
-// bufioOutput is the output struct of the bufioChanScanner channel output.
-type bufioOutput struct {
+// cmdOutput is the output struct of the cmdOutputRunner channel output. You
+// should always check the error first. If it's nil, then you can assume the
+// text data is good to use.
+type cmdOutput struct {
 	text string
 	err  error
 }
 
-// bufioChanScanner wraps the scanner output in a channel.
-func (obj *ExecRes) bufioChanScanner(ctx context.Context, scanner *bufio.Scanner) chan *bufioOutput {
-	ch := make(chan *bufioOutput)
+// cmdOutputRunner wraps the Cmd in with a StdoutPipe scanner and reads for
+// errors. It runs Start and Wait, and errors runtime things in the channel.
+// If it can't start up the command, it will fail early. Once it's running, it
+// will return the channel which can be used for the duration of the process.
+// Cancelling the context merely unblocks the sending on the output channel, it
+// does not Kill the cmd process. For that you must do it yourself elsewhere.
+func (obj *ExecRes) cmdOutputRunner(ctx context.Context, cmd *exec.Cmd) (chan *cmdOutput, error) {
+	cmdReader, err := cmd.StdoutPipe()
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "error creating StdoutPipe for Cmd")
+	}
+	scanner := bufio.NewScanner(cmdReader)
+	if err := cmd.Start(); err != nil {
+		return nil, errwrap.Wrapf(err, "error starting Cmd")
+	}
+
+	ch := make(chan *cmdOutput)
 	obj.wg.Add(1)
 	go func() {
 		defer obj.wg.Done()
 		defer close(ch)
 		for scanner.Scan() {
 			select {
-			case ch <- &bufioOutput{text: scanner.Text()}: // blocks here ?
+			case ch <- &cmdOutput{text: scanner.Text()}: // blocks here ?
 			case <-ctx.Done():
 				return
 			}
 		}
+
 		// on EOF, scanner.Err() will be nil
-		if err := scanner.Err(); err != nil {
+		reterr := scanner.Err()
+		reterr = errwrap.Append(reterr, cmd.Wait()) // always run Wait()
+		// send any misc errors we encounter on the channel
+		if reterr != nil {
 			select {
-			case ch <- &bufioOutput{err: err}: // send any misc errors we encounter
+			case ch <- &cmdOutput{err: reterr}:
 			case <-ctx.Done():
 				return
 			}
 		}
 	}()
-	return ch
+	return ch, nil
 }
 
 // splitWriter mimics what the ssh.CombinedOutput command does, but stores the

engine/resources/exec_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -20,20 +20,34 @@
 package resources
 
 import (
+	"context"
+	"fmt"
+	"os/exec"
+	"syscall"
 	"testing"
+	"time"
 
 	"github.com/purpleidea/mgmt/engine"
 )
 
-func fakeInit(t *testing.T) *engine.Init {
+func fakeExecInit(t *testing.T) (*engine.Init, *ExecSends) {
 	debug := testing.Verbose() // set via the -test.v flag to `go test`
 	logf := func(format string, v ...interface{}) {
 		t.Logf("test: "+format, v...)
 	}
+	execSends := &ExecSends{}
 	return &engine.Init{
+		Send: func(st interface{}) error {
+			x, ok := st.(*ExecSends)
+			if !ok {
+				return fmt.Errorf("unable to send")
+			}
+			*execSends = *x // set
+			return nil
+		},
 		Debug: debug,
 		Logf:  logf,
-	}
+	}, execSends
 }
 
 func TestExecSendRecv1(t *testing.T) {
@@ -50,7 +64,8 @@ func TestExecSendRecv1(t *testing.T) {
 			t.Errorf("close failed with: %v", err)
 		}
 	}()
-	if err := r1.Init(fakeInit(t)); err != nil {
+	init, execSends := fakeExecInit(t)
+	if err := r1.Init(init); err != nil {
 		t.Errorf("init failed with: %v", err)
 	}
 	// run artificially without the entire engine
@@ -58,23 +73,23 @@ func TestExecSendRecv1(t *testing.T) {
 		t.Errorf("checkapply failed with: %v", err)
 	}
 
-	t.Logf("output is: %v", r1.Output)
-	if r1.Output != nil {
-		t.Logf("output is: %v", *r1.Output)
+	t.Logf("output is: %v", execSends.Output)
+	if execSends.Output != nil {
+		t.Logf("output is: %v", *execSends.Output)
 	}
-	t.Logf("stdout is: %v", r1.Stdout)
-	if r1.Stdout != nil {
-		t.Logf("stdout is: %v", *r1.Stdout)
+	t.Logf("stdout is: %v", execSends.Stdout)
+	if execSends.Stdout != nil {
+		t.Logf("stdout is: %v", *execSends.Stdout)
 	}
-	t.Logf("stderr is: %v", r1.Stderr)
-	if r1.Stderr != nil {
-		t.Logf("stderr is: %v", *r1.Stderr)
+	t.Logf("stderr is: %v", execSends.Stderr)
+	if execSends.Stderr != nil {
+		t.Logf("stderr is: %v", *execSends.Stderr)
 	}
 
-	if r1.Stdout == nil {
+	if execSends.Stdout == nil {
 		t.Errorf("stdout is nil")
 	} else {
-		if out := *r1.Stdout; out != "hello world\n" {
+		if out := *execSends.Stdout; out != "hello world\n" {
 			t.Errorf("got wrong stdout(%d): %s", len(out), out)
 		}
 	}
@@ -94,7 +109,8 @@ func TestExecSendRecv2(t *testing.T) {
 			t.Errorf("close failed with: %v", err)
 		}
 	}()
-	if err := r1.Init(fakeInit(t)); err != nil {
+	init, execSends := fakeExecInit(t)
+	if err := r1.Init(init); err != nil {
 		t.Errorf("init failed with: %v", err)
 	}
 	// run artificially without the entire engine
@@ -102,23 +118,23 @@ func TestExecSendRecv2(t *testing.T) {
 		t.Errorf("checkapply failed with: %v", err)
 	}
 
-	t.Logf("output is: %v", r1.Output)
-	if r1.Output != nil {
-		t.Logf("output is: %v", *r1.Output)
+	t.Logf("output is: %v", execSends.Output)
+	if execSends.Output != nil {
+		t.Logf("output is: %v", *execSends.Output)
 	}
-	t.Logf("stdout is: %v", r1.Stdout)
-	if r1.Stdout != nil {
-		t.Logf("stdout is: %v", *r1.Stdout)
+	t.Logf("stdout is: %v", execSends.Stdout)
+	if execSends.Stdout != nil {
+		t.Logf("stdout is: %v", *execSends.Stdout)
 	}
-	t.Logf("stderr is: %v", r1.Stderr)
-	if r1.Stderr != nil {
-		t.Logf("stderr is: %v", *r1.Stderr)
+	t.Logf("stderr is: %v", execSends.Stderr)
+	if execSends.Stderr != nil {
+		t.Logf("stderr is: %v", *execSends.Stderr)
 	}
 
-	if r1.Stderr == nil {
+	if execSends.Stderr == nil {
 		t.Errorf("stderr is nil")
 	} else {
-		if out := *r1.Stderr; out != "hello world\n" {
+		if out := *execSends.Stderr; out != "hello world\n" {
 			t.Errorf("got wrong stderr(%d): %s", len(out), out)
 		}
 	}
@@ -138,7 +154,8 @@ func TestExecSendRecv3(t *testing.T) {
 			t.Errorf("close failed with: %v", err)
 		}
 	}()
-	if err := r1.Init(fakeInit(t)); err != nil {
+	init, execSends := fakeExecInit(t)
+	if err := r1.Init(init); err != nil {
 		t.Errorf("init failed with: %v", err)
 	}
 	// run artificially without the entire engine
@@ -146,42 +163,97 @@ func TestExecSendRecv3(t *testing.T) {
 		t.Errorf("checkapply failed with: %v", err)
 	}
 
-	t.Logf("output is: %v", r1.Output)
-	if r1.Output != nil {
-		t.Logf("output is: %v", *r1.Output)
+	t.Logf("output is: %v", execSends.Output)
+	if execSends.Output != nil {
+		t.Logf("output is: %v", *execSends.Output)
 	}
-	t.Logf("stdout is: %v", r1.Stdout)
-	if r1.Stdout != nil {
-		t.Logf("stdout is: %v", *r1.Stdout)
+	t.Logf("stdout is: %v", execSends.Stdout)
+	if execSends.Stdout != nil {
+		t.Logf("stdout is: %v", *execSends.Stdout)
 	}
-	t.Logf("stderr is: %v", r1.Stderr)
-	if r1.Stderr != nil {
-		t.Logf("stderr is: %v", *r1.Stderr)
+	t.Logf("stderr is: %v", execSends.Stderr)
+	if execSends.Stderr != nil {
+		t.Logf("stderr is: %v", *execSends.Stderr)
 	}
 
-	if r1.Output == nil {
+	if execSends.Output == nil {
 		t.Errorf("output is nil")
 	} else {
 		// it looks like bash or golang race to the write, so whichever
 		// order they come out in is ok, as long as they come out whole
-		if out := *r1.Output; out != "hello world\ngoodbye world\n" && out != "goodbye world\nhello world\n" {
+		if out := *execSends.Output; out != "hello world\ngoodbye world\n" && out != "goodbye world\nhello world\n" {
 			t.Errorf("got wrong output(%d): %s", len(out), out)
 		}
 	}
 
-	if r1.Stdout == nil {
+	if execSends.Stdout == nil {
 		t.Errorf("stdout is nil")
 	} else {
-		if out := *r1.Stdout; out != "hello world\n" {
+		if out := *execSends.Stdout; out != "hello world\n" {
 			t.Errorf("got wrong stdout(%d): %s", len(out), out)
 		}
 	}
 
-	if r1.Stderr == nil {
+	if execSends.Stderr == nil {
 		t.Errorf("stderr is nil")
 	} else {
-		if out := *r1.Stderr; out != "goodbye world\n" {
+		if out := *execSends.Stderr; out != "goodbye world\n" {
 			t.Errorf("got wrong stderr(%d): %s", len(out), out)
 		}
 	}
 }
+
+func TestExecTimeoutBehaviour(t *testing.T) {
+	// cmd.Process.Kill() is called on timeout
+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
+	defer cancel()
+	cmdName := "/bin/sleep"    // it's /usr/bin/sleep on modern distros
+	cmdArgs := []string{"300"} // 5 min in seconds
+	cmd := exec.CommandContext(ctx, cmdName, cmdArgs...)
+	// ignore signals sent to parent process (we're in our own group)
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Setpgid: true,
+		Pgid:    0,
+	}
+
+	if err := cmd.Start(); err != nil {
+		t.Errorf("error starting cmd: %+v", err)
+		return
+	}
+
+	err := cmd.Wait() // we can unblock this with the timeout
+
+	if err == nil {
+		t.Errorf("expected error, got nil")
+		return
+	}
+
+	exitErr, ok := err.(*exec.ExitError) // embeds an os.ProcessState
+	if err != nil && ok {
+		pStateSys := exitErr.Sys() // (*os.ProcessState) Sys
+		wStatus, ok := pStateSys.(syscall.WaitStatus)
+		if !ok {
+			t.Errorf("error running cmd")
+			return
+		}
+		if !wStatus.Signaled() {
+			t.Errorf("did not get signal, exit status: %d", wStatus.ExitStatus())
+			return
+		}
+
+		// we get this on timeout, because ctx calls cmd.Process.Kill()
+		if sig := wStatus.Signal(); sig != syscall.SIGKILL {
+			t.Errorf("got wrong signal: %+v, exit status: %d", sig, wStatus.ExitStatus())
+			return
+		}
+
+		t.Logf("exit status: %d", wStatus.ExitStatus())
+		return
+
+	} else if err != nil {
+		t.Errorf("general cmd error")
+		return
+	}
+
+	// no error
+}

engine/resources/file_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -78,7 +78,7 @@ func TestMiscEncodeDecode1(t *testing.T) {
 	e := gob.NewEncoder(&b1)
 	err = e.Encode(&input) // pass with &
 	if err != nil {
-		t.Errorf("Gob failed to Encode: %v", err)
+		t.Errorf("gob failed to Encode: %v", err)
 	}
 	str := base64.StdEncoding.EncodeToString(b1.Bytes())
 
@@ -86,27 +86,27 @@ func TestMiscEncodeDecode1(t *testing.T) {
 	var output interface{}
 	bb, err := base64.StdEncoding.DecodeString(str)
 	if err != nil {
-		t.Errorf("Base64 failed to Decode: %v", err)
+		t.Errorf("base64 failed to Decode: %v", err)
 	}
 	b2 := bytes.NewBuffer(bb)
 	d := gob.NewDecoder(b2)
 	err = d.Decode(&output) // pass with &
 	if err != nil {
-		t.Errorf("Gob failed to Decode: %v", err)
+		t.Errorf("gob failed to Decode: %v", err)
 	}
 
 	res1, ok := input.(engine.Res)
 	if !ok {
-		t.Errorf("Input %v is not a Res", res1)
+		t.Errorf("input %v is not a Res", res1)
 		return
 	}
 	res2, ok := output.(engine.Res)
 	if !ok {
-		t.Errorf("Output %v is not a Res", res2)
+		t.Errorf("output %v is not a Res", res2)
 		return
 	}
 	if err := res1.Cmp(res2); err != nil {
-		t.Errorf("The input and output Res values do not match: %+v", err)
+		t.Errorf("the input and output Res values do not match: %+v", err)
 	}
 }
 
@@ -116,34 +116,135 @@ func TestMiscEncodeDecode2(t *testing.T) {
 	// encode
 	input, err := engine.NewNamedResource("file", "file1")
 	if err != nil {
-		t.Errorf("Can't create: %v", err)
+		t.Errorf("can't create: %v", err)
 		return
 	}
+	// NOTE: Do not add this bit of code, because it would cause the path to
+	// get taken from the actual Path parameter, instead of using the name,
+	// and if we use the name, the Cmp function will detect if the name is
+	// stored properly or not.
+	//fileRes := input.(*FileRes) // must not panic
+	//fileRes.Path = "/tmp/whatever"
 
 	b64, err := engineUtil.ResToB64(input)
 	if err != nil {
-		t.Errorf("Can't encode: %v", err)
+		t.Errorf("can't encode: %v", err)
 		return
 	}
 
 	output, err := engineUtil.B64ToRes(b64)
 	if err != nil {
-		t.Errorf("Can't decode: %v", err)
+		t.Errorf("can't decode: %v", err)
 		return
 	}
 
 	res1, ok := input.(engine.Res)
 	if !ok {
-		t.Errorf("Input %v is not a Res", res1)
+		t.Errorf("input %v is not a Res", res1)
 		return
 	}
 	res2, ok := output.(engine.Res)
 	if !ok {
-		t.Errorf("Output %v is not a Res", res2)
+		t.Errorf("output %v is not a Res", res2)
 		return
 	}
+	// this uses the standalone file cmp function
 	if err := res1.Cmp(res2); err != nil {
-		t.Errorf("The input and output Res values do not match: %+v", err)
+		t.Errorf("the input and output Res values do not match: %+v", err)
+	}
+}
+
+func TestMiscEncodeDecode3(t *testing.T) {
+	var err error
+
+	// encode
+	input, err := engine.NewNamedResource("file", "file1")
+	if err != nil {
+		t.Errorf("can't create: %v", err)
+		return
+	}
+	fileRes := input.(*FileRes) // must not panic
+	fileRes.Path = "/tmp/whatever"
+	// TODO: add other params/traits/etc here!
+
+	b64, err := engineUtil.ResToB64(input)
+	if err != nil {
+		t.Errorf("can't encode: %v", err)
+		return
+	}
+
+	output, err := engineUtil.B64ToRes(b64)
+	if err != nil {
+		t.Errorf("can't decode: %v", err)
+		return
+	}
+
+	res1, ok := input.(engine.Res)
+	if !ok {
+		t.Errorf("input %v is not a Res", res1)
+		return
+	}
+	res2, ok := output.(engine.Res)
+	if !ok {
+		t.Errorf("output %v is not a Res", res2)
+		return
+	}
+	// this uses the more complete, engine cmp function
+	if err := engine.ResCmp(res1, res2); err != nil {
+		t.Errorf("the input and output Res values do not match: %+v", err)
+	}
+}
+
+func TestMiscEncodeDecode4(t *testing.T) {
+	var err error
+	const (
+		Kind = "file"
+		Name = "file1"
+	)
+
+	// encode
+	input, err := engine.NewNamedResource(Kind, Name)
+	if err != nil {
+		t.Errorf("can't create: %v", err)
+		return
+	}
+	fileRes := input.(*FileRes) // must not panic
+	fileRes.Path = "/tmp/whatever"
+	// TODO: add other params/traits/etc here!
+
+	b64, err := engineUtil.ResToB64(input)
+	if err != nil {
+		t.Errorf("can't encode: %v", err)
+		return
+	}
+
+	output, err := engineUtil.B64ToRes(b64)
+	if err != nil {
+		t.Errorf("can't decode: %v", err)
+		return
+	}
+
+	res1, ok := input.(engine.Res)
+	if !ok {
+		t.Errorf("input %v is not a Res", res1)
+		return
+	}
+	res2, ok := output.(engine.Res)
+	if !ok {
+		t.Errorf("output %v is not a Res", res2)
+		return
+	}
+	// this uses the more complete, engine cmp function
+	if err := engine.ResCmp(res1, res2); err != nil {
+		t.Errorf("the input and output Res values do not match: %+v", err)
+	}
+
+	// ensure the kind and name are correctly decoded too!
+	if kind := res2.Kind(); kind != Kind {
+		t.Errorf("the output kind was `%s`, expected `%s`", kind, Kind)
+	}
+	if name := res2.Name(); name != Name {
+		t.Errorf("the output name was `%s`, expected `%s`", name, Name)
 	}
 }
 

engine/resources/group.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -28,8 +28,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -59,7 +58,7 @@ func (obj *GroupRes) Default() engine.Res {
 // Validate if the params passed in are valid data.
 func (obj *GroupRes) Validate() error {
 	if obj.State != "exists" && obj.State != "absent" {
-		return fmt.Errorf("State must be 'exists' or 'absent'")
+		return fmt.Errorf("state must be 'exists' or 'absent'")
 	}
 	return nil
 }
@@ -119,7 +118,7 @@ func (obj *GroupRes) Watch() error {
 }
 
 // CheckApply method for Group resource.
-func (obj *GroupRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *GroupRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
 	// check if the group exists
@@ -221,32 +220,24 @@ func (obj *GroupRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *GroupRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *GroupRes) Compare(r engine.Res) bool {
 	// we can only compare GroupRes to others of the same resource kind
 	res, ok := r.(*GroupRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if (obj.GID == nil) != (res.GID == nil) {
-		return false
+		return fmt.Errorf("the GID differs")
 	}
 	if obj.GID != nil && res.GID != nil {
 		if *obj.GID != *res.GID {
-			return false
+			return fmt.Errorf("the GID differs")
 		}
 	}
-	return true
+	return nil
 }
 
 // GroupUID is the UID struct for GroupRes.

engine/resources/hostname.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,9 +25,9 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -110,7 +110,7 @@ func (obj *HostnameRes) Watch() error {
 	// if we share the bus with others, we will get each others messages!!
 	bus, err := util.SystemBusPrivateUsable() // don't share the bus connection!
 	if err != nil {
-		return errwrap.Wrap(err, "Failed to connect to bus")
+		return errwrap.Wrapf(err, "failed to connect to bus")
 	}
 	defer bus.Close()
 	// watch the PropertiesChanged signal on the hostname1 dbus path
@@ -120,7 +120,7 @@ func (obj *HostnameRes) Watch() error {
 		dbusPropertiesIface,
 	)
 	if call := bus.BusObject().Call(engineUtil.DBusAddMatch, 0, args); call.Err != nil {
-		return errwrap.Wrap(call.Err, "Failed to subscribe to DBus events for hostname1")
+		return errwrap.Wrapf(call.Err, "failed to subscribe to DBus events for hostname1")
 	}
 	defer bus.BusObject().Call(engineUtil.DBusRemoveMatch, 0, args) // ignore the error
 
@@ -147,18 +147,18 @@ func (obj *HostnameRes) Watch() error {
 	}
 }
 
-func (obj *HostnameRes) updateHostnameProperty(object dbus.BusObject, expectedValue, property, setterName string, apply bool) (checkOK bool, err error) {
+func (obj *HostnameRes) updateHostnameProperty(object dbus.BusObject, expectedValue, property, setterName string, apply bool) (bool, error) {
 	propertyObject, err := object.GetProperty("org.freedesktop.hostname1." + property)
 	if err != nil {
 		return false, errwrap.Wrapf(err, "failed to get org.freedesktop.hostname1.%s", property)
 	}
 	if propertyObject.Value() == nil {
-		return false, errwrap.Errorf("Unexpected nil value received when reading property %s", property)
+		return false, fmt.Errorf("unexpected nil value received when reading property %s", property)
 	}
 
 	propertyValue, ok := propertyObject.Value().(string)
 	if !ok {
-		return false, fmt.Errorf("Received unexpected type as %s value, expected string got '%T'", property, propertyValue)
+		return false, fmt.Errorf("received unexpected type as %s value, expected string got '%T'", property, propertyValue)
 	}
 
 	// expected value and actual value match => checkOk
@@ -182,16 +182,16 @@ func (obj *HostnameRes) updateHostnameProperty(object dbus.BusObject, expectedVa
 }
 
 // CheckApply method for Hostname resource.
-func (obj *HostnameRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *HostnameRes) CheckApply(apply bool) (bool, error) {
 	conn, err := util.SystemBusPrivateUsable()
 	if err != nil {
-		return false, errwrap.Wrap(err, "Failed to connect to the private system bus")
+		return false, errwrap.Wrapf(err, "failed to connect to the private system bus")
 	}
 	defer conn.Close()
 
 	hostnameObject := conn.Object(hostname1Iface, hostname1Path)
 
-	checkOK = true
+	checkOK := true
 	if obj.PrettyHostname != "" {
 		propertyCheckOK, err := obj.updateHostnameProperty(hostnameObject, obj.PrettyHostname, "PrettyHostname", "SetPrettyHostname", apply)
 		if err != nil {
@@ -219,31 +219,23 @@ func (obj *HostnameRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *HostnameRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *HostnameRes) Compare(r engine.Res) bool {
 	// we can only compare HostnameRes to others of the same resource kind
 	res, ok := r.(*HostnameRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.PrettyHostname != res.PrettyHostname {
-		return false
+		return fmt.Errorf("the PrettyHostname differs")
 	}
 	if obj.StaticHostname != res.StaticHostname {
-		return false
+		return fmt.Errorf("the StaticHostname differs")
 	}
 	if obj.TransientHostname != res.TransientHostname {
-		return false
+		return fmt.Errorf("the TransientHostname differs")
 	}
 
-	return true
+	return nil
 }
 
 // HostnameUID is the UID struct for HostnameRes.

engine/resources/kv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -18,13 +18,16 @@
 package resources
 
 import (
+	"context"
 	"fmt"
 	"strconv"
+	"sync"
+	"time"
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -40,6 +43,10 @@ const (
 	SkipCmpStyleString
 )
 
+const (
+	kvCheckApplyTimeout = 5 * time.Second
+)
+
 // KVRes is a resource which writes a key/value pair into cluster wide storage.
 // It will ensure that the key is set to the requested value. The one exception
 // is that if you use the SkipLessThan parameter, then it will only replace the
@@ -56,14 +63,32 @@ type KVRes struct {
 
 	init *engine.Init
 
-	// XXX: shouldn't the name be the key?
-	Key          string            `yaml:"key"`          // key to set
-	Value        *string           `yaml:"value"`        // value to set (nil to delete)
-	SkipLessThan bool              `yaml:"skiplessthan"` // skip updates as long as stored value is greater
-	SkipCmpStyle KVResSkipCmpStyle `yaml:"skipcmpstyle"` // how to do the less than cmp
+	// Key represents the key to set. If it is not specified, the Name value
+	// is used instead.
+	Key string `lang:"key" yaml:"key"`
+	// Value represents the string value to set. If this value is nil or,
+	// undefined, then this will delete that key.
+	Value *string `lang:"value" yaml:"value"`
+	// SkipLessThan causes the value to be updated as long as it is greater.
+	SkipLessThan bool `lang:"skiplessthan" yaml:"skiplessthan"`
+	// SkipCmpStyle is the type of compare function used when determining if
+	// the value is greater when using the SkipLessThan parameter.
+	SkipCmpStyle KVResSkipCmpStyle `lang:"skipcmpstyle" yaml:"skipcmpstyle"`
+
+	interruptChan chan struct{}
+
 	// TODO: does it make sense to have different backends here? (eg: local)
 }
 
+// getKey returns the key to be used for this resource. If the Key field is
+// specified, it will use that, otherwise it uses the Name.
+func (obj *KVRes) getKey() string {
+	if obj.Key != "" {
+		return obj.Key
+	}
+	return obj.Name()
+}
+
 // Default returns some sensible defaults for this resource.
 func (obj *KVRes) Default() engine.Res {
 	return &KVRes{}
@@ -71,7 +96,7 @@ func (obj *KVRes) Default() engine.Res {
 
 // Validate if the params passed in are valid data.
 func (obj *KVRes) Validate() error {
-	if obj.Key == "" {
+	if obj.getKey() == "" {
 		return fmt.Errorf("key must not be empty")
 	}
 	if obj.SkipLessThan {
@@ -92,6 +117,8 @@ func (obj *KVRes) Validate() error {
 func (obj *KVRes) Init(init *engine.Init) error {
 	obj.init = init // save for later
 
+	obj.interruptChan = make(chan struct{})
+
 	return nil
 }
 
@@ -102,9 +129,17 @@ func (obj *KVRes) Close() error {
 
 // Watch is the primary listener for this resource and it outputs events.
 func (obj *KVRes) Watch() error {
-	obj.init.Running() // when started, notify engine that we're running
+	// FIXME: add timeout to context
+	// The obj.init.Done channel is closed by the engine to signal shutdown.
+	ctx, cancel := util.ContextWithCloser(context.Background(), obj.init.Done)
+	defer cancel()
 
-	ch := obj.init.World.StrMapWatch(obj.Key) // get possible events!
+	ch, err := obj.init.World.StrMapWatch(ctx, obj.getKey()) // get possible events!
+	if err != nil {
+		return err
+	}
+
+	obj.init.Running() // when started, notify engine that we're running
 
 	var send = false // send event?
 	for {
@@ -118,7 +153,7 @@ func (obj *KVRes) Watch() error {
 				return errwrap.Wrapf(err, "unknown %s watcher error", obj)
 			}
 			if obj.init.Debug {
-				obj.init.Logf("Event!")
+				obj.init.Logf("event!")
 			}
 			send = true
 
@@ -135,7 +170,7 @@ func (obj *KVRes) Watch() error {
 }
 
 // lessThanCheck checks for less than validity.
-func (obj *KVRes) lessThanCheck(value string) (checkOK bool, err error) {
+func (obj *KVRes) lessThanCheck(value string) (bool, error) {
 	v := *obj.Value
 	if value == v { // redundant check for safety
 		return true, nil
@@ -173,16 +208,31 @@ func (obj *KVRes) lessThanCheck(value string) (checkOK bool, err error) {
 }
 
 // CheckApply method for Password resource. Does nothing, returns happy!
-func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *KVRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
+	wg := &sync.WaitGroup{}
+	defer wg.Wait() // this must be above the defer cancel() call
+	ctx, cancel := context.WithTimeout(context.Background(), kvCheckApplyTimeout)
+	defer cancel()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-obj.interruptChan:
+			cancel()
+		case <-ctx.Done():
+			// let this exit
+		}
+	}()
+
 	if val, exists := obj.init.Recv()["Value"]; exists && val.Changed {
 		// if we received on Value, and it changed, wooo, nothing to do.
 		obj.init.Logf("CheckApply: `Value` was updated!")
 	}
 
 	hostname := obj.init.Hostname // me
-	keyMap, err := obj.init.World.StrMapGet(obj.Key)
+	keyMap, err := obj.init.World.StrMapGet(ctx, obj.getKey())
 	if err != nil {
 		return false, errwrap.Wrapf(err, "check error during StrGet")
 	}
@@ -202,7 +252,7 @@ func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
 		return true, nil // nothing to delete, we're good!
 
 	} else if ok && obj.Value == nil { // delete
-		err := obj.init.World.StrMapDel(obj.Key)
+		err := obj.init.World.StrMapDel(ctx, obj.getKey())
 		return false, errwrap.Wrapf(err, "apply error during StrDel")
 	}
 
@@ -210,7 +260,7 @@ func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
 		return false, nil
 	}
 
-	if err := obj.init.World.StrMapSet(obj.Key, *obj.Value); err != nil {
+	if err := obj.init.World.StrMapSet(ctx, obj.getKey(), *obj.Value); err != nil {
 		return false, errwrap.Wrapf(err, "apply error during StrSet")
 	}
 
@@ -219,39 +269,37 @@ func (obj *KVRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *KVRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *KVRes) Compare(r engine.Res) bool {
 	// we can only compare KVRes to others of the same resource kind
 	res, ok := r.(*KVRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
-	if obj.Key != res.Key {
-		return false
+	if obj.getKey() != res.getKey() {
+		return fmt.Errorf("the Key differs")
 	}
 	if (obj.Value == nil) != (res.Value == nil) { // xor
-		return false
+		return fmt.Errorf("the Value differs")
 	}
 	if obj.Value != nil && res.Value != nil {
 		if *obj.Value != *res.Value { // compare the strings
-			return false
+			return fmt.Errorf("the contents of Value differs")
 		}
 	}
 	if obj.SkipLessThan != res.SkipLessThan {
-		return false
+		return fmt.Errorf("the SkipLessThan param differs")
 	}
 	if obj.SkipCmpStyle != res.SkipCmpStyle {
-		return false
+		return fmt.Errorf("the SkipCmpStyle param differs")
 	}
 
-	return true
+	return nil
+}
+
+// Interrupt is called to ask the execution of this resource to end early.
+func (obj *KVRes) Interrupt() error {
+	close(obj.interruptChan)
+	return nil
 }
 
 // KVUID is the UID struct for KVRes.

engine/resources/mount.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -33,13 +33,13 @@ import (
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	sdbus "github.com/coreos/go-systemd/dbus"
 	"github.com/coreos/go-systemd/unit"
 	systemdUtil "github.com/coreos/go-systemd/util"
 	fstab "github.com/deniswernert/go-fstab"
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 )
 
@@ -75,6 +75,8 @@ const (
 	// diskByLabel is the location of symlinks for partitions by label.
 	diskByPartLabel = devDisk + "by-partlabel/"
 
+	// dbusSystemdService is the service to connect to systemd itself.
+	dbusSystemd1Service = "org.freedesktop.systemd1"
 	// dbusSystemd1Interface is the base systemd1 path.
 	dbusSystemd1Path = "/org/freedesktop/systemd1"
 	// dbusUnitPath is the dbus path where mount unit files are found.
@@ -88,6 +90,9 @@ const (
 	dbusManagerInterface = dbusSystemd1Interface + ".Manager"
 	// dbusRestartUnit is the dbus method for restarting systemd units.
 	dbusRestartUnit = dbusManagerInterface + ".RestartUnit"
+	// dbusReloadSystemd is the dbus method for reloading systemd settings.
+	// (i.e. systemctl daemon-reload)
+	dbusReloadSystemd = dbusManagerInterface + ".Reload"
 	// restartTimeout is the delay before restartUnit is assumed to have
 	// failed.
 	dbusRestartCtxTimeout = 10
@@ -275,7 +280,7 @@ func (obj *MountRes) Watch() error {
 
 // fstabCheckApply checks /etc/fstab for entries corresponding to the resource
 // definition, and adds or deletes the entry as needed.
-func (obj *MountRes) fstabCheckApply(apply bool) (checkOK bool, err error) {
+func (obj *MountRes) fstabCheckApply(apply bool) (bool, error) {
 	exists, err := fstabEntryExists(fstabPath, obj.mount)
 	if err != nil {
 		return false, errwrap.Wrapf(err, "error checking if fstab entry exists")
@@ -339,8 +344,8 @@ func (obj *MountRes) mountCheckApply(apply bool) (bool, error) {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *MountRes) CheckApply(apply bool) (checkOK bool, err error) {
-	checkOK = true
+func (obj *MountRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
 
 	if c, err := obj.fstabCheckApply(apply); err != nil {
 		return false, err
@@ -576,7 +581,10 @@ func mountReload() error {
 	}
 	defer conn.Close()
 	// systemctl daemon-reload
-	conn.BusObject().Call("Reload", 0)
+	call := conn.Object(dbusSystemd1Service, dbusSystemd1Path).Call(dbusReloadSystemd, 0)
+	if call.Err != nil {
+		return errwrap.Wrapf(call.Err, "error reloading systemd")
+	}
 
 	// systemctl restart local-fs.target
 	if err := restartUnit(conn, "local-fs.target"); err != nil {
@@ -584,7 +592,7 @@ func mountReload() error {
 	}
 
 	// systemctl restart remote-fs.target
-	if err := restartUnit(conn, "local-fs.target"); err != nil {
+	if err := restartUnit(conn, "remote-fs.target"); err != nil {
 		return errwrap.Wrapf(err, "error restarting unit")
 	}
 
@@ -619,7 +627,7 @@ func restartUnit(conn *dbus.Conn, unit string) error {
 	defer conn.RemoveSignal(ch)
 
 	// restart the unit
-	sd1 := conn.Object(dbusSystemd1Interface, dbus.ObjectPath(dbusSystemd1Path))
+	sd1 := conn.Object(dbusSystemd1Service, dbus.ObjectPath(dbusSystemd1Path))
 	if call := sd1.Call(dbusRestartUnit, 0, unit, "fail"); call.Err != nil {
 		return errwrap.Wrapf(call.Err, "error restarting unit: %s", unit)
 	}

engine/resources/mount_linux_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/mount_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/msg.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -200,36 +200,28 @@ func (obj *MsgRes) CheckApply(apply bool) (bool, error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *MsgRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *MsgRes) Compare(r engine.Res) bool {
 	// we can only compare MsgRes to others of the same resource kind
 	res, ok := r.(*MsgRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.Body != res.Body {
-		return false
+		return fmt.Errorf("the Body differs")
 	}
 	if obj.Priority != res.Priority {
-		return false
+		return fmt.Errorf("the Priority differs")
 	}
 	if len(obj.Fields) != len(res.Fields) {
-		return false
+		return fmt.Errorf("the length of Fields differs")
 	}
 	for field, value := range obj.Fields {
 		if res.Fields[field] != value {
-			return false
+			return fmt.Errorf("the Fields differ")
 		}
 	}
 
-	return true
+	return nil
 }
 
 // MsgUID is a unique representation for a MsgRes object.

engine/resources/msg_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/net.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -34,10 +34,9 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 	"github.com/purpleidea/mgmt/util/socketset"
 
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 	// XXX: Do NOT use subscribe methods from this lib, as they are racey and
 	// do not clean up spawned goroutines. Should be replaced when a suitable
 	// alternative is available.
@@ -179,9 +178,7 @@ func (obj *NetRes) Close() error {
 		return fmt.Errorf("socket file should not be the root path")
 	}
 	if obj.socketFile != "" { // safety
-		if err := os.Remove(obj.socketFile); err != nil {
-			errList = multierr.Append(errList, err)
-		}
+		errList = errwrap.Append(errList, os.Remove(obj.socketFile))
 	}
 
 	return errList
@@ -463,8 +460,8 @@ func (obj *NetRes) fileCheckApply(apply bool) (bool, error) {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *NetRes) CheckApply(apply bool) (checkOK bool, err error) {
-	checkOK = true
+func (obj *NetRes) CheckApply(apply bool) (bool, error) {
+	checkOK := true
 
 	// check the network device
 	if c, err := obj.ifaceCheckApply(apply); err != nil {
@@ -509,34 +506,26 @@ func (obj *NetRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *NetRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *NetRes) Compare(r engine.Res) bool {
 	// we can only compare NetRes to others of the same resource kind
 	res, ok := r.(*NetRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if (obj.Addrs == nil) != (res.Addrs == nil) {
-		return false
+		return fmt.Errorf("the Addrs differ")
 	}
 	if err := util.SortedStrSliceCompare(obj.Addrs, res.Addrs); err != nil {
-		return false
+		return fmt.Errorf("the Addrs differ")
 	}
 	if obj.Gateway != res.Gateway {
-		return false
+		return fmt.Errorf("the Gateway differs")
 	}
 
-	return true
+	return nil
 }
 
 // NetUID is a unique resource identifier.

engine/resources/net_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/noop.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -75,7 +75,7 @@ func (obj *NoopRes) Watch() error {
 }
 
 // CheckApply method for Noop resource. Does nothing, returns happy!
-func (obj *NoopRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *NoopRes) CheckApply(apply bool) (bool, error) {
 	if obj.init.Refresh() {
 		obj.init.Logf("received a notification!")
 	}

engine/resources/noop_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/nspawn.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -21,18 +21,19 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
+	"strings"
 	"unicode"
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	systemdDbus "github.com/coreos/go-systemd/dbus"
 	machined "github.com/coreos/go-systemd/machine1"
 	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus"
-	errwrap "github.com/pkg/errors"
 )
 
 const (
@@ -201,7 +202,7 @@ func (obj *NspawnRes) Watch() error {
 // CheckApply is run to check the state and, if apply is true, to apply the
 // necessary changes to reach the desired state. This is run before Watch and
 // again if Watch finds a change occurring to the state.
-func (obj *NspawnRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *NspawnRes) CheckApply(apply bool) (bool, error) {
 	// this resource depends on systemd to ensure that it's running
 	if !systemdUtil.IsRunningSystemd() {
 		return false, errors.New("systemd is not running")
@@ -260,35 +261,27 @@ func (obj *NspawnRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *NspawnRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *NspawnRes) Compare(r engine.Res) bool {
 	// we can only compare NspawnRes to others of the same resource kind
 	res, ok := r.(*NspawnRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 
 	// TODO: why is res.svc ever nil?
 	if (obj.svc == nil) != (res.svc == nil) { // xor
-		return false
+		return fmt.Errorf("the svc differs")
 	}
 	if obj.svc != nil && res.svc != nil {
-		if !obj.svc.Compare(res.svc) {
-			return false
+		if err := obj.svc.Cmp(res.svc); err != nil {
+			return errwrap.Wrapf(err, "the svc differs")
 		}
 	}
 
-	return true
+	return nil
 }
 
 // NspawnUID is a unique resource identifier.
@@ -358,10 +351,12 @@ func systemdVersion() (uint16, error) {
 		return 0, errwrap.Wrapf(err, "could not get version property")
 	}
 	// lose the surrounding quotes
-	verNum, err := strconv.Unquote(verString)
+	verNumString, err := strconv.Unquote(verString)
 	if err != nil {
 		return 0, errwrap.Wrapf(err, "error unquoting version number")
 	}
+	// trim possible version suffix like in "242.19-1"
+	verNum := strings.Split(verNumString, ".")[0]
 	// cast to uint16
 	ver, err := strconv.ParseUint(verNum, 10, 16)
 	if err != nil {

engine/resources/packagekit/packagekit.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -27,10 +27,9 @@ import (
 
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/godbus/dbus"
-	multierr "github.com/hashicorp/go-multierror"
-	errwrap "github.com/pkg/errors"
 )
 
 // global tweaks of verbosity and code path
@@ -198,9 +197,8 @@ func (obj *Conn) matchSignal(ch chan *dbus.Signal, path dbus.ObjectPath, iface s
 	removeSignals := func() error {
 		var errList error
 		for i := len(argsList) - 1; i >= 0; i-- { // last in first out
-			if call := bus.Call(engineUtil.DBusRemoveMatch, 0, argsList[i]); call.Err != nil {
-				errList = multierr.Append(errList, call.Err)
-			}
+			call := bus.Call(engineUtil.DBusRemoveMatch, 0, argsList[i])
+			errList = errwrap.Append(errList, call.Err)
 		}
 		return errList
 	}
@@ -354,7 +352,7 @@ loop:
 				// should already be broken
 				break loop
 			} else {
-				return []string{}, fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return []string{}, fmt.Errorf("error in body: %v", signal.Body)
 			}
 		}
 	}
@@ -365,9 +363,9 @@ loop:
 func (obj *Conn) IsInstalledList(packages []string) ([]bool, error) {
 	var filter uint64          // initializes at the "zero" value of 0
 	filter += PkFilterEnumArch // always search in our arch
-	packageIDs, e := obj.ResolvePackages(packages, filter)
-	if e != nil {
-		return nil, fmt.Errorf("ResolvePackages error: %v", e)
+	packageIDs, err := obj.ResolvePackages(packages, filter)
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "error resolving packages")
 	}
 
 	var m = make(map[string]int)
@@ -445,7 +443,7 @@ loop:
 			}
 
 			if signal.Name == FmtTransactionMethod("ErrorCode") {
-				return fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return fmt.Errorf("error in body: %v", signal.Body)
 			} else if signal.Name == FmtTransactionMethod("Package") {
 				// a package was installed...
 				// only start the timer once we're here...
@@ -456,14 +454,14 @@ loop:
 			} else if signal.Name == FmtTransactionMethod("Destroy") {
 				return nil // success
 			} else {
-				return fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return fmt.Errorf("error in body: %v", signal.Body)
 			}
 		case <-util.TimeAfterOrBlock(timeout):
 			if finished {
 				obj.Logf("Timeout: InstallPackages: Waiting for 'Destroy'")
 				return nil // got tired of waiting for Destroy
 			}
-			return fmt.Errorf("PackageKit: Timeout: InstallPackages: %s", strings.Join(packageIDs, ", "))
+			return fmt.Errorf("timeout installing packages: %s", strings.Join(packageIDs, ", "))
 		}
 	}
 }
@@ -502,7 +500,7 @@ loop:
 			}
 
 			if signal.Name == FmtTransactionMethod("ErrorCode") {
-				return fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return fmt.Errorf("error in body: %v", signal.Body)
 			} else if signal.Name == FmtTransactionMethod("Package") {
 				// a package was installed...
 				continue loop
@@ -513,7 +511,7 @@ loop:
 				// should already be broken
 				break loop
 			} else {
-				return fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return fmt.Errorf("error in body: %v", signal.Body)
 			}
 		}
 	}
@@ -551,7 +549,7 @@ loop:
 			}
 
 			if signal.Name == FmtTransactionMethod("ErrorCode") {
-				return fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return fmt.Errorf("error in body: %v", signal.Body)
 			} else if signal.Name == FmtTransactionMethod("Package") {
 			} else if signal.Name == FmtTransactionMethod("Finished") {
 				// TODO: should we wait for the Destroy signal?
@@ -560,7 +558,7 @@ loop:
 				// should already be broken
 				break loop
 			} else {
-				return fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return fmt.Errorf("error in body: %v", signal.Body)
 			}
 		}
 	}
@@ -603,7 +601,7 @@ loop:
 			}
 
 			if signal.Name == FmtTransactionMethod("ErrorCode") {
-				err = fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				err = fmt.Errorf("error in body: %v", signal.Body)
 				return
 
 				// one signal returned per packageID found...
@@ -628,7 +626,7 @@ loop:
 				// should already be broken
 				break loop
 			} else {
-				err = fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				err = fmt.Errorf("error in body: %v", signal.Body)
 				return
 			}
 		}
@@ -671,7 +669,7 @@ loop:
 			}
 
 			if signal.Name == FmtTransactionMethod("ErrorCode") {
-				return nil, fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return nil, fmt.Errorf("error in body: %v", signal.Body)
 			} else if signal.Name == FmtTransactionMethod("Package") {
 
 				//pkg_int, ok := signal.Body[0].(int)
@@ -694,7 +692,7 @@ loop:
 				// should already be broken
 				break loop
 			} else {
-				return nil, fmt.Errorf("PackageKit: Error: %v", signal.Body)
+				return nil, fmt.Errorf("error in body: %v", signal.Body)
 			}
 		}
 	}
@@ -720,9 +718,9 @@ func (obj *Conn) PackagesToPackageIDs(packageMap map[string]string, filter uint6
 	if obj.Debug {
 		obj.Logf("PackagesToPackageIDs(): %s", strings.Join(packages, ", "))
 	}
-	resolved, e := obj.ResolvePackages(packages, filter)
-	if e != nil {
-		return nil, fmt.Errorf("Resolve error: %v", e)
+	resolved, err := obj.ResolvePackages(packages, filter)
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "error resolving")
 	}
 
 	found := make([]bool, count) // default false
@@ -760,7 +758,7 @@ func (obj *Conn) PackagesToPackageIDs(packageMap map[string]string, filter uint6
 		}
 		state := packageMap[pkg] // lookup the requested state/version
 		if state == "" {
-			return nil, fmt.Errorf("Empty package state for %v", pkg)
+			return nil, fmt.Errorf("empty package state for: `%s`", pkg)
 		}
 		found[index] = true
 		stateIsVersion := (state != "installed" && state != "uninstalled" && state != "newest") // must be a ver. string
@@ -796,9 +794,9 @@ func (obj *Conn) PackagesToPackageIDs(packageMap map[string]string, filter uint6
 	// to be done, and if so, anything that needs updating isn't newest!
 	// if something isn't installed, we can't verify it with this method
 	// FIXME: https://github.com/hughsie/PackageKit/issues/116
-	updates, e := obj.GetUpdates(filter)
-	if e != nil {
-		return nil, fmt.Errorf("Updates error: %v", e)
+	updates, err := obj.GetUpdates(filter)
+	if err != nil {
+		return nil, errwrap.Wrapf(err, "updates error")
 	}
 	for _, packageID := range updates {
 		//obj.Logf("* %v", packageID)
@@ -846,9 +844,9 @@ func (obj *Conn) PackagesToPackageIDs(packageMap map[string]string, filter uint6
 			if obj.Debug {
 				obj.Logf("PackagesToPackageIDs(): Recurse: %s", strings.Join(checkPackages, ", "))
 			}
-			recursion, e = obj.PackagesToPackageIDs(filteredPackageMap, filter+PkFilterEnumNewest)
-			if e != nil {
-				return nil, fmt.Errorf("Recursion error: %v", e)
+			recursion, err = obj.PackagesToPackageIDs(filteredPackageMap, filter+PkFilterEnumNewest)
+			if err != nil {
+				return nil, errwrap.Wrapf(err, "recursion error")
 			}
 		}
 	}

engine/resources/password.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -29,8 +29,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -210,7 +209,7 @@ func (obj *PasswordRes) Watch() error {
 }
 
 // CheckApply method for Password resource. Does nothing, returns happy!
-func (obj *PasswordRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *PasswordRes) CheckApply(apply bool) (bool, error) {
 	var refresh = obj.init.Refresh() // do we have a pending reload to apply?
 	var exists = true                // does the file (aka the token) exist?
 	var generate bool                // do we need to generate a new password?
@@ -296,33 +295,25 @@ func (obj *PasswordRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *PasswordRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *PasswordRes) Compare(r engine.Res) bool {
 	// we can only compare PasswordRes to others of the same resource kind
 	res, ok := r.(*PasswordRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.Length != res.Length {
-		return false
+		return fmt.Errorf("the Length differs")
 	}
 	// TODO: we *could* optimize by allowing CheckApply to move from
 	// saved->!saved, by removing the file, but not likely worth it!
 	if obj.Saved != res.Saved {
-		return false
+		return fmt.Errorf("the Saved differs")
 	}
 	if obj.CheckRecovery != res.CheckRecovery {
-		return false
+		return fmt.Errorf("the CheckRecovery differs")
 	}
 
-	return true
+	return nil
 }
 
 // PasswordUID is the UID struct for PasswordRes.
@@ -344,7 +335,7 @@ func (obj *PasswordRes) UIDs() []engine.ResUID {
 // PasswordSends is the struct of data which is sent after a successful Apply.
 type PasswordSends struct {
 	// Password is the generated password being sent.
-	Password *string
+	Password *string `lang:"password"`
 	// Hashing is the algorithm used for this password. Empty is plain text.
 	Hashing string // TODO: implement me
 }

engine/resources/pkg.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -26,8 +26,7 @@ import (
 	"github.com/purpleidea/mgmt/engine/resources/packagekit"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/util"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -263,7 +262,7 @@ func (obj *PkgRes) populateFileList() error {
 
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
-func (obj *PkgRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *PkgRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("Check: %s", obj.fmtNames(obj.getNames()))
 
 	bus := packagekit.NewBus()

engine/resources/pkg_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/print.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -40,6 +40,10 @@ type PrintRes struct {
 	init *engine.Init
 
 	Msg string `lang:"msg" yaml:"msg"` // the message to display
+	// RefreshOnly is an option that causes the message to be printed only
+	// when notified by another resource. When set to true, this resource
+	// cannot be autogrouped.
+	RefreshOnly bool `lang:"refresh_only" yaml:"refresh_only"`
 }
 
 // Default returns some sensible defaults for this resource.
@@ -78,24 +82,32 @@ func (obj *PrintRes) Watch() error {
 }
 
 // CheckApply method for Print resource. Does nothing, returns happy!
-func (obj *PrintRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *PrintRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply: %t", apply)
 	if val, exists := obj.init.Recv()["Msg"]; exists && val.Changed {
 		// if we received on Msg, and it changed, log message
 		obj.init.Logf("CheckApply: Received `Msg` of: %s", obj.Msg)
 	}
 
-	if obj.init.Refresh() {
+	var refresh = obj.init.Refresh()
+	// we output if not RefreshOnly, or we are in refresh mode and RefreshOnly
+	var display = refresh || !obj.RefreshOnly
+
+	if refresh {
 		obj.init.Logf("Received a notification!")
 	}
-	obj.init.Logf("Msg: %s", obj.Msg)
+	if display {
+		obj.init.Logf("Msg: %s", obj.Msg)
+	}
 	if g := obj.GetGroup(); len(g) > 0 { // add any grouped elements
 		for _, x := range g {
 			print, ok := x.(*PrintRes) // convert from Res
 			if !ok {
 				panic(fmt.Sprintf("grouped member %v is not a %s", x, obj.Kind()))
 			}
-			obj.init.Logf("%s: Msg: %s", print, print.Msg)
+			if display {
+				obj.init.Logf("%s: Msg: %s", print, print.Msg)
+			}
 		}
 	}
 	return true, nil // state is always okay
@@ -103,24 +115,16 @@ func (obj *PrintRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *PrintRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *PrintRes) Compare(r engine.Res) bool {
 	// we can only compare PrintRes to others of the same resource kind
 	res, ok := r.(*PrintRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.Msg != res.Msg {
-		return false
+		return fmt.Errorf("the Msg differs")
 	}
-	return true
+	return nil
 }
 
 // PrintUID is the UID struct for PrintRes.
@@ -141,10 +145,14 @@ func (obj *PrintRes) UIDs() []engine.ResUID {
 
 // GroupCmp returns whether two resources can be grouped together or not.
 func (obj *PrintRes) GroupCmp(r engine.GroupableRes) error {
-	_, ok := r.(*PrintRes)
+	res, ok := r.(*PrintRes)
 	if !ok {
 		return fmt.Errorf("resource is not the same kind")
 	}
+	// we don't group if it's RefreshOnly: only the notifier may trigger
+	if obj.RefreshOnly || res.RefreshOnly {
+		return fmt.Errorf("resource uses RefreshOnly, it cannot be merged")
+	}
 	return nil // grouped together if we were asked to
 }
 

engine/resources/resources_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -30,6 +30,7 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 // TODO: consider providing this as a lib so that we can add tests into the
@@ -152,6 +153,45 @@ func NewClearChangedStep(ms uint) Step {
 	}
 }
 
+// FileExpect takes a path and a string to expect in that file, and builds a
+// Step that checks that out of them.
+func FileExpect(p, s string) Step { // path & string
+	return &manualStep{
+		action: func() error { return nil },
+		expect: func() error {
+			content, err := ioutil.ReadFile(p)
+			if err != nil {
+				return err
+			}
+			if string(content) != s {
+				return fmt.Errorf("contents did not match in %s", p)
+			}
+			return nil
+		},
+	}
+}
+
+// FileExpect takes a path and a string to write to that file, and builds a Step
+// that does that to them.
+func FileWrite(p, s string) Step { // path & string
+	return &manualStep{
+		action: func() error {
+			// TODO: apparently using 0666 is equivalent to respecting the current umask
+			const umask = 0666
+			return ioutil.WriteFile(p, []byte(s), umask)
+		},
+		expect: func() error { return nil },
+	}
+}
+
+// ErrIsNotExistOK returns nil if we get an IsNotExist true result on the error.
+func ErrIsNotExistOK(e error) error {
+	if os.IsNotExist(e) {
+		return nil
+	}
+	return errwrap.Wrapf(e, "unexpected error")
+}
+
 func TestResources1(t *testing.T) {
 	type test struct { // an individual test
 		name      string
@@ -161,6 +201,7 @@ func TestResources1(t *testing.T) {
 		experrstr string       // expected error prefix
 		timeline  []Step       // TODO: this could be a generator that keeps pushing out steps until it's done!
 		expect    func() error // function to check for expected state
+		startup   func() error // function to run as startup
 		cleanup   func() error // function to run as cleanup
 	}
 
@@ -176,59 +217,118 @@ func TestResources1(t *testing.T) {
 			expect: func() error { return nil },
 		}
 	}
-	fileExpect := func(p, s string) Step { // path & string
-		return &manualStep{
-			action: func() error { return nil },
-			expect: func() error {
-				content, err := ioutil.ReadFile(p)
-				if err != nil {
-					return err
-				}
-				if string(content) != s {
-					return fmt.Errorf("contents did not match in %s", p)
-				}
-				return nil
-			},
-		}
-	}
-	fileWrite := func(p, s string) Step { // path & string
-		return &manualStep{
-			action: func() error {
-				// TODO: apparently using 0666 is equivalent to respecting the current umask
-				const umask = 0666
-				return ioutil.WriteFile(p, []byte(s), umask)
-			},
-			expect: func() error { return nil },
-		}
-	}
 
 	testCases := []test{}
 	{
-		res := makeRes("file", "r1")
-		r := res.(*FileRes) // if this panics, the test will panic
+		r := makeRes("file", "r1")
+		res := r.(*FileRes) // if this panics, the test will panic
 		p := "/tmp/whatever"
 		s := "hello, world\n"
-		r.Path = p
+		res.Path = p
+		res.State = "exists"
 		contents := s
-		r.Content = &contents
+		res.Content = &contents
 
 		timeline := []Step{
 			NewStartupStep(1000 * 60),          // startup
 			NewChangedStep(1000*60, false),     // did we do something?
-			fileExpect(p, s),                   // check initial state
+			FileExpect(p, s),                   // check initial state
 			NewClearChangedStep(1000 * 15),     // did we do something?
-			fileWrite(p, "this is whatever\n"), // change state
+			FileWrite(p, "this is whatever\n"), // change state
 			NewChangedStep(1000*60, false),     // did we do something?
-			fileExpect(p, s),                   // check again
+			FileExpect(p, s),                   // check again
 			sleep(1),                           // we can sleep too!
 		}
 
 		testCases = append(testCases, test{
-			name:     "simple res",
+			name:     "simple file",
 			res:      res,
 			fail:     false,
 			timeline: timeline,
 			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return os.Remove(p) },
+		})
+	}
+	{
+		r := makeRes("exec", "x1")
+		res := r.(*ExecRes) // if this panics, the test will panic
+		s := "hello, world"
+		f := "/tmp/whatever"
+		res.Cmd = fmt.Sprintf("echo '%s' > '%s'", s, f)
+		res.Shell = "/bin/bash"
+		res.IfCmd = "! diff <(cat /tmp/whatever) <(echo hello, world)"
+		res.IfShell = "/bin/bash"
+		res.WatchCmd = fmt.Sprintf("/usr/bin/inotifywait -e modify -m %s", f)
+		//res.WatchShell = "/bin/bash"
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),        // startup
+			NewChangedStep(1000*60, false),   // did we do something?
+			FileExpect(f, s+"\n"),            // check initial state
+			NewClearChangedStep(1000 * 15),   // did we do something?
+			FileWrite(f, "this is stuff!\n"), // change state
+			NewChangedStep(1000*60, false),   // did we do something?
+			FileExpect(f, s+"\n"),            // check again
+			sleep(1),                         // we can sleep too!
+		}
+
+		testCases = append(testCases, test{
+			name:     "simple exec",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			// build file for inotifywait
+			startup: func() error { return ioutil.WriteFile(f, []byte("starting...\n"), 0666) },
+			cleanup: func() error { return os.Remove(f) },
+		})
+	}
+	{
+		r := makeRes("file", "r1")
+		res := r.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/emptyfile"
+		res.Path = p
+		res.State = "exists"
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),      // startup
+			NewChangedStep(1000*60, false), // did we do something?
+			FileExpect(p, ""),              // check initial state
+			NewClearChangedStep(1000 * 15), // did we do something?
+		}
+
+		testCases = append(testCases, test{
+			name:     "touch file",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return os.Remove(p) },
+		})
+	}
+	{
+		r := makeRes("file", "r1")
+		res := r.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/existingfile"
+		res.Path = p
+		res.State = "exists"
+		content := "some existing text\n"
+
+		timeline := []Step{
+			NewStartupStep(1000 * 60),     // startup
+			NewChangedStep(1000*60, true), // did we do something?
+			FileExpect(p, content),        // check initial state
+		}
+
+		testCases = append(testCases, test{
+			name:     "existing file",
+			res:      res,
+			fail:     false,
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return ioutil.WriteFile(p, []byte(content), 0666) },
 			cleanup:  func() error { return os.Remove(p) },
 		})
 	}
@@ -245,7 +345,7 @@ func TestResources1(t *testing.T) {
 		}
 		names = append(names, tc.name)
 		t.Run(fmt.Sprintf("test #%d (%s)", index, tc.name), func(t *testing.T) {
-			res, fail, experr, experrstr, timeline, expect, cleanup := tc.res, tc.fail, tc.experr, tc.experrstr, tc.timeline, tc.expect, tc.cleanup
+			res, fail, experr, experrstr, timeline, expect, startup, cleanup := tc.res, tc.fail, tc.experr, tc.experrstr, tc.timeline, tc.expect, tc.startup, tc.cleanup
 
 			t.Logf("\n\ntest #%d: Res: %+v\n", index, res)
 			defer t.Logf("test #%d: done!", index)
@@ -288,7 +388,7 @@ func TestResources1(t *testing.T) {
 			doneChan := make(chan struct{})
 			debug := testing.Verbose() // set via the -test.v flag to `go test`
 			logf := func(format string, v ...interface{}) {
-				t.Logf(fmt.Sprintf("test #%d: Res: ", index)+format, v...)
+				t.Logf(fmt.Sprintf("test #%d: ", index)+format, v...)
 			}
 			init := &engine.Init{
 				Running: func() {
@@ -312,11 +412,19 @@ func TestResources1(t *testing.T) {
 				Logf:  logf,
 
 				// unused
+				Send: func(st interface{}) error {
+					return nil
+				},
 				Recv: func() map[string]*engine.Send {
 					return map[string]*engine.Send{}
 				},
 			}
 
+			t.Logf("test #%d: running startup()", index)
+			if err := startup(); err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: could not startup: %+v", index, err)
+			}
 			// run init
 			t.Logf("test #%d: running Init", index)
 			err = res.Init(init)
@@ -435,6 +543,7 @@ func TestResources1(t *testing.T) {
 					t.Errorf("test #%d: CheckApply failed: %s", index, err.Error())
 					return
 				}
+				//t.Logf("test #%d: CheckApply(true) (%t, %+v)", index, checkOK, err)
 				select {
 				// send a msg if we can, but never block
 				case changedChan <- checkOK:
@@ -455,3 +564,619 @@ func TestResources1(t *testing.T) {
 		})
 	}
 }
+
+// TestResources2 just tests a partial execution of the resource by running
+// CheckApply and Reverse and basics without the mainloop. It's a less accurate
+// representation of a running resource, but is still useful for many
+// circumstances. This also uses a simpler timeline, because it was not possible
+// to get the reference passing of the reversed resource working with the fancy
+// version.
+func TestResources2(t *testing.T) {
+	type test struct { // an individual test
+		name     string
+		timeline []func() error // TODO: this could be a generator that keeps pushing out steps until it's done!
+		expect   func() error   // function to check for expected state
+		startup  func() error   // function to run as startup (unused?)
+		cleanup  func() error   // function to run as cleanup
+	}
+
+	// resValidate runs Validate on the res.
+	resValidate := func(res engine.Res) func() error {
+		// run Close
+		return func() error {
+			return res.Validate()
+		}
+	}
+	// resInit runs Init on the res.
+	resInit := func(res engine.Res) func() error {
+		logf := func(format string, v ...interface{}) {
+			// noop for now
+		}
+		init := &engine.Init{
+			//Debug: debug,
+			Logf: logf,
+
+			// unused
+			Send: func(st interface{}) error {
+				return nil
+			},
+			Recv: func() map[string]*engine.Send {
+				return map[string]*engine.Send{}
+			},
+		}
+		// run Init
+		return func() error {
+			return res.Init(init)
+
+		}
+	}
+	// resCheckApplyError runs CheckApply with noop = false for the res. It
+	// errors if the returned checkOK values isn't what we were expecting or
+	// if the errOK function returns an error when given a chance to inspect
+	// the returned error.
+	resCheckApplyError := func(res engine.Res, expCheckOK bool, errOK func(e error) error) func() error {
+		return func() error {
+			checkOK, err := res.CheckApply(true) // no noop!
+			if e := errOK(err); e != nil {
+				return errwrap.Wrapf(e, "error from CheckApply did not match expected")
+			}
+			if checkOK != expCheckOK {
+				return fmt.Errorf("result from CheckApply did not match expected: `%t` != `%t`", checkOK, expCheckOK)
+			}
+			return nil
+		}
+	}
+	// resCheckApply runs CheckApply with noop = false for the res. It
+	// errors if the returned checkOK values isn't what we were expecting or
+	// if there was an error.
+	resCheckApply := func(res engine.Res, expCheckOK bool) func() error {
+		errOK := func(e error) error {
+			if e == nil {
+				return nil
+			}
+			return errwrap.Wrapf(e, "unexpected error from CheckApply")
+		}
+		return resCheckApplyError(res, expCheckOK, errOK)
+	}
+	// resClose runs Close on the res.
+	resClose := func(res engine.Res) func() error {
+		// run Close
+		return func() error {
+			return res.Close()
+		}
+	}
+	// resReversal runs Reverse on the resource and stores the result in the
+	// rev variable. This should be called before the res CheckApply, and
+	// usually before Init, but after Validate.
+	resReversal := func(res engine.Res, rev *engine.Res) func() error {
+		return func() error {
+			r, ok := res.(engine.ReversibleRes)
+			if !ok {
+				return fmt.Errorf("res is not a ReversibleRes")
+			}
+
+			// We don't really need this to be checked here.
+			//if r.ReversibleMeta().Disabled {
+			//	return fmt.Errorf("res did not specify Meta:reverse")
+			//}
+
+			if r.ReversibleMeta().Reversal {
+				//logf("triangle reversal") // warn!
+			}
+
+			reversed, err := r.Reversed()
+			if err != nil {
+				return errwrap.Wrapf(err, "could not reverse: %s", r.String())
+			}
+			if reversed == nil {
+				return nil // this can't be reversed, or isn't implemented here
+			}
+
+			reversed.ReversibleMeta().Reversal = true // set this for later...
+
+			retRes, ok := reversed.(engine.Res)
+			if !ok {
+				return fmt.Errorf("not a Res")
+			}
+
+			*rev = retRes // store!
+			return nil
+		}
+	}
+	fileWrite := func(p, s string) func() error {
+		// write the file to path
+		return func() error {
+			return ioutil.WriteFile(p, []byte(s), 0666)
+		}
+	}
+	fileExpect := func(p, s string) func() error {
+		// check the contents at the path match the string we expect
+		return func() error {
+			content, err := ioutil.ReadFile(p)
+			if err != nil {
+				return err
+			}
+			if string(content) != s {
+				return fmt.Errorf("contents did not match in %s", p)
+			}
+			return nil
+		}
+	}
+	fileAbsent := func(p string) func() error {
+		// does the file exist?
+		return func() error {
+			_, err := os.Stat(p)
+			if !os.IsNotExist(err) {
+				return fmt.Errorf("file was supposed to be absent, got: %+v", err)
+			}
+			return nil
+		}
+	}
+	fileRemove := func(p string) func() error {
+		// remove the file at path
+		return func() error {
+			err := os.Remove(p)
+			// if the file isn't there, don't error
+			if err != nil && !os.IsNotExist(err) {
+				return err
+			}
+			return nil
+		}
+	}
+
+	testCases := []test{}
+	{
+		//file "/tmp/somefile" {
+		//	state => "exists",
+		//	content => "some new text\n",
+		//}
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		res.State = "exists"
+		content := "some new text\n"
+		res.Content = &content
+
+		timeline := []func() error{
+			fileWrite(p, "whatever"),
+			resValidate(r1),
+			resInit(r1),
+			resCheckApply(r1, false), // changed
+			fileExpect(p, content),
+			resCheckApply(r1, true), // it's already good
+			resClose(r1),
+			fileExpect(p, content), // ensure it exists
+		}
+
+		testCases = append(testCases, test{
+			name:     "simple file",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	# state is NOT specified
+		//	content => "some new text\n",
+		//}
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		//res.State = "exists" // not specified!
+		content := "some new text\n"
+		res.Content = &content
+
+		timeline := []func() error{
+			fileWrite(p, "whatever"),
+			resValidate(r1),
+			resInit(r1),
+			resCheckApply(r1, false), // changed
+			fileExpect(p, content),
+			resCheckApply(r1, true), // it's already good
+			resClose(r1),
+			fileExpect(p, content), // ensure it exists
+		}
+
+		testCases = append(testCases, test{
+			name:     "edit file only",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	# state is NOT specified
+		//	content => "some new text\n",
+		//}
+		// and no existing file exists! (therefore we want an error!)
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		//res.State = "exists" // not specified!
+		content := "some new text\n"
+		res.Content = &content
+
+		timeline := []func() error{
+			fileRemove(p), // nothing here
+			resValidate(r1),
+			resInit(r1),
+			resCheckApplyError(r1, false, ErrIsNotExistOK), // should error
+			resCheckApplyError(r1, false, ErrIsNotExistOK), // double check
+			resClose(r1),
+			fileAbsent(p), // ensure it's absent
+		}
+
+		testCases = append(testCases, test{
+			name:     "strict file",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	state => "absent",
+		//}
+		// and no existing file exists!
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		res.State = "absent"
+
+		timeline := []func() error{
+			fileRemove(p), // nothing here
+			resValidate(r1),
+			resInit(r1),
+			resCheckApply(r1, true),
+			resCheckApply(r1, true),
+			resClose(r1),
+			fileAbsent(p), // ensure it's absent
+		}
+
+		testCases = append(testCases, test{
+			name:     "absent file",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	state => "absent",
+		//}
+		// and a file already exists!
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		res.State = "absent"
+
+		timeline := []func() error{
+			fileWrite(p, "whatever"),
+			resValidate(r1),
+			resInit(r1),
+			resCheckApply(r1, false),
+			resCheckApply(r1, true),
+			resClose(r1),
+			fileAbsent(p), // ensure it's absent
+		}
+
+		testCases = append(testCases, test{
+			name:     "absent file pre-existing",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	content => "some new text\n",
+		//	state => "exists",
+		//
+		//	Meta:reverse => true,
+		//}
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		res.State = "exists"
+		content := "some new text\n"
+		res.Content = &content
+		original := "this is the original state\n" // original state
+		var r2 engine.Res                          // future reversed resource
+
+		timeline := []func() error{
+			fileWrite(p, original),
+			fileExpect(p, original),
+			resValidate(r1),
+			resReversal(r1, &r2), // runs in Init to snapshot
+			func() error { // random test
+				if st := r2.(*FileRes).State; st != "absent" {
+					return fmt.Errorf("unexpected state: %s", st)
+				}
+				return nil
+			},
+			resInit(r1),
+			resCheckApply(r1, false), // changed
+			fileExpect(p, content),
+			resCheckApply(r1, true), // it's already good
+			resClose(r1),
+			//resValidate(r2), // no!!!
+			func() error {
+				// wrap it b/c it is currently nil
+				return r2.Validate()
+			},
+			func() error {
+				return resInit(r2)()
+			},
+			func() error {
+				return resCheckApply(r2, false)()
+			},
+			func() error {
+				return resCheckApply(r2, true)()
+			},
+			func() error {
+				return resClose(r2)()
+			},
+			fileAbsent(p), // ensure it's absent
+		}
+
+		testCases = append(testCases, test{
+			name:     "some file",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	content => "some new text\n",
+		//
+		//	Meta:reverse => true,
+		//}
+		//# and there's an existing file at this path...
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		//res.State = "exists" // unspecified
+		content := "some new text\n"
+		res.Content = &content
+		original := "this is the original state\n" // original state
+		var r2 engine.Res                          // future reversed resource
+
+		timeline := []func() error{
+			fileWrite(p, original),
+			fileExpect(p, original),
+			resValidate(r1),
+			resReversal(r1, &r2), // runs in Init to snapshot
+			func() error { // random test
+				// state should be unspecified
+				if st := r2.(*FileRes).State; st == "absent" || st == "exists" {
+					return fmt.Errorf("unexpected state: %s", st)
+				}
+				return nil
+			},
+			resInit(r1),
+			resCheckApply(r1, false), // changed
+			fileExpect(p, content),
+			resCheckApply(r1, true), // it's already good
+			resClose(r1),
+			//resValidate(r2),
+			func() error {
+				// wrap it b/c it is currently nil
+				return r2.Validate()
+			},
+			func() error {
+				return resInit(r2)()
+			},
+			func() error {
+				return resCheckApply(r2, false)()
+			},
+			func() error {
+				return resCheckApply(r2, true)()
+			},
+			func() error {
+				return resClose(r2)()
+			},
+			fileExpect(p, original), // we restored the contents!
+			fileRemove(p),           // cleanup
+		}
+
+		testCases = append(testCases, test{
+			name:     "some file restore",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	content => "some new text\n",
+		//
+		//	Meta:reverse => true,
+		//}
+		//# and there's NO existing file at this path...
+		//# NOTE: This used to be a corner case subtlety for reversal.
+		//# Now that we error in this scenario before reversal, it's ok!
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		//res.State = "exists" // unspecified
+		content := "some new text\n"
+		res.Content = &content
+		var r2 engine.Res // future reversed resource
+
+		timeline := []func() error{
+			fileRemove(p), // ensure no file exists
+			resValidate(r1),
+			resReversal(r1, &r2), // runs in Init to snapshot
+			func() error { // random test
+				// state should be unspecified i think
+				// TODO: or should it be absent?
+				if st := r2.(*FileRes).State; st == "absent" || st == "exists" {
+					return fmt.Errorf("unexpected state: %s", st)
+				}
+				return nil
+			},
+			resInit(r1),
+			resCheckApplyError(r1, false, ErrIsNotExistOK), // changed
+			//fileExpect(p, content),
+			//resCheckApply(r1, true), // it's already good
+			resClose(r1),
+			//func() error {
+			//	// wrap it b/c it is currently nil
+			//	return r2.Validate()
+			//},
+			//func() error {
+			//	return resInit(r2)()
+			//},
+			//func() error { // it's already in the correct state
+			//	return resCheckApply(r2, true)()
+			//},
+			//func() error {
+			//	return resClose(r2)()
+			//},
+			//fileExpect(p, content), // we never changed it back...
+			//fileRemove(p),          // cleanup
+		}
+
+		testCases = append(testCases, test{
+			name:     "ambiguous file restore",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+	{
+		//file "/tmp/somefile" {
+		//	state => "absent",
+		//
+		//	Meta:reverse => true,
+		//}
+		r1 := makeRes("file", "r1")
+		res := r1.(*FileRes) // if this panics, the test will panic
+		p := "/tmp/somefile"
+		res.Path = p
+		res.State = "absent"
+		original := "this is the original state\n" // original state
+		var r2 engine.Res                          // future reversed resource
+
+		timeline := []func() error{
+			fileWrite(p, original),
+			fileExpect(p, original),
+			resValidate(r1),
+			resReversal(r1, &r2), // runs in Init to snapshot
+			func() error { // random test
+				if st := r2.(*FileRes).State; st != "exists" {
+					return fmt.Errorf("unexpected state: %s", st)
+				}
+				return nil
+			},
+			resInit(r1),
+			resCheckApply(r1, false), // changed
+			fileAbsent(p),            // ensure it got removed
+			resCheckApply(r1, true),  // it's already good
+			resClose(r1),
+			//resValidate(r2), // no!!!
+			func() error {
+				// wrap it b/c it is currently nil
+				return r2.Validate()
+			},
+			func() error {
+				return resInit(r2)()
+			},
+			func() error {
+				return resCheckApply(r2, false)()
+			},
+			func() error {
+				return resCheckApply(r2, true)()
+			},
+			func() error {
+				return resClose(r2)()
+			},
+			fileExpect(p, original), // ensure it's back to original
+		}
+
+		testCases = append(testCases, test{
+			name:     "some removal",
+			timeline: timeline,
+			expect:   func() error { return nil },
+			startup:  func() error { return nil },
+			cleanup:  func() error { return nil },
+		})
+	}
+
+	names := []string{}
+	for index, tc := range testCases { // run all the tests
+		if tc.name == "" {
+			t.Errorf("test #%d: not named", index)
+			continue
+		}
+		if util.StrInList(tc.name, names) {
+			t.Errorf("test #%d: duplicate sub test name of: %s", index, tc.name)
+			continue
+		}
+		names = append(names, tc.name)
+		t.Run(fmt.Sprintf("test #%d (%s)", index, tc.name), func(t *testing.T) {
+			timeline, expect, startup, cleanup := tc.timeline, tc.expect, tc.startup, tc.cleanup
+
+			t.Logf("test #%d: starting...\n", index)
+			defer t.Logf("test #%d: done!", index)
+
+			//debug := testing.Verbose() // set via the -test.v flag to `go test`
+			//logf := func(format string, v ...interface{}) {
+			//	t.Logf(fmt.Sprintf("test #%d: ", index)+format, v...)
+			//}
+
+			t.Logf("test #%d: running startup()", index)
+			if err := startup(); err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: could not startup: %+v", index, err)
+			}
+			defer func() {
+				t.Logf("test #%d: running cleanup()", index)
+				if err := cleanup(); err != nil {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: could not cleanup: %+v", index, err)
+				}
+			}()
+
+			// run timeline
+			t.Logf("test #%d: executing timeline", index)
+			for ix, step := range timeline {
+				t.Logf("test #%d: step(%d)...", index, ix)
+				if err := step(); err != nil {
+					t.Errorf("test #%d: FAIL", index)
+					t.Errorf("test #%d: step(%d) action failed: %s", index, ix, err.Error())
+					break
+				}
+			}
+
+			t.Logf("test #%d: shutting down...", index)
+
+			if err := expect(); err != nil {
+				t.Errorf("test #%d: FAIL", index)
+				t.Errorf("test #%d: expect failed: %s", index, err.Error())
+				return
+			}
+
+			// all done!
+		})
+	}
+}

engine/resources/svc.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -28,11 +28,11 @@ import (
 	"github.com/purpleidea/mgmt/engine/traits"
 	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	systemd "github.com/coreos/go-systemd/dbus" // change namespace
 	systemdUtil "github.com/coreos/go-systemd/util"
 	"github.com/godbus/dbus" // namespace collides with systemd wrapper
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -229,12 +229,13 @@ func (obj *SvcRes) Watch() error {
 
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
-func (obj *SvcRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *SvcRes) CheckApply(apply bool) (bool, error) {
 	if !systemdUtil.IsRunningSystemd() {
 		return false, fmt.Errorf("systemd is not running")
 	}
 
 	var conn *systemd.Conn
+	var err error
 	if obj.Session {
 		conn, err = systemd.NewUserConnection() // user session
 	} else {
@@ -353,31 +354,23 @@ func (obj *SvcRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *SvcRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *SvcRes) Compare(r engine.Res) bool {
 	// we can only compare SvcRes to others of the same resource kind
 	res, ok := r.(*SvcRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if obj.Startup != res.Startup {
-		return false
+		return fmt.Errorf("the Startup differs")
 	}
 	if obj.Session != res.Session {
-		return false
+		return fmt.Errorf("the Session differs")
 	}
 
-	return true
+	return nil
 }
 
 // SvcUID is the UID struct for SvcRes.

engine/resources/test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -137,7 +137,7 @@ func (obj *TestRes) Watch() error {
 }
 
 // CheckApply method for Test resource. Does nothing, returns happy!
-func (obj *TestRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *TestRes) CheckApply(apply bool) (bool, error) {
 	for key, val := range obj.init.Recv() {
 		obj.init.Logf("CheckApply: Received `%s`, changed: %t", key, val.Changed)
 	}
@@ -199,25 +199,17 @@ func (obj *TestRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *TestRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *TestRes) Compare(r engine.Res) bool {
 	// we can only compare TestRes to others of the same resource kind
 	res, ok := r.(*TestRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 	//if obj.Name != res.Name {
 	//	return false
 	//}
 
 	if obj.CompareFail || res.CompareFail {
-		return false
+		return fmt.Errorf("the CompareFail is true")
 	}
 
 	// TODO: yes, I know the long manual version is absurd, but I couldn't
@@ -228,145 +220,145 @@ func (obj *TestRes) Compare(r engine.Res) bool {
 	//}
 
 	if obj.Bool != res.Bool {
-		return false
+		return fmt.Errorf("the Bool differs")
 	}
 	if obj.Str != res.Str {
-		return false
+		return fmt.Errorf("the Str differs")
 	}
 
 	if obj.Int != res.Int {
-		return false
+		return fmt.Errorf("the Str differs")
 	}
 	if obj.Int8 != res.Int8 {
-		return false
+		return fmt.Errorf("the Int8 differs")
 	}
 	if obj.Int16 != res.Int16 {
-		return false
+		return fmt.Errorf("the Int16 differs")
 	}
 	if obj.Int32 != res.Int32 {
-		return false
+		return fmt.Errorf("the Int32 differs")
 	}
 	if obj.Int64 != res.Int64 {
-		return false
+		return fmt.Errorf("the Int64 differs")
 	}
 
 	if obj.Uint != res.Uint {
-		return false
+		return fmt.Errorf("the Uint differs")
 	}
 	if obj.Uint8 != res.Uint8 {
-		return false
+		return fmt.Errorf("the Uint8 differs")
 	}
 	if obj.Uint16 != res.Uint16 {
-		return false
+		return fmt.Errorf("the Uint16 differs")
 	}
 	if obj.Uint32 != res.Uint32 {
-		return false
+		return fmt.Errorf("the Uint32 differs")
 	}
 	if obj.Uint64 != res.Uint64 {
-		return false
+		return fmt.Errorf("the Uint64 differs")
 	}
 
 	//if obj.Uintptr
 	if obj.Byte != res.Byte {
-		return false
+		return fmt.Errorf("the Byte differs")
 	}
 	if obj.Rune != res.Rune {
-		return false
+		return fmt.Errorf("the Rune differs")
 	}
 
 	if obj.Float32 != res.Float32 {
-		return false
+		return fmt.Errorf("the Float32 differs")
 	}
 	if obj.Float64 != res.Float64 {
-		return false
+		return fmt.Errorf("the Float64 differs")
 	}
 	if obj.Complex64 != res.Complex64 {
-		return false
+		return fmt.Errorf("the Complex64 differs")
 	}
 	if obj.Complex128 != res.Complex128 {
-		return false
+		return fmt.Errorf("the Complex128 differs")
 	}
 
 	if (obj.BoolPtr == nil) != (res.BoolPtr == nil) { // xor
-		return false
+		return fmt.Errorf("the BoolPtr differs")
 	}
 	if obj.BoolPtr != nil && res.BoolPtr != nil {
 		if *obj.BoolPtr != *res.BoolPtr { // compare
-			return false
+			return fmt.Errorf("the BoolPtr differs")
 		}
 	}
 	if (obj.StringPtr == nil) != (res.StringPtr == nil) { // xor
-		return false
+		return fmt.Errorf("the StringPtr differs")
 	}
 	if obj.StringPtr != nil && res.StringPtr != nil {
 		if *obj.StringPtr != *res.StringPtr { // compare
-			return false
+			return fmt.Errorf("the StringPtr differs")
 		}
 	}
 	if (obj.Int64Ptr == nil) != (res.Int64Ptr == nil) { // xor
-		return false
+		return fmt.Errorf("the Int64Ptr differs")
 	}
 	if obj.Int64Ptr != nil && res.Int64Ptr != nil {
 		if *obj.Int64Ptr != *res.Int64Ptr { // compare
-			return false
+			return fmt.Errorf("the Int64Ptr differs")
 		}
 	}
 	if (obj.Int8Ptr == nil) != (res.Int8Ptr == nil) { // xor
-		return false
+		return fmt.Errorf("the Int8Ptr differs")
 	}
 	if obj.Int8Ptr != nil && res.Int8Ptr != nil {
 		if *obj.Int8Ptr != *res.Int8Ptr { // compare
-			return false
+			return fmt.Errorf("the Int8Ptr differs")
 		}
 	}
 	if (obj.Uint8Ptr == nil) != (res.Uint8Ptr == nil) { // xor
-		return false
+		return fmt.Errorf("the Uint8Ptr differs")
 	}
 	if obj.Uint8Ptr != nil && res.Uint8Ptr != nil {
 		if *obj.Uint8Ptr != *res.Uint8Ptr { // compare
-			return false
+			return fmt.Errorf("the Uint8Ptr differs")
 		}
 	}
 
 	if !reflect.DeepEqual(obj.Int8PtrPtrPtr, res.Int8PtrPtrPtr) {
-		return false
+		return fmt.Errorf("the Int8PtrPtrPtr differs")
 	}
 
 	if !reflect.DeepEqual(obj.SliceString, res.SliceString) {
-		return false
+		return fmt.Errorf("the SliceString differs")
 	}
 	if !reflect.DeepEqual(obj.MapIntFloat, res.MapIntFloat) {
-		return false
+		return fmt.Errorf("the MapIntFloat differs")
 	}
 	if !reflect.DeepEqual(obj.MixedStruct, res.MixedStruct) {
-		return false
+		return fmt.Errorf("the MixedStruct differs")
 	}
 	if !reflect.DeepEqual(obj.Interface, res.Interface) {
-		return false
+		return fmt.Errorf("the Interface differs")
 	}
 
 	if obj.AnotherStr != res.AnotherStr {
-		return false
+		return fmt.Errorf("the AnotherStr differs")
 	}
 
 	if obj.ValidateBool != res.ValidateBool {
-		return false
+		return fmt.Errorf("the ValidateBool differs")
 	}
 	if obj.ValidateError != res.ValidateError {
-		return false
+		return fmt.Errorf("the ValidateError differs")
 	}
 	if obj.AlwaysGroup != res.AlwaysGroup {
-		return false
+		return fmt.Errorf("the AlwaysGroup differs")
 	}
 	if obj.SendValue != res.SendValue {
-		return false
+		return fmt.Errorf("the SendValue differs")
 	}
 
 	if obj.Comment != res.Comment {
-		return false
+		return fmt.Errorf("the Comment differs")
 	}
 
-	return true
+	return nil
 }
 
 // TestUID is the UID struct for TestRes.
@@ -401,8 +393,8 @@ func (obj *TestRes) GroupCmp(r engine.GroupableRes) error {
 // TestSends is the struct of data which is sent after a successful Apply.
 type TestSends struct {
 	// Hello is some value being sent.
-	Hello  *string
-	Answer int // some other value being sent
+	Hello  *string `lang:"hello"`
+	Answer int     `lang:"answer"` // some other value being sent
 }
 
 // Sends represents the default struct of values we can send using Send/Recv.

engine/resources/test_test.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/resources/timer.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -113,25 +113,17 @@ func (obj *TimerRes) CheckApply(apply bool) (bool, error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *TimerRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *TimerRes) Compare(r engine.Res) bool {
 	// we can only compare TimerRes to others of the same resource kind
 	res, ok := r.(*TimerRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.Interval != res.Interval {
-		return false
+		return fmt.Errorf("the Interval differs")
 	}
 
-	return true
+	return nil
 }
 
 // TimerUID is the UID struct for TimerRes.

engine/resources/user.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -30,8 +30,7 @@ import (
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
 	"github.com/purpleidea/mgmt/recwatch"
-
-	errwrap "github.com/pkg/errors"
+	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
 func init() {
@@ -153,7 +152,7 @@ func (obj *UserRes) Watch() error {
 }
 
 // CheckApply method for User resource.
-func (obj *UserRes) CheckApply(apply bool) (checkOK bool, err error) {
+func (obj *UserRes) CheckApply(apply bool) (bool, error) {
 	obj.init.Logf("CheckApply(%t)", apply)
 
 	var exists = true
@@ -274,45 +273,37 @@ func (obj *UserRes) CheckApply(apply bool) (checkOK bool, err error) {
 
 // Cmp compares two resources and returns an error if they are not equivalent.
 func (obj *UserRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
-	}
-	return nil
-}
-
-// Compare two resources and return if they are equivalent.
-func (obj *UserRes) Compare(r engine.Res) bool {
 	// we can only compare UserRes to others of the same resource kind
 	res, ok := r.(*UserRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if (obj.UID == nil) != (res.UID == nil) {
-		return false
+		return fmt.Errorf("the UID differs")
 	}
 	if obj.UID != nil && res.UID != nil {
 		if *obj.UID != *res.UID {
-			return false
+			return fmt.Errorf("the UID differs")
 		}
 	}
 	if (obj.GID == nil) != (res.GID == nil) {
-		return false
+		return fmt.Errorf("the GID differs")
 	}
 	if obj.GID != nil && res.GID != nil {
 		if *obj.GID != *res.GID {
-			return false
+			return fmt.Errorf("the GID differs")
 		}
 	}
 	if (obj.Groups == nil) != (res.Groups == nil) {
-		return false
+		return fmt.Errorf("the Group differs")
 	}
 	if obj.Groups != nil && res.Groups != nil {
 		if len(obj.Groups) != len(res.Groups) {
-			return false
+			return fmt.Errorf("the Group differs")
 		}
 		objGroups := obj.Groups
 		resGroups := res.Groups
@@ -320,22 +311,22 @@ func (obj *UserRes) Compare(r engine.Res) bool {
 		sort.Strings(resGroups)
 		for i := range objGroups {
 			if objGroups[i] != resGroups[i] {
-				return false
+				return fmt.Errorf("the Group differs at index: %d", i)
 			}
 		}
 	}
 	if (obj.HomeDir == nil) != (res.HomeDir == nil) {
-		return false
+		return fmt.Errorf("the HomeDirs differs")
 	}
 	if obj.HomeDir != nil && res.HomeDir != nil {
-		if *obj.HomeDir != *obj.HomeDir {
-			return false
+		if *obj.HomeDir != *res.HomeDir {
+			return fmt.Errorf("the HomeDir differs")
 		}
 	}
 	if obj.AllowDuplicateUID != res.AllowDuplicateUID {
-		return false
+		return fmt.Errorf("the AllowDuplicateUID differs")
 	}
-	return true
+	return nil
 }
 
 // UserUID is the UID struct for UserRes.

engine/resources/virt.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -29,11 +29,12 @@ import (
 
 	"github.com/purpleidea/mgmt/engine"
 	"github.com/purpleidea/mgmt/engine/traits"
+	engineUtil "github.com/purpleidea/mgmt/engine/util"
 	"github.com/purpleidea/mgmt/util"
+	"github.com/purpleidea/mgmt/util/errwrap"
 
 	"github.com/libvirt/libvirt-go"
 	libvirtxml "github.com/libvirt/libvirt-go-xml"
-	errwrap "github.com/pkg/errors"
 )
 
 func init() {
@@ -65,30 +66,53 @@ const (
 // VirtRes is a libvirt resource. A transient virt resource, which has its state
 // set to `shutoff` is one which does not exist. The parallel equivalent is a
 // file resource which removes a particular path.
+// TODO: some values inside here should be enum's!
 type VirtRes struct {
 	traits.Base // add the base methods without re-implementation
 	traits.Refreshable
 
 	init *engine.Init
 
-	URI        string             `lang:"uri" yaml:"uri"`             // connection uri, eg: qemu:///session
-	State      string             `lang:"state" yaml:"state"`         // running, paused, shutoff
-	Transient  bool               `lang:"transient" yaml:"transient"` // defined (false) or undefined (true)
-	CPUs       uint               `lang:"cpus" yaml:"cpus"`
-	MaxCPUs    uint               `lang:"maxcpus" yaml:"maxcpus"`
-	Memory     uint64             `lang:"memory" yaml:"memory"` // in KBytes
-	OSInit     string             `lang:"osinit" yaml:"osinit"` // init used by lxc
-	Boot       []string           `lang:"boot" yaml:"boot"`     // boot order. values: fd, hd, cdrom, network
-	Disk       []DiskDevice       `lang:"disk" yaml:"disk"`
-	CDRom      []CDRomDevice      `lang:"cdrom" yaml:"cdrom"`
-	Network    []NetworkDevice    `lang:"network" yaml:"network"`
-	Filesystem []FilesystemDevice `lang:"filesystem" yaml:"filesystem"`
-	Auth       *VirtAuth          `lang:"auth" yaml:"auth"`
+	// URI is the libvirt connection URI, eg: `qemu:///session`.
+	URI string `lang:"uri" yaml:"uri"`
+	// State is the desired vm state. Possible values include: `running`,
+	// `paused` and `shutoff`.
+	State string `lang:"state" yaml:"state"`
+	// Transient is whether the vm is defined (false) or undefined (true).
+	Transient bool `lang:"transient" yaml:"transient"`
 
-	HotCPUs bool `lang:"hotcpus" yaml:"hotcpus"` // allow hotplug of cpus?
-	// FIXME: values here should be enum's!
-	RestartOnDiverge string `lang:"restartondiverge" yaml:"restartondiverge"` // restart policy: "ignore", "ifneeded", "error"
-	RestartOnRefresh bool   `lang:"restartonrefresh" yaml:"restartonrefresh"` // restart on refresh?
+	// CPUs is the desired cpu count of the machine.
+	CPUs uint `lang:"cpus" yaml:"cpus"`
+	// MaxCPUs is the maximum number of cpus allowed in the machine. You
+	// need to set this so that on boot the `hardware` knows how many cpu
+	// `slots` it might need to make room for.
+	MaxCPUs uint `lang:"maxcpus" yaml:"maxcpus"`
+	// HotCPUs specifies whether we can hot plug and unplug cpus.
+	HotCPUs bool `lang:"hotcpus" yaml:"hotcpus"`
+	// Memory is the size in KBytes of memory to include in the machine.
+	Memory uint64 `lang:"memory" yaml:"memory"`
+
+	// OSInit is the init used by lxc.
+	OSInit string `lang:"osinit" yaml:"osinit"`
+	// Boot is the boot order. Values are `fd`, `hd`, `cdrom` and `network`.
+	Boot []string `lang:"boot" yaml:"boot"`
+	// Disk is the list of disk devices to include.
+	Disk []*DiskDevice `lang:"disk" yaml:"disk"`
+	// CdRom is the list of cdrom devices to include.
+	CDRom []*CDRomDevice `lang:"cdrom" yaml:"cdrom"`
+	// Network is the list of network devices to include.
+	Network []*NetworkDevice `lang:"network" yaml:"network"`
+	// Filesystem is the list of file system devices to include.
+	Filesystem []*FilesystemDevice `lang:"filesystem" yaml:"filesystem"`
+
+	// Auth points to the libvirt credentials to use if any are necessary.
+	Auth *VirtAuth `lang:"auth" yaml:"auth"`
+
+	// RestartOnDiverge is the restart policy, and can be: `ignore`,
+	// `ifneeded` or `error`.
+	RestartOnDiverge string `lang:"restartondiverge" yaml:"restartondiverge"`
+	// RestartOnRefresh specifies if we restart on refresh signal.
+	RestartOnRefresh bool `lang:"restartonrefresh" yaml:"restartonrefresh"`
 
 	wg                  *sync.WaitGroup
 	conn                *libvirt.Connect
@@ -107,6 +131,24 @@ type VirtAuth struct {
 	Password string `lang:"password" yaml:"password"`
 }
 
+// Cmp compares two VirtAuth structs. It errors if they are not identical.
+func (obj *VirtAuth) Cmp(auth *VirtAuth) error {
+	if (obj == nil) != (auth == nil) { // xor
+		return fmt.Errorf("the VirtAuth differs")
+	}
+	if obj == nil && auth == nil {
+		return nil
+	}
+
+	if obj.Username != auth.Username {
+		return fmt.Errorf("the Username differs")
+	}
+	if obj.Password != auth.Password {
+		return fmt.Errorf("the Password differs")
+	}
+	return nil
+}
+
 // Default returns some sensible defaults for this resource.
 func (obj *VirtRes) Default() engine.Res {
 	return &VirtRes{
@@ -138,7 +180,7 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 	var u *url.URL
 	var err error
 	if u, err = url.Parse(obj.URI); err != nil {
-		return errwrap.Wrapf(err, "%s: Parsing URI failed: %s", obj, obj.URI)
+		return errwrap.Wrapf(err, "parsing URI (`%s`) failed", obj.URI)
 	}
 	switch u.Scheme {
 	case "lxc":
@@ -149,7 +191,7 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 
 	obj.conn, err = obj.connect() // gets closed in Close method of Res API
 	if err != nil {
-		return errwrap.Wrapf(err, "%s: Connection to libvirt failed in init", obj)
+		return errwrap.Wrapf(err, "connection to libvirt failed in init")
 	}
 
 	// check for hard to change properties
@@ -157,14 +199,14 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 	if err == nil {
 		defer dom.Free()
 	} else if !isNotFound(err) {
-		return errwrap.Wrapf(err, "%s: Could not lookup on init", obj)
+		return errwrap.Wrapf(err, "could not lookup on init")
 	}
 
 	if err == nil {
 		// maxCPUs, err := dom.GetMaxVcpus()
 		i, err := dom.GetVcpusFlags(libvirt.DOMAIN_VCPU_MAXIMUM)
 		if err != nil {
-			return errwrap.Wrapf(err, "%s: Could not lookup MaxCPUs on init", obj)
+			return errwrap.Wrapf(err, "could not lookup MaxCPUs on init")
 		}
 		maxCPUs := uint(i)
 		if obj.MaxCPUs != maxCPUs { // max cpu slots is hard to change
@@ -177,11 +219,11 @@ func (obj *VirtRes) Init(init *engine.Init) error {
 		// event handlers so that we don't miss any events via race?
 		xmlDesc, err := dom.GetXMLDesc(0) // 0 means no flags
 		if err != nil {
-			return errwrap.Wrapf(err, "%s: Could not GetXMLDesc on init", obj)
+			return errwrap.Wrapf(err, "could not GetXMLDesc on init")
 		}
 		domXML := &libvirtxml.Domain{}
 		if err := domXML.Unmarshal(xmlDesc); err != nil {
-			return errwrap.Wrapf(err, "%s: Could not unmarshal XML on init", obj)
+			return errwrap.Wrapf(err, "could not unmarshal XML on init")
 		}
 
 		// guest agent: domain->devices->channel->target->state == connected?
@@ -382,22 +424,22 @@ func (obj *VirtRes) Watch() error {
 			if state == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_STATE_CONNECTED {
 				obj.guestAgentConnected = true
 				send = true
-				obj.init.Logf("Guest agent connected")
+				obj.init.Logf("guest agent connected")
 
 			} else if state == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_STATE_DISCONNECTED {
 				obj.guestAgentConnected = false
 				// ignore CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_DOMAIN_STARTED
 				// events because they just tell you that guest agent channel was added
 				if reason == libvirt.CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL {
-					obj.init.Logf("Guest agent disconnected")
+					obj.init.Logf("guest agent disconnected")
 				}
 
 			} else {
-				return fmt.Errorf("unknown %s guest agent state: %v", obj, state)
+				return fmt.Errorf("unknown guest agent state: %v", state)
 			}
 
 		case err := <-errorChan:
-			return fmt.Errorf("unknown %s libvirt error: %s", obj, err)
+			return errwrap.Wrapf(err, "unknown libvirt error")
 
 		case <-obj.init.Done: // closed by the engine to signal shutdown
 			return nil
@@ -434,7 +476,7 @@ func (obj *VirtRes) domainCreate() (*libvirt.Domain, bool, error) {
 		if err != nil {
 			return dom, false, err // returned dom is invalid
 		}
-		obj.init.Logf("Domain transient %s", state)
+		obj.init.Logf("transient domain %s", state) // log the state
 		return dom, false, nil
 	}
 
@@ -442,20 +484,20 @@ func (obj *VirtRes) domainCreate() (*libvirt.Domain, bool, error) {
 	if err != nil {
 		return dom, false, err // returned dom is invalid
 	}
-	obj.init.Logf("Domain defined")
+	obj.init.Logf("domain defined")
 
 	if obj.State == "running" {
 		if err := dom.Create(); err != nil {
 			return dom, false, err
 		}
-		obj.init.Logf("Domain started")
+		obj.init.Logf("domain started")
 	}
 
 	if obj.State == "paused" {
 		if err := dom.CreateWithFlags(libvirt.DOMAIN_START_PAUSED); err != nil {
 			return dom, false, err
 		}
-		obj.init.Logf("Domain created paused")
+		obj.init.Logf("domain created paused")
 	}
 
 	return dom, false, nil
@@ -483,7 +525,7 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 		}
 		if domInfo.State == libvirt.DOMAIN_BLOCKED {
 			// TODO: what should happen?
-			return false, fmt.Errorf("domain %s is blocked", obj.Name())
+			return false, fmt.Errorf("domain is blocked")
 		}
 		if !apply {
 			return false, nil
@@ -493,14 +535,14 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 				return false, errwrap.Wrapf(err, "domain.Resume failed")
 			}
 			checkOK = false
-			obj.init.Logf("Domain resumed")
+			obj.init.Logf("domain resumed")
 			break
 		}
 		if err := dom.Create(); err != nil {
 			return false, errwrap.Wrapf(err, "domain.Create failed")
 		}
 		checkOK = false
-		obj.init.Logf("Domain created")
+		obj.init.Logf("domain created")
 
 	case "paused":
 		if domInfo.State == libvirt.DOMAIN_PAUSED {
@@ -514,14 +556,14 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 				return false, errwrap.Wrapf(err, "domain.Suspend failed")
 			}
 			checkOK = false
-			obj.init.Logf("Domain paused")
+			obj.init.Logf("domain paused")
 			break
 		}
 		if err := dom.CreateWithFlags(libvirt.DOMAIN_START_PAUSED); err != nil {
 			return false, errwrap.Wrapf(err, "domain.CreateWithFlags failed")
 		}
 		checkOK = false
-		obj.init.Logf("Domain created paused")
+		obj.init.Logf("domain created paused")
 
 	case "shutoff":
 		if domInfo.State == libvirt.DOMAIN_SHUTOFF || domInfo.State == libvirt.DOMAIN_SHUTDOWN {
@@ -535,7 +577,7 @@ func (obj *VirtRes) stateCheckApply(apply bool, dom *libvirt.Domain) (bool, erro
 			return false, errwrap.Wrapf(err, "domain.Destroy failed")
 		}
 		checkOK = false
-		obj.init.Logf("Domain destroyed")
+		obj.init.Logf("domain destroyed")
 	}
 
 	return checkOK, nil
@@ -561,7 +603,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 		if err := dom.SetMemory(obj.Memory); err != nil {
 			return false, errwrap.Wrapf(err, "domain.SetMemory failed")
 		}
-		obj.init.Logf("Memory changed to %d", obj.Memory)
+		obj.init.Logf("memory changed to: %d", obj.Memory)
 	}
 
 	// check cpus
@@ -600,7 +642,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 				return false, errwrap.Wrapf(err, "domain.SetVcpus failed")
 			}
 			checkOK = false
-			obj.init.Logf("CPUs (hot) changed to %d", obj.CPUs)
+			obj.init.Logf("cpus (hot) changed to: %d", obj.CPUs)
 
 		case libvirt.DOMAIN_SHUTOFF, libvirt.DOMAIN_SHUTDOWN:
 			if !obj.Transient {
@@ -612,7 +654,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 					return false, errwrap.Wrapf(err, "domain.SetVcpus failed")
 				}
 				checkOK = false
-				obj.init.Logf("CPUs (cold) changed to %d", obj.CPUs)
+				obj.init.Logf("cpus (cold) changed to: %d", obj.CPUs)
 			}
 
 		default:
@@ -643,7 +685,7 @@ func (obj *VirtRes) attrCheckApply(apply bool, dom *libvirt.Domain) (bool, error
 				return false, errwrap.Wrapf(err, "domain.SetVcpus failed")
 			}
 			checkOK = false
-			obj.init.Logf("CPUs (guest) changed to %d", obj.CPUs)
+			obj.init.Logf("cpus (guest) changed to: %d", obj.CPUs)
 		}
 	}
 
@@ -667,7 +709,7 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 			return false, errwrap.Wrapf(err, "domain.GetInfo failed")
 		}
 		if domInfo.State == libvirt.DOMAIN_SHUTOFF || domInfo.State == libvirt.DOMAIN_SHUTDOWN {
-			obj.init.Logf("Shutdown")
+			obj.init.Logf("shutdown")
 			break
 		}
 
@@ -679,7 +721,7 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 			obj.processExitChan = make(chan struct{})
 			// if machine shuts down before we call this, we error;
 			// this isn't ideal, but it happened due to user error!
-			obj.init.Logf("Running shutdown")
+			obj.init.Logf("running shutdown")
 			if err := dom.Shutdown(); err != nil {
 				// FIXME: if machine is already shutdown completely, return early
 				return false, errwrap.Wrapf(err, "domain.Shutdown failed")
@@ -700,7 +742,7 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 			// https://libvirt.org/formatdomain.html#elementsEvents
 			continue
 		case <-timeout:
-			return false, fmt.Errorf("%s: didn't shutdown after %d seconds", obj, MaxShutdownDelayTimeout)
+			return false, fmt.Errorf("didn't shutdown after %d seconds", MaxShutdownDelayTimeout)
 		}
 	}
 
@@ -710,8 +752,8 @@ func (obj *VirtRes) domainShutdownSync(apply bool, dom *libvirt.Domain) (bool, e
 // CheckApply checks the resource state and applies the resource if the bool
 // input is true. It returns error info and if the state check passed or not.
 func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
-	if obj.conn == nil {
-		panic("virt: CheckApply is being called with nil connection")
+	if obj.conn == nil { // programming error?
+		return false, fmt.Errorf("got called with nil connection")
 	}
 	// if we do the restart, we must flip the flag back to false as evidence
 	var restart bool                                // do we need to do a restart?
@@ -772,7 +814,7 @@ func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
 			if err := dom.Undefine(); err != nil {
 				return false, errwrap.Wrapf(err, "domain.Undefine failed")
 			}
-			obj.init.Logf("Domain undefined")
+			obj.init.Logf("domain undefined")
 		} else {
 			domXML, err := dom.GetXMLDesc(libvirt.DOMAIN_XML_INACTIVE)
 			if err != nil {
@@ -781,7 +823,7 @@ func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
 			if _, err = obj.conn.DomainDefineXML(domXML); err != nil {
 				return false, errwrap.Wrapf(err, "conn.DomainDefineXML failed")
 			}
-			obj.init.Logf("Domain defined")
+			obj.init.Logf("domain defined")
 		}
 		checkOK = false
 	}
@@ -829,7 +871,7 @@ func (obj *VirtRes) CheckApply(apply bool) (bool, error) {
 
 	// we had to do a restart, we didn't, and we should error if it was needed
 	if obj.restartScheduled && restart == true && obj.RestartOnDiverge == "error" {
-		return false, fmt.Errorf("%s: needed restart but didn't! (RestartOnDiverge: %v)", obj, obj.RestartOnDiverge)
+		return false, fmt.Errorf("needed restart but didn't! (RestartOnDiverge: %s)", obj.RestartOnDiverge)
 	}
 
 	return checkOK, nil // w00t
@@ -959,27 +1001,6 @@ type DiskDevice struct {
 	Type   string `lang:"type" yaml:"type"`
 }
 
-// CDRomDevice represents a CDRom device that is attached to the virt machine.
-type CDRomDevice struct {
-	Source string `lang:"source" yaml:"source"`
-	Type   string `lang:"type" yaml:"type"`
-}
-
-// NetworkDevice represents a network card that is attached to the virt machine.
-type NetworkDevice struct {
-	Name string `lang:"name" yaml:"name"`
-	MAC  string `lang:"mac" yaml:"mac"`
-}
-
-// FilesystemDevice represents a filesystem that is attached to the virt
-// machine.
-type FilesystemDevice struct {
-	Access   string `lang:"access" yaml:"access"`
-	Source   string `lang:"source" yaml:"source"`
-	Target   string `lang:"target" yaml:"target"`
-	ReadOnly bool   `lang:"read_only" yaml:"read_only"`
-}
-
 // GetXML returns the XML representation of this device.
 func (obj *DiskDevice) GetXML(idx int) string {
 	source, _ := util.ExpandHome(obj.Source) // TODO: should we handle errors?
@@ -992,6 +1013,32 @@ func (obj *DiskDevice) GetXML(idx int) string {
 	return b
 }
 
+// Cmp compares two DiskDevice's and returns an error if they are not
+// equivalent.
+func (obj *DiskDevice) Cmp(dev *DiskDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the DiskDevice differs")
+	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Source != dev.Source {
+		return fmt.Errorf("the Source differs")
+	}
+	if obj.Type != dev.Type {
+		return fmt.Errorf("the Type differs")
+	}
+
+	return nil
+}
+
+// CDRomDevice represents a CDRom device that is attached to the virt machine.
+type CDRomDevice struct {
+	Source string `lang:"source" yaml:"source"`
+	Type   string `lang:"type" yaml:"type"`
+}
+
 // GetXML returns the XML representation of this device.
 func (obj *CDRomDevice) GetXML(idx int) string {
 	source, _ := util.ExpandHome(obj.Source) // TODO: should we handle errors?
@@ -1005,6 +1052,32 @@ func (obj *CDRomDevice) GetXML(idx int) string {
 	return b
 }
 
+// Cmp compares two CDRomDevice's and returns an error if they are not
+// equivalent.
+func (obj *CDRomDevice) Cmp(dev *CDRomDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the CDRomDevice differs")
+	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Source != dev.Source {
+		return fmt.Errorf("the Source differs")
+	}
+	if obj.Type != dev.Type {
+		return fmt.Errorf("the Type differs")
+	}
+
+	return nil
+}
+
+// NetworkDevice represents a network card that is attached to the virt machine.
+type NetworkDevice struct {
+	Name string `lang:"name" yaml:"name"`
+	MAC  string `lang:"mac" yaml:"mac"`
+}
+
 // GetXML returns the XML representation of this device.
 func (obj *NetworkDevice) GetXML(idx int) string {
 	if obj.MAC == "" {
@@ -1018,6 +1091,35 @@ func (obj *NetworkDevice) GetXML(idx int) string {
 	return b
 }
 
+// Cmp compares two NetworkDevice's and returns an error if they are not
+// equivalent.
+func (obj *NetworkDevice) Cmp(dev *NetworkDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the NetworkDevice differs")
+	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Name != dev.Name {
+		return fmt.Errorf("the Name differs")
+	}
+	if obj.MAC != dev.MAC {
+		return fmt.Errorf("the MAC differs")
+	}
+
+	return nil
+}
+
+// FilesystemDevice represents a filesystem that is attached to the virt
+// machine.
+type FilesystemDevice struct {
+	Access   string `lang:"access" yaml:"access"`
+	Source   string `lang:"source" yaml:"source"`
+	Target   string `lang:"target" yaml:"target"`
+	ReadOnly bool   `lang:"read_only" yaml:"read_only"`
+}
+
 // GetXML returns the XML representation of this device.
 func (obj *FilesystemDevice) GetXML(idx int) string {
 	source, _ := util.ExpandHome(obj.Source) // TODO: should we handle errors?
@@ -1036,39 +1138,61 @@ func (obj *FilesystemDevice) GetXML(idx int) string {
 	return b
 }
 
-// Cmp compares two resources and returns an error if they are not equivalent.
-func (obj *VirtRes) Cmp(r engine.Res) error {
-	if !obj.Compare(r) {
-		return fmt.Errorf("did not compare")
+// Cmp compares two FilesystemDevice's and returns an error if they are not
+// equivalent.
+func (obj *FilesystemDevice) Cmp(dev *FilesystemDevice) error {
+	if (obj == nil) != (dev == nil) { // xor
+		return fmt.Errorf("the FilesystemDevice differs")
 	}
+	if obj == nil && dev == nil {
+		return nil
+	}
+
+	if obj.Access != dev.Access {
+		return fmt.Errorf("the Access differs")
+	}
+	if obj.Source != dev.Source {
+		return fmt.Errorf("the Source differs")
+	}
+	if obj.Target != dev.Target {
+		return fmt.Errorf("the Target differs")
+	}
+	if obj.ReadOnly != dev.ReadOnly {
+		return fmt.Errorf("the ReadOnly differs")
+	}
+
 	return nil
 }
 
-// Compare two resources and return if they are equivalent.
-func (obj *VirtRes) Compare(r engine.Res) bool {
+// Cmp compares two resources and returns an error if they are not equivalent.
+func (obj *VirtRes) Cmp(r engine.Res) error {
 	// we can only compare VirtRes to others of the same resource kind
 	res, ok := r.(*VirtRes)
 	if !ok {
-		return false
+		return fmt.Errorf("not a %s", obj.Kind())
 	}
 
 	if obj.URI != res.URI {
-		return false
+		return fmt.Errorf("the URI differs")
 	}
 	if obj.State != res.State {
-		return false
+		return fmt.Errorf("the State differs")
 	}
 	if obj.Transient != res.Transient {
-		return false
+		return fmt.Errorf("the Transient differs")
 	}
+
 	if obj.CPUs != res.CPUs {
-		return false
+		return fmt.Errorf("the CPUs differ")
 	}
 	// we can't change this property while machine is running!
 	// we do need to return false, so that a new struct gets built,
 	// which will cause at least one Init() & CheckApply() to run.
 	if obj.MaxCPUs != res.MaxCPUs {
-		return false
+		return fmt.Errorf("the MaxCPUs differ")
+	}
+	if obj.HotCPUs != res.HotCPUs {
+		return fmt.Errorf("the HotCPUs differ")
 	}
 	// TODO: can we skip the compare of certain properties such as
 	// Memory because this object (but with different memory) can be
@@ -1076,26 +1200,61 @@ func (obj *VirtRes) Compare(r engine.Res) bool {
 	// We would need to run some sort of "old struct update", to get
 	// the new values, but that's easy to add.
 	if obj.Memory != res.Memory {
-		return false
+		return fmt.Errorf("the Memory differs")
 	}
-	// TODO:
-	//if obj.Boot != res.Boot {
-	//	return false
-	//}
-	//if obj.Disk != res.Disk {
-	//	return false
-	//}
-	//if obj.CDRom != res.CDRom {
-	//	return false
-	//}
-	//if obj.Network != res.Network {
-	//	return false
-	//}
-	//if obj.Filesystem != res.Filesystem {
-	//	return false
-	//}
 
-	return true
+	if obj.OSInit != res.OSInit {
+		return fmt.Errorf("the OSInit differs")
+	}
+	if err := engineUtil.StrListCmp(obj.Boot, res.Boot); err != nil {
+		return errwrap.Wrapf(err, "the Boot differs")
+	}
+
+	if len(obj.Disk) != len(res.Disk) {
+		return fmt.Errorf("the Disk length differs")
+	}
+	for i := range obj.Disk {
+		if err := obj.Disk[i].Cmp(res.Disk[i]); err != nil {
+			return errwrap.Wrapf(err, "the Disk differs")
+		}
+	}
+	if len(obj.CDRom) != len(res.CDRom) {
+		return fmt.Errorf("the CDRom length differs")
+	}
+	for i := range obj.CDRom {
+		if err := obj.CDRom[i].Cmp(res.CDRom[i]); err != nil {
+			return errwrap.Wrapf(err, "the CDRom differs")
+		}
+	}
+	if len(obj.Network) != len(res.Network) {
+		return fmt.Errorf("the Network length differs")
+	}
+	for i := range obj.Network {
+		if err := obj.Network[i].Cmp(res.Network[i]); err != nil {
+			return errwrap.Wrapf(err, "the Network differs")
+		}
+	}
+	if len(obj.Filesystem) != len(res.Filesystem) {
+		return fmt.Errorf("the Filesystem length differs")
+	}
+	for i := range obj.Filesystem {
+		if err := obj.Filesystem[i].Cmp(res.Filesystem[i]); err != nil {
+			return errwrap.Wrapf(err, "the Filesystem differs")
+		}
+	}
+
+	if err := obj.Auth.Cmp(res.Auth); err != nil {
+		return errwrap.Wrapf(err, "the Auth differs")
+	}
+
+	if obj.RestartOnDiverge != res.RestartOnDiverge {
+		return fmt.Errorf("the RestartOnDiverge differs")
+	}
+	if obj.RestartOnRefresh != res.RestartOnRefresh {
+		return fmt.Errorf("the RestartOnRefresh differs")
+	}
+
+	return nil
 }
 
 // VirtUID is the UID struct for FileRes.

engine/reverse.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -41,10 +41,14 @@ type ReversibleRes interface {
 
 	// Reversed returns the "reverse" or "reciprocal" resource. This is used
 	// to "clean" up after a previously defined resource has been removed.
-	// Interestingly, this returns the core Res interface instead of a
+	// Interestingly, this could return the core Res interface instead of a
 	// ReversibleRes, because there is no requirement that the reverse of a
 	// Res be the same kind of Res, and the reverse might not be reversible!
-	Reversed() (Res, error)
+	// However, in practice, it's nice to use some of the Reversible meta
+	// params in the built value, so keep things simple and have this be a
+	// reversible res. The Res itself doesn't have to implement Reversed()
+	// in a meaningful way, it can just return nil and it will get ignored.
+	Reversed() (ReversibleRes, error)
 }
 
 // ReversibleMeta provides some parameters specific to reversible resources.
@@ -53,6 +57,16 @@ type ReversibleMeta struct {
 	// resource.
 	Disabled bool
 
+	// Reversal specifies that the resource was built from a reversal. This
+	// must be set if the resource was built by a reversal.
+	Reversal bool
+
+	// Overwrite specifies that we should overwrite any existing stored
+	// reversible resource if one that is pending already exists. If this is
+	// false, and a resource with the same name and kind exists, then this
+	// will cause an error.
+	Overwrite bool
+
 	// TODO: add options here, including whether to reverse edges, etc...
 }
 
@@ -61,5 +75,11 @@ func (obj *ReversibleMeta) Cmp(rm *ReversibleMeta) error {
 	if obj.Disabled != rm.Disabled {
 		return fmt.Errorf("values for Disabled are different")
 	}
+	if obj.Reversal != rm.Reversal { // TODO: do we want to compare these?
+		return fmt.Errorf("values for Reversal are different")
+	}
+	if obj.Overwrite != rm.Overwrite {
+		return fmt.Errorf("values for Overwrite are different")
+	}
 	return nil
 }

engine/sendrecv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/traits/autoedge.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,7 +25,9 @@ import (
 // methods needed to support autoedges on resources. It may be used as a start
 // point to avoid re-implementing the straightforward methods.
 type Edgeable struct {
-	meta *engine.AutoEdgeMeta
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.AutoEdgeMeta
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -33,16 +35,16 @@ type Edgeable struct {
 
 // AutoEdgeMeta lets you get or set meta params for the automatic edges trait.
 func (obj *Edgeable) AutoEdgeMeta() *engine.AutoEdgeMeta {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = &engine.AutoEdgeMeta{
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = &engine.AutoEdgeMeta{
 			Disabled: false,
 		}
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetAutoEdgeMeta lets you set all of the meta params for the automatic edges
 // trait in a single call.
 func (obj *Edgeable) SetAutoEdgeMeta(meta *engine.AutoEdgeMeta) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }

engine/traits/autogroup.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -27,7 +27,9 @@ import (
 // methods needed to support autogrouping on resources. It may be used as a
 // starting point to avoid re-implementing the straightforward methods.
 type Groupable struct {
-	meta *engine.AutoGroupMeta
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.AutoGroupMeta
 
 	isGrouped bool                  // am i contained within a group?
 	grouped   []engine.GroupableRes // list of any grouped resources
@@ -39,18 +41,18 @@ type Groupable struct {
 // AutoGroupMeta lets you get or set meta params for the automatic grouping
 // trait.
 func (obj *Groupable) AutoGroupMeta() *engine.AutoGroupMeta {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = &engine.AutoGroupMeta{
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = &engine.AutoGroupMeta{
 			Disabled: false,
 		}
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetAutoGroupMeta lets you set all of the meta params for the automatic
 // grouping trait in a single call.
 func (obj *Groupable) SetAutoGroupMeta(meta *engine.AutoGroupMeta) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }
 
 // GroupCmp compares two resources and decides if they're suitable for grouping.

engine/traits/base.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/traits/kind.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -17,11 +17,21 @@
 
 package traits
 
+import (
+	"encoding/gob"
+)
+
+func init() {
+	gob.Register(&Kinded{})
+}
+
 // Kinded contains a general implementation of the properties and methods needed
 // to support the resource kind. It should be used as a starting point to avoid
 // re-implementing the straightforward kind methods.
 type Kinded struct {
-	kind string
+	// Xkind is the stored kind. It should be called `kind` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xkind string
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -29,11 +39,11 @@ type Kinded struct {
 
 // Kind returns the string representation for the kind this resource is.
 func (obj *Kinded) Kind() string {
-	return obj.kind
+	return obj.Xkind
 }
 
 // SetKind sets the kind string for this resource. It must only be set by the
 // engine.
 func (obj *Kinded) SetKind(kind string) {
-	obj.kind = kind
+	obj.Xkind = kind
 }

engine/traits/meta.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,7 +25,9 @@ import (
 // to support meta parameters. It should be used as a starting point to avoid
 // re-implementing the straightforward meta methods.
 type Meta struct {
-	meta *engine.MetaParams
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.MetaParams
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -33,14 +35,14 @@ type Meta struct {
 
 // MetaParams lets you get or set meta params for this trait.
 func (obj *Meta) MetaParams() *engine.MetaParams {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = engine.DefaultMetaParams.Copy()
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = engine.DefaultMetaParams.Copy()
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetMetaParams lets you set all of the meta params for the resource in a
 // single call.
 func (obj *Meta) SetMetaParams(meta *engine.MetaParams) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }

engine/traits/named.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -21,7 +21,9 @@ package traits
 // to support named resources. It should be used as a starting point to avoid
 // re-implementing the straightforward name methods.
 type Named struct {
-	name string
+	// Xname is the stored name. It should be called `name` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xname string
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -30,11 +32,11 @@ type Named struct {
 // Name returns the unique name this resource has. It is only unique within its
 // own kind.
 func (obj *Named) Name() string {
-	return obj.name
+	return obj.Xname
 }
 
 // SetName sets the unique name for this resource. It must only be unique within
 // its own kind.
 func (obj *Named) SetName(name string) {
-	obj.name = name
+	obj.Xname = name
 }

engine/traits/refresh.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify

engine/traits/reverse.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -25,7 +25,9 @@ import (
 // methods needed to support reversing resources. It may be used as a starting
 // point to avoid re-implementing the straightforward methods.
 type Reversible struct {
-	meta *engine.ReversibleMeta
+	// Xmeta is the stored meta. It should be called `meta` but it must be
+	// public so that the `encoding/gob` package can encode it properly.
+	Xmeta *engine.ReversibleMeta
 
 	// Bug5819 works around issue https://github.com/golang/go/issues/5819
 	Bug5819 interface{} // XXX: workaround
@@ -33,16 +35,16 @@ type Reversible struct {
 
 // ReversibleMeta lets you get or set meta params for the reversing trait.
 func (obj *Reversible) ReversibleMeta() *engine.ReversibleMeta {
-	if obj.meta == nil { // set the defaults if previously empty
-		obj.meta = &engine.ReversibleMeta{
+	if obj.Xmeta == nil { // set the defaults if previously empty
+		obj.Xmeta = &engine.ReversibleMeta{
 			Disabled: true, // by default we're disabled
 		}
 	}
-	return obj.meta
+	return obj.Xmeta
 }
 
 // SetReversibleMeta lets you set all of the meta params for the reversing trait
 // in a single call.
 func (obj *Reversible) SetReversibleMeta(meta *engine.ReversibleMeta) {
-	obj.meta = meta
+	obj.Xmeta = meta
 }

engine/traits/sendrecv.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify
@@ -33,7 +33,7 @@ type Sendable struct {
 
 // Sends returns a struct containing the defaults of the type we send. This
 // needs to be implemented (overridden) by the struct with the Sendable trait to
-// be able to send any values. The public struct field names are the keys used.
+// be able to send any values. The field struct tag names are the keys used.
 func (obj *Sendable) Sends() interface{} {
 	return nil
 }

engine/util.go

@@ -1,5 +1,5 @@
 // Mgmt
-// Copyright (C) 2013-2018+ James Shubin and the project contributors
+// Copyright (C) 2013-2019+ James Shubin and the project contributors
 // Written by James Shubin <james@shubin.ca> and the project contributors
 //
 // This program is free software: you can redistribute it and/or modify