Validation warnings #73
Conversation
| @@ -35,6 +35,16 @@ type ConfigFile struct { | |||
| ExcludePatterns *RegexCollection `yaml:"exclude_patterns"` | |||
| } | |||
|
|
|||
| func (cf ConfigFile) printValidationWarnings() { | |||
| if len(cf.Files) == 0 { | |||
| log.Warningf("Configuration file contains no files to watch") | |||
There was a problem hiding this comment.
@leonsodhi: Do you think this is worth printing?
There was a problem hiding this comment.
I'd say the more error handling the better. Silence is a right PITA to troubleshoot, and means we have no error message to work from when customers get in touch.
|
@leonsodhi: Code looks good to me. Can you edit the error messages (and if you think they can be clearer, comments), either by merging and creating a new branch+PR or by branching from this branch? |
|
@leonsodhi @troy I added more validation warnings. They will come before daemonization on parent stage or if no daemonization required. To determine the stage, I updated |
|
@leonsodhi this looks good to me other than wording and grammar on the error strings and error code variable name ("Exists"). If it saves you time to put revisions in here for Yury to implement, go for it. |
| @@ -189,7 +206,7 @@ func (cm *ConfigManager) loadConfigFile() error { | |||
| file, err := ioutil.ReadFile(cm.Flags.ConfigFile) | |||
| // don't error if the default config file isn't found | |||
| if os.IsNotExist(err) && cm.Flags.ConfigFile == DefaultConfigFile { | |||
| return nil | |||
| return defaultConfigDoesNotExist | |||
There was a problem hiding this comment.
It's totally fine for someone to only pass in params via the CLI switches and not have a config file. I'd vote for removing this warning.
There was a problem hiding this comment.
If user misplaced config file, there is no way to know that. In common, users do not read the code to understand why utility fails. They write to support. One notification line can save your and users' time.
There was a problem hiding this comment.
Completely agree on users not wanting to read code on failures, and they definitely shouldn't have to. However, IMO, we should validate that all required parameters have been supplied and not worry about where they came from. If a user specifies all required parameters via the CLI, a config file isn't necessary and thus we can't justify suggesting something is wrong.
The only situation I can think of where this falls down is when an optional parameter is specified in a config file and r_s2 is never asked to load it. One solution to that might be printing the state of all settings when -D is used. I'd prefer to hold off doing that ATM as it hasn't been something that comes up in practice. We can always revisit if things change.
Let me know if I've missed a scenario here or there's a hole in my logic.
|
Summing all comments above, do you want to have |
Print configuration file validation warnings if:
Exit with error code if no files to watch specified at all (in config and via command line).
Related to #43