engine: resources, lang: Set resource fields more accurately

There were some bugs about setting resource fields that were structs
with various fields. This makes things more strict and correct. Now we
check for duplicate field names earlier (duplicates due to identical
aliases) and we also don't try and set private fields, or incorrectly
set partial structs.

Most interestingly, this also cleans up all of the resources and ensures
that each one has nicer docs and a clear struct tag for fields that we
want to use in mcl. These are mandatory now, and if you're missing the
tag, then we will ignore the field.

engine/resources/augeas.go

@@ -50,24 +50,24 @@ type AugeasRes struct {
 	init *engine.Init
 
 	// File is the path to the file targeted by this resource.
-	File string `yaml:"file"`
+	File string `lang:"file" yaml:"file"`
 
 	// Lens is the lens used by this resource. If specified, mgmt
 	// will lower the augeas overhead by only loading that lens.
-	Lens string `yaml:"lens"`
+	Lens string `lang:"lens" yaml:"lens"`
 
-	// Sets is a list of changes that will be applied to the file, in the form of
+	// Sets is a list of changes that will be applied to the file, in the
-	// ["path", "value"]. mgmt will run augeas.Get() before augeas.Set(), to
+	// form of ["path", "value"]. mgmt will run augeas.Get() before
-	// prevent changing the file when it is not needed.
+	// augeas.Set(), to prevent changing the file when it is not needed.
-	Sets []*AugeasSet `yaml:"sets"`
+	Sets []*AugeasSet `lang:"sets" yaml:"sets"`
 
 	recWatcher *recwatch.RecWatcher // used to watch the changed files
 }
 
 // AugeasSet represents a key/value pair of settings to be applied.
 type AugeasSet struct {
-	Path  string `yaml:"path"`  // The relative path to the value to be changed.
+	Path  string `lang:"path" yaml:"path"`   // The relative path to the value to be changed.
-	Value string `yaml:"value"` // The value to be set on the given Path.
+	Value string `lang:"value" yaml:"value"` // The value to be set on the given Path.
 }
 
 // Cmp compares this set with another one.

engine/resources/aws_ec2.go

@@ -147,26 +147,42 @@ var AwsRegions = []string{
 // http://docs.aws.amazon.com/cli/latest/userguide/cli-config-files.html
 type AwsEc2Res struct {
 	traits.Base // add the base methods without re-implementation
+	traits.Sendable
 
 	init *engine.Init
 
-	State   string `yaml:"state"`   // state: running, stopped, terminated
+	// State must be running, stopped, or terminated.
-	Region  string `yaml:"region"`  // region must match an element of AwsRegions
+	State string `lang:"state" yaml:"state"`
-	Type    string `yaml:"type"`    // type of ec2 instance, eg: t2.micro
+
-	ImageID string `yaml:"imageid"` // imageid must be available on the chosen region
+	// Region must match one of the AwsRegions. This list is static at the
+	// moment.
+	Region string `lang:"region" yaml:"region"`
+
+	// Type of ec2 instance, eg: t2.micro for example.
+	Type string `lang:"type" yaml:"type"`
+
+	// ImageID to use, and note that it must be available on the chosen
+	// region.
+	ImageID string `lang:"imageid" yaml:"imageid"`
+
+	// WatchEndpoint is the public url of the sns endpoint, eg:
+	// http://server:12345/ for example.
+	WatchEndpoint string `lang:"watchendpoint" yaml:"watchendpoint"`
+
+	// WatchListenAddr is the local address or port that the sns listens on,
+	// eg: 10.0.0.0:23456 or 23456.
+	WatchListenAddr string `lang:"watchlistenaddr" yaml:"watchlistenaddr"`
 
-	WatchEndpoint   string `yaml:"watchendpoint"`   // the public url of the sns endpoint, eg: http://server:12345/
-	WatchListenAddr string `yaml:"watchlistenaddr"` // the local address or port that the sns listens on, eg: 10.0.0.0:23456 or 23456
 	// ErrorOnMalformedPost controls whether or not malformed HTTP post
 	// requests, that cause JSON decoder errors, will also make the engine
 	// shut down. If ErrorOnMalformedPost set to true and an error occurs,
 	// Watch() will return the error and the engine will shut down.
-	ErrorOnMalformedPost bool `yaml:"erroronmalformedpost"`
+	ErrorOnMalformedPost bool `lang:"erroronmalformedpost" yaml:"erroronmalformedpost"`
 
 	// UserData is used to run bash and cloud-init commands on first launch.
 	// See http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/user-data.html
 	// for documantation and examples.
-	UserData string `yaml:"userdata"`
+	UserData string `lang:"userdata" yaml:"userdata"`
 
 	client *ec2.EC2 // client session for AWS API calls
 

engine/resources/cron.go

@@ -75,6 +75,7 @@ func init() {
 }
 
 // CronRes is a systemd-timer cron resource.
+// TODO: If we want to have an actual `crond` resource, name it LegacyCron.
 type CronRes struct {
 	traits.Base
 	traits.Edgeable
@@ -86,46 +87,52 @@ type CronRes struct {
 	// Unit is the name of the systemd service unit. It is only necessary to
 	// set if you want to specify a service with a different name than the
 	// resource.
-	Unit string `yaml:"unit"`
+	Unit string `lang:"unit" yaml:"unit"`
+
 	// State must be 'exists' or 'absent'.
-	State string `yaml:"state"`
+	State string `lang:"state" yaml:"state"`
 
 	// Session, if true, creates the timer as the current user, rather than
-	// root. The service it points to must also be a user unit. It defaults to
+	// root. The service it points to must also be a user unit. It defaults
-	// false.
+	// to false.
-	Session bool `yaml:"session"`
+	Session bool `lang:"session" yaml:"session"`
 
 	// Trigger is the type of timer. Valid types are 'OnCalendar',
 	// 'OnActiveSec'. 'OnBootSec'. 'OnStartupSec'. 'OnUnitActiveSec', and
 	// 'OnUnitInactiveSec'. For more information see 'man systemd.timer'.
-	Trigger string `yaml:"trigger"`
+	Trigger string `lang:"trigger" yaml:"trigger"`
+
 	// Time must be used with all triggers. For 'OnCalendar', it must be in
 	// the format defined in 'man systemd-time' under the heading 'Calendar
 	// Events'. For all other triggers, time should be a valid time span as
 	// defined in 'man systemd-time'
-	Time string `yaml:"time"`
+	Time string `lang:"time" yaml:"time"`
 
 	// AccuracySec is the accuracy of the timer in systemd-time time span
 	// format. It defaults to one minute.
-	AccuracySec string `yaml:"accuracysec"`
+	AccuracySec string `lang:"accuracysec" yaml:"accuracysec"`
+
 	// RandomizedDelaySec delays the timer by a randomly selected, evenly
-	// distributed amount of time between 0 and the specified time value. The
+	// distributed amount of time between 0 and the specified time value.
-	// value must be a valid systemd-time time span.
+	// The value must be a valid systemd-time time span.
-	RandomizedDelaySec string `yaml:"randomizeddelaysec"`
+	RandomizedDelaySec string `lang:"randomizeddelaysec" yaml:"randomizeddelaysec"`
 
 	// Persistent, if true, means the time when the service unit was last
 	// triggered is stored on disk. When the timer is activated, the service
-	// unit is triggered immediately if it would have been triggered at least
+	// unit is triggered immediately if it would have been triggered at
-	// once during the time when the timer was inactive. It defaults to false.
+	// least once during the time when the timer was inactive. It defaults
-	Persistent bool `yaml:"persistent"`
+	// to false.
+	Persistent bool `lang:"persistent" yaml:"persistent"`
+
 	// WakeSystem, if true, will cause the system to resume from suspend,
-	// should it be suspended and if the system supports this. It defaults to
+	// should it be suspended and if the system supports this. It defaults
-	// false.
+	// to false.
-	WakeSystem bool `yaml:"wakesystem"`
+	WakeSystem bool `lang:"wakesystem" yaml:"wakesystem"`
-	// RemainAfterElapse, if true, means an elapsed timer will stay loaded, and
+
-	// its state remains queriable. If false, an elapsed timer unit that cannot
+	// RemainAfterElapse, if true, means an elapsed timer will stay loaded,
-	// elapse anymore is unloaded. It defaults to true.
+	// and its state remains queriable. If false, an elapsed timer unit that
-	RemainAfterElapse bool `yaml:"remainafterelapse"`
+	// cannot elapse anymore is unloaded. It defaults to true.
+	RemainAfterElapse bool `lang:"remainafterelapse" yaml:"remainafterelapse"`
 
 	file       *FileRes             // nested file resource
 	recWatcher *recwatch.RecWatcher // recwatcher for nested file

engine/resources/docker_container.go

@@ -65,22 +65,27 @@ type DockerContainerRes struct {
 	traits.Edgeable
 
 	// State of the container must be running, stopped, or removed.
-	State string `yaml:"state"`
+	State string `lang:"state" yaml:"state"`
+
 	// Image is a docker image, or image:tag.
-	Image string `yaml:"image"`
+	Image string `lang:"image" yaml:"image"`
+
 	// Cmd is a command, or list of commands to run on the container.
-	Cmd []string `yaml:"cmd"`
+	Cmd []string `lang:"cmd" yaml:"cmd"`
+
 	// Env is a list of environment variables. E.g. ["VAR=val",].
-	Env []string `yaml:"env"`
+	Env []string `lang:"env" yaml:"env"`
+
 	// Ports is a map of port bindings. E.g. {"tcp" => {80 => 8080},}.
-	Ports map[string]map[int64]int64 `yaml:"ports"`
+	Ports map[string]map[int64]int64 `lang:"ports" yaml:"ports"`
+
 	// APIVersion allows you to override the host's default client API
 	// version.
-	APIVersion string `yaml:"apiversion"`
+	APIVersion string `lang:"apiversion" yaml:"apiversion"`
 
 	// Force, if true, this will destroy and redeploy the container if the
 	// image is incorrect.
-	Force bool `yaml:"force"`
+	Force bool `lang:"force" yaml:"force"`
 
 	client *client.Client // docker api client
 

engine/resources/docker_image.go

@@ -56,10 +56,11 @@ type DockerImageRes struct {
 	traits.Edgeable
 
 	// State of the image must be exists or absent.
-	State string `yaml:"state"`
+	State string `lang:"state" yaml:"state"`
+
 	// APIVersion allows you to override the host's default client API
 	// version.
-	APIVersion string `yaml:"apiversion"`
+	APIVersion string `lang:"apiversion" yaml:"apiversion"`
 
 	image  string         // full image:tag format
 	client *client.Client // docker api client

engine/resources/exec.go

@@ -49,51 +49,61 @@ type ExecRes struct {
 	init *engine.Init
 
 	// Cmd is the command to run. If this is not specified, we use the name.
-	Cmd string `yaml:"cmd"`
+	Cmd string `lang:"cmd" 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"`
+	Args []string `lang:"args" yaml:"args"`
+
 	// Cwd 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"`
+	Cwd string `lang:"cwd" 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"`
+	Shell string `lang:"shell" 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"`
+	Timeout uint64 `lang:"timeout" yaml:"timeout"`
+
 	// Env allows the user to specify environment variables for script
 	// execution. These are taken using a map of format of VAR_NAME -> value.
-	Env map[string]string `yaml:"env"`
+	Env map[string]string `lang:"env" yaml:"env"`
 
-	// Watch is the command to run to detect event changes. Each line of
+	// WatchCmd 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"`
+	WatchCmd string `lang:"watchcmd" yaml:"watchcmd"`
+
 	// WatchCwd is the Cwd for the WatchCmd. See the docs for Cwd.
-	WatchCwd string `yaml:"watchcwd"`
+	WatchCwd string `lang:"watchcwd" yaml:"watchcwd"`
+
 	// WatchShell is the Shell for the WatchCmd. See the docs for Shell.
-	WatchShell string `yaml:"watchshell"`
+	WatchShell string `lang:"watchshell" 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"`
+	IfCmd string `lang:"ifcmd" yaml:"ifcmd"`
+
 	// IfCwd is the Cwd for the IfCmd. See the docs for Cwd.
-	IfCwd string `yaml:"ifcwd"`
+	IfCwd string `lang:"ifcwd" yaml:"ifcwd"`
+
 	// IfShell is the Shell for the IfCmd. See the docs for Shell.
-	IfShell string `yaml:"ifshell"`
+	IfShell string `lang:"ifshell" 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"`
+	User string `lang:"user" 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"`
+	Group string `lang:"group" yaml:"group"`
 
 	output *string // all cmd output, read only, do not set!
 	stdout *string // the cmd stdout, read only, do not set!

engine/resources/file.go

@@ -108,9 +108,14 @@ type FileRes struct {
 	// Path, which defaults to the name if not specified, represents the
 	// destination path for the file or directory being managed. It must be
 	// an absolute path, and as a result must start with a slash.
-	Path     string `lang:"path" yaml:"path"`
+	Path string `lang:"path" yaml:"path"`
-	Dirname  string `lang:"dirname" yaml:"dirname"`   // override the path dirname
+
-	Basename string `lang:"basename" yaml:"basename"` // override the path basename
+	// Dirname is used to override the path dirname. (The directory
+	// portion.)
+	Dirname string `lang:"dirname" yaml:"dirname"`
+
+	// Basename is used to override the path basename. (The file portion.)
+	Basename string `lang:"basename" yaml:"basename"`
 
 	// State specifies the desired state of the file. It can be either
 	// `exists` or `absent`. If you do not specify this, we will not be able
@@ -123,6 +128,7 @@ type FileRes struct {
 	// left undefined. It cannot be combined with the Source or Fragments
 	// parameters.
 	Content *string `lang:"content" yaml:"content"`
+
 	// Source specifies the source contents for the file resource. It cannot
 	// be combined with the Content or Fragments parameters. It must be an
 	// absolute path, and it can point to a file or a directory. If it
@@ -139,6 +145,7 @@ type FileRes struct {
 	// combined with the Purge option too, then any unmanaged file in this
 	// dir will be removed.
 	Source string `lang:"source" yaml:"source"`
+
 	// Fragments specifies that the file is built from a list of individual
 	// files. If one of the files is a directory, then the list of files in
 	// that directory are the fragments to combine. Multiple of these can be
@@ -156,14 +163,25 @@ type FileRes struct {
 	// Owner specifies the file owner. You can specify either the string
 	// name, or a string representation of the owner integer uid.
 	Owner string `lang:"owner" yaml:"owner"`
+
 	// Group specifies the file group. You can specify either the string
 	// name, or a string representation of the group integer gid.
 	Group string `lang:"group" yaml:"group"`
+
 	// Mode is the mode of the file as a string representation of the octal
 	// form or symbolic form.
-	Mode    string `lang:"mode" yaml:"mode"`
+	Mode string `lang:"mode" yaml:"mode"`
-	Recurse bool   `lang:"recurse" yaml:"recurse"`
+
-	Force   bool   `lang:"force" yaml:"force"`
+	// Recurse specifies if you want to work recursively on the resource. It
+	// is used when copying a source directory, or to determine if a watch
+	// should be recursive or not.
+	// FIXME: There are some unimplemented cases where we should look at it.
+	Recurse bool `lang:"recurse" yaml:"recurse"`
+
+	// Force must be set if we want to perform an unusual operation, such as
+	// changing a file into a directory or vice-versa.
+	Force bool `lang:"force" yaml:"force"`
+
 	// Purge specifies that when true, any unmanaged file in this file
 	// directory will be removed. As a result, this file resource must be a
 	// directory. This isn't particularly meaningful if you don't also set

engine/resources/group.go

@@ -45,8 +45,11 @@ type GroupRes struct {
 
 	init *engine.Init
 
-	State string  `yaml:"state"` // state: exists, absent
+	// State is `exists` or `absent`.
-	GID   *uint32 `yaml:"gid"`   // the group's gid
+	State string `lang:"state" yaml:"state"`
+
+	// GID is the group's gid.
+	GID *uint32 `lang:"gid" yaml:"gid"`
 
 	recWatcher *recwatch.RecWatcher
 }

engine/resources/hostname.go

@@ -46,28 +46,32 @@ const (
 var ErrResourceInsufficientParameters = errors.New("insufficient parameters for this resource")
 
 // HostnameRes is a resource that allows setting and watching the hostname.
-//
-// StaticHostname is the one configured in /etc/hostname or a similar file. It
-// is chosen by the local user. It is not always in sync with the current host
-// name as returned by the gethostname() system call.
-//
-// TransientHostname is the one configured via the kernel's sethostbyname(). It
-// can be different from the static hostname in case DHCP or mDNS have been
-// configured to change the name based on network information.
-//
-// PrettyHostname is a free-form UTF8 host name for presentation to the user.
-//
-// Hostname is the fallback value for all 3 fields above, if only Hostname is
-// specified, it will set all 3 fields to this value.
 type HostnameRes struct {
 	traits.Base // add the base methods without re-implementation
 
 	init *engine.Init
 
-	Hostname          string `yaml:"hostname"`
+	// Hostname specifies the hostname we want to set in all of the places
-	PrettyHostname    string `yaml:"pretty_hostname"`
+	// that it's possible. This is the fallback value for all the three
-	StaticHostname    string `yaml:"static_hostname"`
+	// fields below. If only this Hostname field is specified, this will set
-	TransientHostname string `yaml:"transient_hostname"`
+	// all tree fields (PrettyHostname, StaticHostname, TransientHostname)
+	// to this value.
+	Hostname string `lang:"hostname" yaml:"hostname"`
+
+	// PrettyHostname is a free-form UTF8 host name for presentation to the
+	// user.
+	PrettyHostname string `lang:"pretty_hostname" yaml:"pretty_hostname"`
+
+	// StaticHostname is the one configured in /etc/hostname or a similar
+	// file. It is chosen by the local user. It is not always in sync with
+	// the current host name as returned by the gethostname() system call.
+	StaticHostname string `lang:"static_hostname" yaml:"static_hostname"`
+
+	// TransientHostname is the one configured via the kernel's
+	// sethostbyname(). It can be different from the static hostname in case
+	// DHCP or mDNS have been configured to change the name based on network
+	// information.
+	TransientHostname string `lang:"transient_hostname" yaml:"transient_hostname"`
 
 	conn *dbus.Conn
 }

engine/resources/mount.go

@@ -109,13 +109,24 @@ type MountRes struct {
 
 	init *engine.Init
 
-	// State must be exists ot absent. If absent, remaining fields are ignored.
+	// State must be exists or absent. If absent, remaining fields are
-	State   string            `yaml:"state"`
+	// ignored.
-	Device  string            `yaml:"device"`  // location of the device or image
+	State string `lang:"state" yaml:"state"`
-	Type    string            `yaml:"type"`    // the type of filesystem
+
-	Options map[string]string `yaml:"options"` // mount options
+	// Device is the location of the device or image.
-	Freq    int               `yaml:"freq"`    // dump frequency
+	Device string `lang:"device" yaml:"device"`
-	PassNo  int               `yaml:"passno"`  // verification order
+
+	// Type of the filesystem.
+	Type string `lang:"type" yaml:"type"`
+
+	// Options are mount options.
+	Options map[string]string `lang:"options" yaml:"options"`
+
+	// Freq is the dump frequency.
+	Freq int `lang:"freq" yaml:"freq"`
+
+	// PassNo is the verification order.
+	PassNo int `lang:"passno" yaml:"passno"`
 
 	mount *fstab.Mount // struct representing the mount
 }

engine/resources/msg.go

@@ -40,11 +40,17 @@ type MsgRes struct {
 
 	init *engine.Init
 
-	Body           string            `yaml:"body"`
+	Body     string            `lang:"body" yaml:"body"`
-	Priority       string            `yaml:"priority"`
+	Priority string            `lang:"priority" yaml:"priority"`
-	Fields         map[string]string `yaml:"fields"`
+	Fields   map[string]string `lang:"fields" yaml:"fields"`
-	Journal        bool              `yaml:"journal"` // enable systemd journal output
+
-	Syslog         bool              `yaml:"syslog"`  // enable syslog output
+	// Journal should be true to enable systemd journaled (journald) output.
+	Journal bool `lang:"journal" yaml:"journal"`
+
+	// Syslog should be true to enable traditional syslog output. This is
+	// probably going to somewhere in `/var/log/` on your filesystem.
+	Syslog bool `lang:"syslog" yaml:"syslog"`
+
 	logStateOK     bool
 	journalStateOK bool
 	syslogStateOK  bool

engine/resources/nspawn.go

@@ -58,7 +58,10 @@ type NspawnRes struct {
 
 	init *engine.Init
 
-	State string `yaml:"state"`
+	// State specifies the desired state for this resource. This must be
+	// either `running` or `stopped`.
+	State string `lang:"state" yaml:"state"`
+
 	// We're using the svc resource to start and stop the machine because
 	// that's what machinectl does. We're not using svc.Watch because then we
 	// would have two watches potentially racing each other and producing

engine/resources/password.go

@@ -53,10 +53,19 @@ type PasswordRes struct {
 
 	init *engine.Init
 
+	// Length is the number of characters to return.
 	// FIXME: is uint16 too big?
-	Length        uint16 `yaml:"length"` // number of characters to return
+	Length uint16 `lang:"length" yaml:"length"`
-	Saved         bool   // this caches the password in the clear locally
+
-	CheckRecovery bool   // recovery from integrity checks by re-generating
+	// Saved caches the password in the clear locally.
+	Saved bool
+
+	// CheckRecovery specifies that we should recover from, regenerate, and
+	// carry on casually without erroring the resource if the "check"
+	// facility fails. This can happen when loading a saved password from
+	// disk which is not of the expected length. In this case, we'd discard
+	// the old saved password and create a new one without erroring.
+	CheckRecovery bool
 
 	path       string // the path to local storage
 	recWatcher *recwatch.RecWatcher

engine/resources/pippet.go

@@ -52,15 +52,17 @@ type PippetRes struct {
 	// from a module. The Puppet installation local to the mgmt agent
 	// machine must be able recognize it. It has to be a native type though,
 	// as opposed to defined types from your Puppet manifest code.
-	Type string `yaml:"type" json:"type"`
+	Type string `lang:"type" yaml:"type" json:"type"`
+
 	// Title is used by Puppet as the resource title. Puppet will often
 	// assign special meaning to the title, e.g. use it as the path for a
 	// file resource, or the name of a package.
-	Title string `yaml:"title" json:"title"`
+	Title string `lang:"title" yaml:"title" json:"title"`
+
 	// Params is expected to be a hash in YAML format, pairing resource
 	// parameter names with their respective values, e.g. { ensure: present
 	// }
-	Params string `yaml:"params" json:"params"`
+	Params string `lang:"params" yaml:"params" json:"params"`
 
 	runner *pippetReceiver
 }

engine/resources/pkg.go

@@ -56,10 +56,25 @@ type PkgRes struct {
 
 	init *engine.Init
 
-	State            string `yaml:"state"`            // state: installed, uninstalled, newest, <version>
+	// State determines if we want to install or uninstall the package, and
-	AllowUntrusted   bool   `yaml:"allowuntrusted"`   // allow untrusted packages to be installed?
+	// what version we want to pin if any. Valid values include: installed,
-	AllowNonFree     bool   `yaml:"allownonfree"`     // allow nonfree packages to be found?
+	// uninstalled, newest, and `version`, where you just put the raw
-	AllowUnsupported bool   `yaml:"allowunsupported"` // allow unsupported packages to be found?
+	// version string desired.
+	State string `lang:"state" yaml:"state"`
+
+	// AllowUntrusted specifies if we want to allow untrusted packages to be
+	// installed. Please see the PackageKit documentation for more
+	// information.
+	AllowUntrusted bool `lang:"allowuntrusted" yaml:"allowuntrusted"`
+
+	// AllowNonFree specifies if we want to allow nonfree packages to be
+	// found? Please see the PackageKit documentation for more information.
+	AllowNonFree bool `lang:"allownonfree" yaml:"allownonfree"`
+
+	// AllowUnsupported specifies if we want to unsupported packages to be
+	// found? Please see the PackageKit documentation for more information.
+	AllowUnsupported bool `lang:"allowunsupported" yaml:"allowunsupported"`
+
 	//bus              *packagekit.Conn    // pk bus connection
 	fileList []string // FIXME: update if pkg changes
 }

engine/resources/svc.go

@@ -49,9 +49,17 @@ type SvcRes struct {
 
 	init *engine.Init
 
-	State   string `yaml:"state"`   // state: running, stopped, undefined
+	// State is the desired state for this resource. Valid values include:
-	Startup string `yaml:"startup"` // enabled, disabled, undefined
+	// running, stopped, and undefined (empty string).
-	Session bool   `yaml:"session"` // user session (true) or system?
+	State string `lang:"state" yaml:"state"`
+
+	// Startup specifies what should happen on startup. Values can be:
+	// enabled, disabled, and undefined (empty string).
+	Startup string `lang:"startup" yaml:"startup"`
+
+	// Session specifies if this is for a system service (false) or a user
+	// session specific service (true).
+	Session bool `lang:"session" yaml:"session"` // user session (true) or system?
 }
 
 // Default returns some sensible defaults for this resource.

engine/resources/test.go

@@ -55,7 +55,7 @@ type TestRes struct {
 	Uint32 uint32 `lang:"uint32" yaml:"uint32"`
 	Uint64 uint64 `lang:"uint64" yaml:"uint64"`
 
-	//Uintptr uintptr `yaml:"uintptr"`
+	//Uintptr uintptr `lang:"uintptr" yaml:"uintptr"`
 	Byte byte `lang:"byte" yaml:"byte"` // alias for uint8
 	Rune rune `lang:"rune" yaml:"rune"` // alias for int32, represents a Unicode code point
 
@@ -76,10 +76,11 @@ type TestRes struct {
 	SliceString []string          `lang:"slicestring" yaml:"slicestring"`
 	MapIntFloat map[int64]float64 `lang:"mapintfloat" yaml:"mapintfloat"`
 	MixedStruct struct {
-		somebool  bool
+		SomeBool         bool    `lang:"somebool" yaml:"somebool"`
-		somestr   string
+		SomeStr          string  `lang:"somestr" yaml:"somestr"`
-		someint   int64
+		SomeInt          int64   `lang:"someint" yaml:"someint"`
-		somefloat float64
+		SomeFloat        float64 `lang:"somefloat" yaml:"somefloat"`
+		somePrivatefield string
 	} `lang:"mixedstruct" yaml:"mixedstruct"`
 	Interface interface{} `lang:"interface" yaml:"interface"`
 
@@ -394,8 +395,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 `lang:"hello"`
+	Hello  *string `lang:"hello" yaml:"hello"`
-	Answer int     `lang:"answer"` // some other value being sent
+	Answer int     `lang:"answer" yaml:"answer"` // some other value being sent
 }
 
 // Sends represents the default struct of values we can send using Send/Recv.

engine/resources/timer.go

@@ -38,7 +38,8 @@ type TimerRes struct {
 
 	init *engine.Init
 
-	Interval uint32 `yaml:"interval"` // interval between runs in seconds
+	// Interval between runs in seconds.
+	Interval uint32 `lang:"interval" yaml:"interval"`
 
 	ticker *time.Ticker
 }

engine/resources/user.go

@@ -47,13 +47,29 @@ type UserRes struct {
 
 	init *engine.Init
 
-	State             string   `yaml:"state"`             // state: exists, absent
+	// State is either exists or absent.
-	UID               *uint32  `yaml:"uid"`               // uid must be unique unless AllowDuplicateUID is true
+	State string `lang:"state" yaml:"state"`
-	GID               *uint32  `yaml:"gid"`               // gid of the user's primary group
+
-	Group             *string  `yaml:"group"`             // name of the user's primary group
+	// UID specifies the usually unique user ID. It must be unique unless
-	Groups            []string `yaml:"groups"`            // list of supplemental groups
+	// AllowDuplicateUID is true.
-	HomeDir           *string  `yaml:"homedir"`           // path to the user's home directory
+	UID *uint32 `lang:"uid" yaml:"uid"`
-	AllowDuplicateUID bool     `yaml:"allowduplicateuid"` // allow duplicate uid
+
+	// GID of the user's primary group.
+	GID *uint32 `lang:"gid" yaml:"gid"`
+
+	// Group is the name of the user's primary group.
+	Group *string `lang:"group" yaml:"group"`
+
+	// Groups are a list of supplemental groups.
+	Groups []string `lang:"groups" yaml:"groups"`
+
+	// HomeDir is the path to the user's home directory.
+	HomeDir *string `lang:"homedir" yaml:"homedir"`
+
+	// AllowDuplicateUID is needed for a UID to be non-unique. This is rare
+	// but happens if you want more than one username to access the
+	// resources of the same UID. See the --non-unique flag in `useradd`.
+	AllowDuplicateUID bool `lang:"allowduplicateuid" yaml:"allowduplicateuid"`
 
 	recWatcher *recwatch.RecWatcher
 }

engine/util/util.go

@@ -252,87 +252,29 @@ func LangFieldNameToStructFieldName(kind string) (map[string]string, error) {
 	return mapping, nil // lang field name -> field name
 }
 
-// StructKindToFieldNameTypeMap returns a map from field name to expected type
+// LangFieldNameToStructType returns the mapping from lang (AST) field names,
-// in the lang type system.
+// and the expected type in our type system for each.
-func StructKindToFieldNameTypeMap(kind string) (map[string]*types.Type, error) {
+func LangFieldNameToStructType(kind string) (map[string]*types.Type, error) {
 	res, err := engine.NewResource(kind)
 	if err != nil {
 		return nil, err
 	}
 
-	sv := reflect.ValueOf(res).Elem() // pointer to struct, then struct
+	gtyp := reflect.TypeOf(res)
-	if k := sv.Kind(); k != reflect.Struct {
+	st, err := types.ResTypeOf(gtyp)
-		return nil, fmt.Errorf("expected struct, got: %s", k)
-	}
-
-	result := make(map[string]*types.Type)
-
-	st := reflect.TypeOf(res).Elem() // pointer to struct, then struct
-	for i := 0; i < st.NumField(); i++ {
-		field := st.Field(i)
-		name := field.Name
-		// TODO: in future, skip over fields that don't have a `lang` tag
-		//if name == "Base" { // TODO: hack!!!
-		//	continue
-		//}
-
-		// Skip unexported fields. These should never be mapped golang<->mcl.
-		//if field.PkgPath != "" { // pre-golang 1.17
-		if !field.IsExported() {
-			continue
-		}
-
-		typ, err := types.TypeOf(field.Type)
-		// some types (eg complex64) aren't convertible, so skip for now...
-		if err != nil {
-			continue
-			//return nil, errwrap.Wrapf(err, "could not identify type of field `%s`", name)
-		}
-		result[name] = typ
-	}
-
-	return result, nil
-}
-
-// LangFieldNameToStructType returns the mapping from lang (AST) field names,
-// and the expected type in our type system for each.
-func LangFieldNameToStructType(kind string) (map[string]*types.Type, error) {
-	// returns a mapping between fieldName and expected *types.Type
-	fieldNameTypMap, err := StructKindToFieldNameTypeMap(kind)
-	if err != nil {
-		return nil, errwrap.Wrapf(err, "could not determine types for `%s` resource", kind)
-	}
-
-	mapping, err := LangFieldNameToStructFieldName(kind)
 	if err != nil {
 		return nil, err
 	}
-
+	if st == nil {
-	// transform from field name to tag name
+		return nil, fmt.Errorf("got empty type")
-	typMap := make(map[string]*types.Type)
-	for name, typ := range fieldNameTypMap {
-		if strings.Title(name) != name {
-			continue // skip private fields
-		}
-
-		found := false
-		for k, v := range mapping {
-			if v != name {
-				continue
-			}
-			// found
-			if found { // previously found!
-				return nil, fmt.Errorf("duplicate mapping for: %s", name)
-			}
-			typMap[k] = typ
-			found = true // :)
-		}
-		if !found {
-			return nil, fmt.Errorf("could not find mapping for: %s", name)
-		}
 	}
+	if st.Kind != types.KindStruct {
+		return nil, fmt.Errorf("not a struct kind")
+	}
+	// unpack the top-level struct, it should have the field names matching
+	// the parameters of the struct.
 
-	return typMap, nil
+	return st.Map, nil
 }
 
 // GetUID returns the UID of an user. It supports an UID or an username. Caller

engine/util/util_test.go

@@ -170,8 +170,8 @@ func TestStructTagToFieldName2(t *testing.T) {
 }
 
 type testEngineRes struct {
-	PublicProp1  string
+	PublicProp1  string                      `lang:"PublicProp1" yaml:"PublicProp1"`
-	PublicProp2  map[string][]map[string]int
+	PublicProp2  map[string][]map[string]int `lang:"PublicProp2" yaml:"PublicProp2"`
 	privateProp1 bool
 	privateProp2 []int
 }
@@ -204,11 +204,11 @@ func (t *testEngineRes) Validate() error { return nil }
 
 func (t *testEngineRes) Watch(context.Context) error { return nil }
 
-func TestStructKindToFieldNameTypeMap(t *testing.T) {
+func TestLangFieldNameToStructType(t *testing.T) {
 	k := "test-kind"
 
 	engine.RegisterResource(k, func() engine.Res { return &testEngineRes{} })
-	res, err := StructKindToFieldNameTypeMap(k)
+	res, err := LangFieldNameToStructType(k)
 
 	expected := map[string]*types.Type{
 		"PublicProp1": types.TypeStr,

lang/ast/structs.go

@@ -721,7 +721,7 @@ func (obj *StmtRes) resource(resName string) (engine.Res, error) {
 		}
 
 		// is expr type compatible with expected field type?
-		t, err := types.TypeOf(tf.Type)
+		t, err := types.ResTypeOf(tf.Type)
 		if err != nil {
 			return nil, errwrap.Wrapf(err, "resource field `%s` has no compatible type", x.Field)
 		}

lang/interpret_test/TestAstFunc2/structlookup2.txtar

@@ -0,0 +1,22 @@
+-- main.mcl --
+$st0 struct{x str} = struct{x => "hello",}
+test structlookup($st0, "x") {}
+
+$st1 = struct{y => "world",}
+test structlookup($st1, "y") {}
+
+$st2 = struct{x => true, y => 42, z => "hello world",}
+test structlookup($st2, "z") {}
+test "foo" {
+	mixedstruct => struct{
+		somebool => true,
+		somestr => "hi",
+		someint => 42,
+		somefloat => 1.0,
+	},
+}
+-- OUTPUT --
+Vertex: test[foo]
+Vertex: test[hello world]
+Vertex: test[hello]
+Vertex: test[world]

lang/types/type.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"strings"
 
+	"github.com/purpleidea/mgmt/util"
 	"github.com/purpleidea/mgmt/util/errwrap"
 )
 
@@ -81,6 +82,77 @@ type Type struct {
 // Type of KindList, and KindFunc names the arguments of a func sequentially.
 // The lossy inverse of this is Reflect.
 func TypeOf(t reflect.Type) (*Type, error) {
+	opts := []TypeOfOption{
+		StructTagOpt(StructTag),
+		StrictStructTagOpt(false),
+		SkipBadStructFieldsOpt(false),
+	}
+	return ConfigurableTypeOf(t, opts...)
+}
+
+// ResTypeOf is almost identical to TypeOf, except it behaves slightly
+// differently so that it can return what is needed for resources.
+func ResTypeOf(t reflect.Type) (*Type, error) {
+	opts := []TypeOfOption{
+		StructTagOpt(StructTag),
+		StrictStructTagOpt(true),
+		SkipBadStructFieldsOpt(true),
+	}
+	return ConfigurableTypeOf(t, opts...)
+}
+
+// TypeOfOption is a type that can be used to configure the ConfigurableTypeOf
+// function.
+type TypeOfOption func(*typeOfOptions)
+
+// typeOfOptions represents the different possible configurable options.
+type typeOfOptions struct {
+	structTag           string
+	strictStructTag     bool
+	skipBadStructFields bool
+	// TODO: add more options
+}
+
+// StructTagOpt specifies whether we should skip over struct fields that errored
+// when we tried to find their type. This is used by ResTypeOf.
+func StructTagOpt(structTag string) TypeOfOption {
+	return func(opt *typeOfOptions) {
+		opt.structTag = structTag
+	}
+}
+
+// StrictStructTagOpt specifies whether we require that a struct tag be present
+// to be able to use the field. If false, then the field is skipped if it is
+// missing a struct tag.
+func StrictStructTagOpt(strictStructTag bool) TypeOfOption {
+	return func(opt *typeOfOptions) {
+		opt.strictStructTag = strictStructTag
+	}
+}
+
+// SkipBadStructFieldsOpt specifies whether we should skip over struct fields
+// that errored when we tried to find their type. This is used by ResTypeOf.
+func SkipBadStructFieldsOpt(skipBadStructFields bool) TypeOfOption {
+	return func(opt *typeOfOptions) {
+		opt.skipBadStructFields = skipBadStructFields
+	}
+}
+
+// ConfigurableTypeOf is a configurable version of the TypeOf function to avoid
+// repeating code for the different variants of it that we want.
+func ConfigurableTypeOf(t reflect.Type, opts ...TypeOfOption) (*Type, error) {
+	options := &typeOfOptions{ // default options
+		structTag:           "",
+		strictStructTag:     false,
+		skipBadStructFields: false,
+	}
+	for _, optionFunc := range opts { // apply the options
+		optionFunc(options)
+	}
+	if options.strictStructTag && options.structTag == "" {
+		return nil, fmt.Errorf("strict struct tag is set and struct tag is empty")
+	}
+
 	typ := t
 	kind := typ.Kind()
 	for kind == reflect.Ptr {
@@ -113,7 +185,7 @@ func TypeOf(t reflect.Type) (*Type, error) {
 		}, nil
 
 	case reflect.Array, reflect.Slice:
-		val, err := TypeOf(typ.Elem())
+		val, err := ConfigurableTypeOf(typ.Elem(), opts...)
 		if err != nil {
 			return nil, err
 		}
@@ -124,11 +196,11 @@ func TypeOf(t reflect.Type) (*Type, error) {
 		}, nil
 
 	case reflect.Map:
-		key, err := TypeOf(typ.Key()) // Key returns a map type's key type.
+		key, err := ConfigurableTypeOf(typ.Key(), opts...) // Key returns a map type's key type.
 		if err != nil {
 			return nil, err
 		}
-		val, err := TypeOf(typ.Elem()) // Elem returns a type's element type.
+		val, err := ConfigurableTypeOf(typ.Elem(), opts...) // Elem returns a type's element type.
 		if err != nil {
 			return nil, err
 		}
@@ -145,17 +217,27 @@ func TypeOf(t reflect.Type) (*Type, error) {
 
 		for i := 0; i < typ.NumField(); i++ {
 			field := typ.Field(i)
-			tt, err := TypeOf(field.Type)
+			tt, err := ConfigurableTypeOf(field.Type, opts...)
 			if err != nil {
+				if options.skipBadStructFields {
+					continue // skip over bad fields!
+				}
 				return nil, err
 			}
 			// TODO: should we skip over fields with field.Anonymous ?
 
-			// TODO: make struct field name lookup consistent with a helper function
 			// if struct field has a `lang:""` tag, use that instead of the struct field name
 			fieldName := field.Name
-			if alias, ok := field.Tag.Lookup(StructTag); ok {
+			if options.structTag != "" {
-				fieldName = alias
+				if alias, ok := field.Tag.Lookup(options.structTag); ok {
+					fieldName = alias
+				} else if options.strictStructTag {
+					continue
+				}
+			}
+
+			if util.StrInList(fieldName, ord) {
+				return nil, fmt.Errorf("duplicate struct field name: `%s` alias: `%s`", field.Name, fieldName)
 			}
 
 			m[fieldName] = tt
@@ -173,7 +255,7 @@ func TypeOf(t reflect.Type) (*Type, error) {
 		ord := []string{}
 
 		for i := 0; i < typ.NumIn(); i++ {
-			tt, err := TypeOf(typ.In(i))
+			tt, err := ConfigurableTypeOf(typ.In(i), opts...)
 			if err != nil {
 				return nil, err
 			}
@@ -186,7 +268,7 @@ func TypeOf(t reflect.Type) (*Type, error) {
 		var err error
 		// we currently leave out nil if there are no return values
 		if c := typ.NumOut(); c == 1 {
-			out, err = TypeOf(typ.Out(0))
+			out, err = ConfigurableTypeOf(typ.Out(0), opts...)
 			if err != nil {
 				return nil, err
 			}