Make explicit configuration choices override those set earlier in load order

[atc0005]

Right on the heels of both implementing (#156) and further clarifying the behavior (#160), I wonder whether the implemented behavior is worth violating the Principle of Least Surprise.

If I'm using an application and I specify a command-line flag I will expect that it applies the requested behavior unless I happen to know (or see it clearly mentioned in Help output) that my specifying a flag will fail to have an effect if I choose a value that is the default for the application.

To be clear, picture this configuration load scenario:

1. A default value of text is set for the log format
2. Environment variables are evaluated/scanned and one for this setting is not found (no change)
3. Config file is loaded and json value for the log format is found and set
4. Command-line flags are evaluated and text was specified as the value for the log format

Net result? json wins out because the incoming value for the command-line flag is a default value and the prior value is not.

[atc0005]

This would prove surprising to me and likely to a user as well. I think it is fine to feather the configuration settings as the README (now) shows, but any explicit settings specified by later (re load order) config options should override those set earlier, regardless of whether the incoming value is a default or not.

[atc0005]

Perhaps generate the base config object using default values, but generate the config file and flag (really all other) config objects using invalid/placeholder values. If the placeholder values are found, disregard them (probably explicitly note this using the LogBuffer object) and keep the prior value, otherwise use the new value.

[atc0005]

Relevant:

• Question: See if a flag was set by the user spf13/cobra#23
• Support IsSet() method on flags spf13/cobra#453 (comment)

[atc0005]

Opened issue here: alexflint/go-arg#94

Light research indicates that I might be able to pull this off using custom parsing:

https://github.com/alexflint/go-arg#custom-parsing

Implement encoding.TextUnmarshaler to define your own parsing logic.

Perhaps if pelletier/go-toml provides the same custom parsing option I can use custom struct types in place of the mostly primitive types to hold the intended value and alongside it a boolean flag noting whether it was set?

[atc0005]

• https://godoc.org/github.com/spf13/pflag#FlagSet.Changed
• https://github.com/spf13/pflag/blob/master/flag.go#L508

// Changed returns true if the flag was explicitly set during Parse() and false
// otherwise
func (f *FlagSet) Changed(name string) bool {
	flag := f.Lookup(name)
	// If a flag doesn't exist, it wasn't changed....
	if flag == nil {
		return false
	}
	return flag.Changed
}

---

• https://github.com/spf13/pflag/blob/master/flag.go#L170

// A Flag represents the state of a flag.
type Flag struct {
	Name                string              // name as it appears on command line
	Shorthand           string              // one-letter abbreviated flag
	Usage               string              // help message
	Value               Value               // value as set
	DefValue            string              // default value (as text); for usage message
	Changed             bool                // If the user set the value (or if left to default)
	NoOptDefVal         string              // default value (as text); if the flag is on the command line without any options
	Deprecated          string              // If this flag is deprecated, this string is the new or now thing to use
	Hidden              bool                // used by cobra.Command to allow flags to be hidden from help/usage text
	ShorthandDeprecated string              // If the shorthand of this flag is deprecated, this string is the new or now thing to use
	Annotations         map[string][]string // used by cobra.Command bash autocomple code
}

[atc0005]

• Add scale instancegroup command kubernetes/kops#2765 (comment)

Nit: It's probably better to use the Changed field on the flag (doing this in the cobra.Command Run function instead) (spf13/cobra#23). The reason being that then the -1 default value won't appear in the help!

I noticed this in the latest help output for this app (I was keying off of -1 to indicate a field that had not be updated by a config file, env var or command-line flag). It stuck out rather prominently.

[atc0005]

https://godoc.org/github.com/pelletier/go-toml#ex-package--Tree

This demos using fetching the data directly instead of calling toml.Unmarshal():

config, err := toml.LoadFile("config.toml")

if err != nil {
    fmt.Println("Error ", err.Error())
} else {
    // retrieve data directly
    directUser := config.Get("postgres.user").(string)
    directPassword := config.Get("postgres.password").(string)
    fmt.Println("User is", directUser, " and password is", directPassword)

    // or using an intermediate object
    configTree := config.Get("postgres").(*toml.Tree)
    user := configTree.Get("user").(string)
    password := configTree.Get("password").(string)
    fmt.Println("User is", user, " and password is", password)

    // show where elements are in the file
    fmt.Printf("User position: %v\n", configTree.GetPosition("user"))
    fmt.Printf("Password position: %v\n", configTree.GetPosition("password"))
}

The package also has a GetDefault method that could be used to return an "obvious" placeholder value that our MergeConfig function could match on to determine whether the incoming setting should override an earlier value.

https://godoc.org/github.com/pelletier/go-toml#Tree.GetDefault

[atc0005]

When processing env vars, you're early enough in the process that you don't have to worry with merging: you assign what you find.

When processing config files, merging in non-default, valid values should be "good enough".

When processing command-line flags, I'll need some way to know what options were explicitly set. Ideally the command-line flag package would have a way of turning off or disabling pulling in config values from other sources (e.g., alexflint/go-arg package will automatically pull in env var values). Thanks to Alex Flint for pointing that out; I was so focused on detecting explicit flags that I forgot how that package would build its own unified set of config options based on flags and env vars.

If I can detect that flags were explicitly set, I think I can pull this off without too much trouble.

If alexflint/go-arg does not or will not (e.g., the desired behavior is out of the scope for the package) support this, I will need to (re)-evaluate some of the packages which do support the behavior. Out of those available, I suspect that targeting a config struct is unsupported.

Some potential candidates for use:

• https://github.com/DavidGamba/go-getoptions
• https://github.com/spf13/viper
  • this handles env vars, config files and flags
• https://github.com/spf13/pflag
  • https://godoc.org/github.com/spf13/pflag#Parsed
  • https://godoc.org/github.com/spf13/pflag#Flag
• https://github.com/urfave/cli
  • this handles env vars, config files and flags

[atc0005]

Another option that I've not fully thought through yet:

Have alexflint/go-arg target a separate struct with the majority of fields of either string or []string type. I could then have the default values set to something like PLACEHOLDER and if present, the option hasn't been explicitly set, if not, then run validity checks against the provided (and converted) value before merging into a target config struct.

[atc0005]

What if the precedence order was changed?

https://github.com/urfave/cli/blob/master/docs/v1/manual.md#precedence

This notes that their package places environment variables higher in the list than config file values:

The precedence for flag value sources is as follows (highest to lowest):

Command line flag value from user
Environment variable (if specified)
Configuration file (if specified)
Default defined on the flag

Maybe I assumed the convention was config file over env variable and that's not the case? If we change the precedence order, does that make config merging an easier problem to solve?

[atc0005]

After following some of the ideas found in https://github.com/google/go-github I updated the Config struct fields to be private and created Getter methods (using former CamelCase public field names per the Effective Go doc.

Unfortunately it was after I finished the work and squelched the linting/compile errors that I saw this output:

config\config.go:71:2: `logFilePath` is unused (structcheck)
        logFilePath   *string `toml:"log_file_path" arg:"--log-file,env:ELBOW_LOG_FILE" help:"Optional log file used to hold logged messages. If set, log messages are not displayed on the console."`
        ^
config\config.go:72:2: `consoleOutput` is unused (structcheck)
        consoleOutput *string `toml:"console_output" arg:"--console-output,env:ELBOW_CONSOLE_OUTPUT" help:"Specify how log messages are logged to the console."`
        ^
config\config.go:73:2: `useSyslog` is unused (structcheck)
        useSyslog     *bool   `toml:"use_syslog" arg:"--use-syslog,env:ELBOW_USE_SYSLOG" help:"Log messages to syslog in addition to other outputs. Not supported on Windows."`
        ^
config\config.go:69:2: `logLevel` is unused (structcheck)
        logLevel      *string `toml:"log_level" arg:"--log-level,env:ELBOW_LOG_LEVEL" help:"Maximum log level at which messages will be logged. Log messages below this threshold will be discarded."`
        ^
config\config.go:70:2: `logFormat` is unused (structcheck)
        logFormat     *string `toml:"log_format" arg:"--log-format,env:ELBOW_LOG_FORMAT" help:"Log formatter used by logging package."`
        ^
config\config.go:62:2: `paths` is unused (structcheck)
        paths           *[]string `toml:"paths" arg:"--paths,env:ELBOW_PATHS" help:"List of comma or space-separated paths to process."`
        ^
config\config.go:63:2: `recursiveSearch` is unused (structcheck)
        recursiveSearch *bool     `toml:"recursive_search" arg:"--recurse,env:ELBOW_RECURSE" help:"Perform recursive search into subdirectories per provided path."`
        ^
config\config.go:41:2: `appName` is unused (structcheck)
        appName        *string `toml:"app_name" arg:"-"`
        ^
config\config.go:42:2: `appDescription` is unused (structcheck)
        appDescription *string `toml:"app_description" arg:"-"`
        ^
config\config.go:43:2: `appVersion` is unused (structcheck)
        appVersion     *string `toml:"app_version" arg:"-"`
        ^
config\config.go:44:2: `appURL` is unused (structcheck)
        appURL         *string `toml:"app_url" arg:"-"`
        ^
config\config.go:54:2: `keepOldest` is unused (structcheck)
        keepOldest     *bool     `toml:"keep_oldest" arg:"--keep-old,env:ELBOW_KEEP_OLD" help:"Keep oldest files instead of newer per provided path."`
        ^
config\config.go:55:2: `remove` is unused (structcheck)
        remove         *bool     `toml:"remove" arg:"--remove,env:ELBOW_REMOVE" help:"Remove matched files per provided path."`
        ^
config\config.go:56:2: `ignoreErrors` is unused (structcheck)
        ignoreErrors   *bool     `toml:"ignore_errors" arg:"--ignore-errors,env:ELBOW_IGNORE_ERRORS" help:"Ignore errors encountered during file removal."`
        ^
config\config.go:50:2: `filePattern` is unused (structcheck)
        filePattern    *string   `toml:"pattern" arg:"--pattern,env:ELBOW_FILE_PATTERN" help:"Substring pattern to compare filenames against. Wildcards are not supported."`
        ^
config\config.go:51:2: `fileExtensions` is unused (structcheck)
        fileExtensions *[]string `toml:"file_extensions" arg:"--extensions,env:ELBOW_EXTENSIONS" help:"Limit search to specified file extensions. Specify as space separated list to match multiple required extensions."`
        ^
config\config.go:52:2: `fileAge` is unused (structcheck)
        fileAge        *int      `toml:"file_age" arg:"--age,env:ELBOW_FILE_AGE" help:"Limit search to files that are the specified number of days old or older."`
        ^
config\config.go:53:2: `numFilesToKeep` is unused (structcheck)
        numFilesToKeep *int      `toml:"files_to_keep" arg:"--keep,env:ELBOW_KEEP" help:"Keep specified number of matching files per provided path."`

Yep. Unfortunately I forgot that the go-toml package (the stdlib json package too I think) require public fields in order to populate stucts. Looks like that may be why the google/go-github package uses the Get prefix for its Getter methods?

Taking another look, I see json struct tags sprinkled liberally about their structs, so that's probably the case.

Example:

// apps.go

// App represents a GitHub App.
type App struct {
	ID          *int64     `json:"id,omitempty"`
	NodeID      *string    `json:"node_id,omitempty"`
	Owner       *User      `json:"owner,omitempty"`
	Name        *string    `json:"name,omitempty"`
	Description *string    `json:"description,omitempty"`
	ExternalURL *string    `json:"external_url,omitempty"`
	HTMLURL     *string    `json:"html_url,omitempty"`
	CreatedAt   *Timestamp `json:"created_at,omitempty"`
	UpdatedAt   *Timestamp `json:"updated_at,omitempty"`
}

// github-accessors.go

// GetID returns the ID field if it's non-nil, zero value otherwise.
func (a *App) GetID() int64 {
	if a == nil || a.ID == nil {
		return 0
	}
	return *a.ID
}

[atc0005]

It seems that I don't know how to properly initialize/assign values to struct fields of pointer type. More research/testing.

[atc0005]

It seems that I don't know how to properly initialize/assign values to struct fields of pointer type. More research/testing.

• https://stackoverflow.com/questions/43811652/golang-assigning-a-value-to-struct-member-that-is-a-pointer
• https://github.com/google/go-github/blob/cad32f26bbd0216620995d028657c304c95a5eab/github/admin_users.go#L25

// CreateUser creates a new user in GitHub Enterprise.
//
// GitHub Enterprise API docs: https://developer.github.com/enterprise/v3/enterprise-admin/users/#create-a-new-user
func (s *AdminService) CreateUser(ctx context.Context, login, email string) (*User, *Response, error) {
	u := "admin/users"

	userReq := &createUserRequest{
		Login: &login,
		Email: &email,
	}

	req, err := s.client.NewRequest("POST", u, userReq)
	if err != nil {
		return nil, nil, err
	}

	var user User
	resp, err := s.client.Do(ctx, req, &user)
	if err != nil {
		return nil, resp, err
	}

	return &user, resp, nil
}

In particular:

	userReq := &createUserRequest{
		Login: &login,
		Email: &email,
	}

[atc0005]

Received this via Twitter discussion (https://twitter.com/smazero/status/1196790750660505601):

https://play.golang.com/p/NYoOD_rN03p

Need to go back over it (only briefly scanned it) and study when time permits.

[atc0005]

Latest stab at this: 46e85c2

$ go build
$ ./elbow
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x584727]

goroutine 1 [running]:
github.com/atc0005/elbow/config.Config.Version(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /mnt/t/github/elbow/config/config.go:394 +0x37
github.com/alexflint/go-arg.NewParser(0x0, 0x0, 0xc0000d1e28, 0x1, 0x1, 0xc0000d1bd0, 0x40dbc8, 0x30)
        /home/ubuntu/go/pkg/mod/github.com/alexflint/go-arg@v1.2.0/parse.go:217 +0x98a
github.com/alexflint/go-arg.MustParse(0xc0000d1e28, 0x1, 0x1, 0x0)
        /home/ubuntu/go/pkg/mod/github.com/alexflint/go-arg@v1.2.0/parse.go:79 +0x54
github.com/atc0005/elbow/config.NewConfig(0x5f3eb4, 0x5, 0x5ffe8b, 0x70, 0x5faffd, 0x20, 0x5f3fe0, 0x5, 0x0)
        /mnt/t/github/elbow/config/config.go:167 +0x570
main.main()
        /mnt/t/github/elbow/main.go:50 +0xa3

[atc0005]

Digging further and was reminded that as part of generating Help output, the Description() and Version methods were being called. Those methods were attempting to dereference pointers that (at that point in the code) had no value. I replaced the bare dereference attempts with Getter method calls and that solved the immediate issue.

I'll need to dig further to see how I can partially initialize all config structs to have a useful value. Perhaps move the initial default values for app name, url, version, etc to the Getter methods?

[atc0005]

Is something like this horribly un-idiomatic?

config.SetNumFilesToKeep(config.GetNumFilesToKeep())

Current thinking is that the Getter method can return default values instead of manually setting them inside of the NewConfig() function. This would allow non-configured structs to have starter values for methods that are called on them (provided those methods don't access struct fields directly), but should the config struct already be configured via flags, env vars, config file then those values would be returned.

[atc0005]

At this point I've mostly fleshed out a direction I think I'm going to go for a time. I worked out most of the invalid pointer dereference issues, but when running a battery of tests (external, far from complete or "proper") I got output that I tracked down to an error from the logrus package:

Failed to write to log, invalid argument
Failed to write to log, invalid argument
Failed to write to log, invalid argument
Failed to write to log, invalid argument

After additional digging, I found that I made at least two mistakes:

• failed to properly panic/abort after merging in config settings from the go-arg package and finding that the result failed validation (e.g., providing a known invalid flag value)
• failed to properly guard against an unexpected flag setting by specifying a safe default case statement

I don't know which is more idiomatic:

• providing a safe default case statement even if I don't expect it will ever be used
• trigger a panic if the default case statement is used (tempting, very, very tempting)
• keep switch options limited to just valid values and "be careful"
  • I suspect this is not the recommended way to handle this

As an example, here is the function containing the switch block that I mentioned:

func SetLoggerConsoleOutput(logger *logrus.Logger, consoleOutput string) {

	var loggerOutput *os.File

	switch {
	case consoleOutput == "stdout":
		loggerOutput = os.Stdout
	case consoleOutput == "stderr":
		loggerOutput = os.Stderr
	}

	logger.SetOutput(loggerOutput)

}

Until I learn differently, I'm going to add a default case statement here that calls panic. Once I can confirm best practice I can go further from there.

[atc0005]

From a commit message I just pushed:

Need to refine; should NewConfig() handle panicing or should this
behavior be handled by main() function?

In short, would I expect the go-arg, go-toml and other config
packages to immediately panic when problems are encountered
or would I expect them to return the error "to me" in main() so
I could consider further for final behavior?

I think the latter ...

I'm already relying upon the immediate behavior noted here:

https://godoc.org/github.com/alexflint/go-arg#MustParse

MustParse processes command line arguments and exits upon failure

Though to elaborate, this is if an invalid flag is passed, not an invalid/unsupported setting for the flag. A close approximation is if the specified config filename doesn't exist. That should also result in an immediate failure?

[atc0005]

The package author responded and thankfully it was an easy fix: set os.Args to nil before calling arg.MustParse().

In my case I followed advice mentioned elsewhere to capture the old value and then restore it when finished, but overall the fix is as indicated. The TestMergeConfig() function is working as intended!

[atc0005]

As noted yesterday, it seems that the blockers have been removed and so far; I am hopeful that the logic bugs present in earlier versions have been resolved, based on what the tests are showng.

---

One big item that needs to be fixed is the precedence details. The reality is that the current selection of packages doesn't allow for environment variables to be overridden by config file entries, so my earlier plan of having precedence work in that order will need to be discarded for now; I'll need to update the docs to fix the precedence details.

I also still need to do the research to learn which of the two should override the other. The Twelve-Factor App methodology discourages the use of config files and for the most part I'm coming to agree with the idea, but I still feel there are a large number of one-off or command-line applications that would benefit from being able to reference a common set of configuration options, such as what a config file would provide. That said, I don't know that there are a lot of use cases for needing to override environment variables with a config file. If anything, setting an environment variable would probably be an effort to override a setting elsewhere?

refs:

• https://12factor.net/config
• https://en.wikipedia.org/wiki/Twelve-Factor_App_methodology

[atc0005]

Note to self:

The current TestMergeConfig function assumes that we can safely compare the newest config struct against the baseConfig object to prove that the merge was successful.

We need to be able to also test and ensure that partial configurations merge properly. To do this, we may need to construct an "expected" config struct and compare that against the baseConfig object. Come to think of it, I think that is what I had in mind to start with. The TestMergeConfig function (along with the Compare function), was meant to work with a base-case scenario, believing that if it worked, so would the complete merge process.

[atc0005]

We need to be able to also test and ensure that partial configurations merge properly. To do this, we may need to construct an "expected" config struct and compare that against the baseConfig object. Come to think of it, I think that is what I had in mind to start with. The TestMergeConfig function (along with the Compare function), was meant to work with a base-case scenario, believing that if it worked, so would the complete merge process.

Implemented and working.

[atc0005]

At this point I think I've got "enough" to work with for crafting a new PR to finish the work started in #156. Before I do, I think I'll go ahead and remove the support for overriding the application metadata:

• name
• version
• url
• description

I can't come up with a good reason why this is needed. If I was building a framework for multiple applications perhaps, but not for this.

Once that is done I'll likely create an "archival" branch for later reference that doesn't have the commits squashed, and then refresh the current branch and squash down to just one.

[atc0005]

Question to self:

Where to move/place the metadata details? They're needed early in the application setup process; perhaps just leave them as provided by the Getter methods, but remove support for overriding via config file? That should cover most of the bases for now.

[atc0005]

As part of the cleanup I've opted to change the AppMetadata fields from pointer type back to value types. This requires a lot of adjustments across tests and validation code. I've taken a hatchet and have made bulk changes, but still have a ways to go.

When finished, I'll need to make sure that I've not brute-forced the AppMetadata fields and that the merge behavior is still being tested.

[atc0005]

When finished, I'll need to make sure that I've not brute-forced the AppMetadata fields and that the merge behavior is still being tested.

Just spot checked the TestMergeConfigUsingIncompletConfigObjects() function and it looks like I opted to use the baseConfig values for the AppMetadata fields for the other config objects (putting the focus on merging other fields), so nothing lost there.

[atc0005]

Ran into another problem, opened an issue in the alexflint/go-arg project repo. In short, the go test framework is passing in command-line flag options which the alexflint/go-arg package is interpreting as invalid flags for it and I don't know how to configure it to ignore the test framework flags.

...

The package author responded and thankfully it was an easy fix: set os.Args to nil before calling arg.MustParse().

In my case I followed advice mentioned elsewhere to capture the old value and then restore it when finished, but overall the fix is as indicated. The TestMergeConfig() function is working as intended!

Relevant to this problem:

• unknown arg identification skips test. flags injected by go test integrii/flaggy#59
• tests for skipping parses of test flags integrii/flaggy#60

Here the developer looked for the test prefix for flags used by the Go test framework and skipped processing them. I don't know whether unsetting os.Args or skipping flags with the test prefix is best for long-term use, but knowing both approaches should be useful.