coreos / mayday

A diagnostics tool for capturing system state.
Apache License 2.0
81 stars 27 forks source link

redo config handling #36

Closed tschuy closed 8 years ago

tschuy commented 8 years ago

viper doesn't really have a convenient way to check if the config file wasn't found, so instead I've just added a check to see if viper says it can't read config type "", which is what's returned instead of a more appropriate error.

See also https://github.com/spf13/viper/issues/210

tschuy commented 8 years ago

1: config file missing/not found

tschuy@tschuy:(mayday)(master) → bin/mayday
2016/07/19 14:58:52 Could not find configuration file in /etc/mayday/config.json or in working directory.
tschuy@tschuy:(mayday)(master) → 

2: config file corrupted

2016/07/19 14:59:06 Error reading configuration file.
2016/07/19 14:59:06 Fatal error reading config: While parsing config: invalid character '\n' in string literal
tschuy@tschuy:(mayday)(master) →

3: config file working fine

tschuy@tschuy:(mayday)(master) → bin/mayday
2016/07/19 14:58:58 Could not connect to rkt. Verify mayday has permissions to launch the rkt client.
2016/07/19 14:58:58 Connection error: exit status 1
2016/07/19 14:58:58 Collecting file: "/proc/vmstat"
^C
tschuy@tschuy:(mayday)(master) → bin/mayday
tschuy commented 8 years ago

I've also removed . from the list of config directories, since it's just a -c . away now.

brianredbeard commented 8 years ago

@tschuy I've made your life slightly more of a pain in the ass because of failing to merge some of these PRs previously.

whomp whomp

tschuy commented 8 years ago

@brianredbeard updated.

It doesn't check the current dir unless you give it a -c ., and will always let you use a profile (aka, config file name).

To use /my/fav/dir/filename.json, it now looks like this:

$ mayday -c /my/fav/dir -p filename

brianredbeard commented 8 years ago

@robszumski want your thoughts real fast from the side of -c to specify a directory where a config lives. It seems a little clunky, but want your opinion.

robszumski commented 8 years ago

@robszumski want your thoughts real fast from the side of -c to specify a directory where a config lives. It seems a little clunky, but want your opinion.

Correct, it seems like you want to do

$ mayday -c /my/fav/dir/filename.json

And ideally, a more readable flag alias:

$ mayday --config /my/fav/dir/filename.json

On a second note, why is a config file required?

brianredbeard commented 8 years ago

Alternatively it could just be -b for base path or -d for directory. I'm not really married to any of the above and don't want to nit pick this into oblivion, just make the UX apparent.

tschuy commented 8 years ago

I'm liking the idea of --config. If we ship multiple config files in /etc/mayday, we can just have the docs say you need to specify the entire path of the config, even when it's a "built-in".

brianredbeard commented 8 years ago

Again though, as a reminder to support CoreOS we will want to be able to ship a default config to be located in /usr/share/mayday so as to both have the configuration be in an immutable location as well as be able to be overridden by the administrator (by placing a blank file of the same name or a symlink of the same filename to /dev/null, similar to masking a unit). As per the OS team:

Brian Redbeard [13:27]  
stylistic packaging/design opinion from the OS team needed:   is it better for an application to 1) both look for a configuration in `/usr/share/appname/default.json` AND `/etc/appname/default.json` or 2) _we_ package the config in `/usr/share/appname/default.json` and use `/usr/lib/tmpfiles.d/appname.conf` to define it's behavior in the CoreOS case allowing the user to break the link and override

[13:27]  
I would assume #2, but want to confirm that is still correct

Michael Marineau [13:28]  
the systemd model which I quite like is the first, and we only ship the /usr one.

Alex Crawford [13:28]  
+1

Michael Marineau [13:28]  
the one in /etc may be created by tmpfiles as a plain file with comments so users know it is a thing

[13:28]  
but the app should work with it missing to.
tschuy commented 8 years ago

I'm very much open to changing the user-facing strings involved.

It's a little clunky but this should have the wanted behavior.

brianredbeard commented 8 years ago

It is done. LGTM.

Proud moment