read config from file/stdin #293
Conversation
chris-rock
force-pushed
the
sr/json-config
branch
from
e0b2bcd
to
dee703d
Compare
srenatus
added
Type: Bug
chris-rock
force-pushed
the
sr/json-config
branch
from
dee703d
to
4950e84
Compare
|
|
||
| def read_config(file) | ||
| if file == '-' | ||
| puts 'WARN: reading JSON config from standard input' if STDIN.tty? |
There was a problem hiding this comment.
what advantage do we get from printing a warning here? I'm just thinking about e.g. raw json output on stdout, where this might interfere
There was a problem hiding this comment.
@arlimus when a user passes --json-config=-, she will just see that nothing happens. With this warning, the user will get an indication what is expected of her at the moment (i.e. provide JSON from the tty). It just affects the case of "reading from stdin when the session is interactive" (as opposed to non-interactive, i.e. cat config.json | inspec detect --json-config=-)
|
Awesome new functionality + diagnose helper! |
| pp options_json | ||
| puts 'Merged configuration:' | ||
| pp opts | ||
| end |
There was a problem hiding this comment.
What do you think about an additional newline?
Here's a use-case where this might be clearer:
./bin/inspec detect --diagnose
InSpec version: 0.9.5
Train version: 0.9.1
Command line configuration:
{"diagnose"=>true}
JSON configuration file:
{}
Merged configuration:
{"diagnose"=>true}
{"name":null,"family":"arch","release":"4.2.5-1-ARCH","arch":null}
The last line feels like a part of diagnose.
There was a problem hiding this comment.
Or am I just using it wrong?
There was a problem hiding this comment.
No, it's fine. I'll add a newline.
srenatus
force-pushed
the
sr/json-config
branch
from
4950e84
to
6ac7613
Compare
|
I think I missed something with the reading from STDIN. please don't merge this yet |
|
@arlimus problem solved, ready to go now :) |
srenatus
force-pushed
the
sr/json-config
branch
from
8419999
to
7ea80e7
Compare
|
Ah, didn't see that. Thanks @srenatus , that solves it! |
|
As discussed, I'll give this one a touch-up. Please don't merge. |
Because of the way per-command arguments are handled, this is a little different from the way e.g. kitchen handles it: any inspec command can take the flag `--diagnose` to have it dump configuration first. This add support for a JSON configuration file, where both inspec detect --json-config=config.json and inspec detect --json-config=- <config.json allow for reading the JSON config. There is no validation of its keys in place.
srenatus
force-pushed
the
sr/json-config
branch
from
792915a
to
ef487e3
Compare
|
ok, done :) |
|
👍 Thank you @srenatus !! |
chris-rock
removed
Type: Bug
Fixes #292.
Removing the defaults was pragmatic:
options['something']isnilwhen the key isn't present anyways (and thus bothdefault: nilanddefault: falseare not necessary)options_jsonhash in the merge -- and we actually want the CLI parameters to take precedence.An alternative might have been to declare an explicit "defaults" hashmap, but since the available configurables are supposed to come from train directly (in the future), this might not make too much sense now.
Shall I squash those commits?