Resolve merge issue with json-config vs thor defaults #2377
Conversation
Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/merge_json_cli_options
branch
from
e69ee75 to
54e9f61
Compare
lib/inspec/base_cli.rb
Outdated
| @@ -120,7 +120,7 @@ def opts | |||
|
|
|||
| def merged_opts | |||
| # argv overrides json | |||
| Thor::CoreExt::HashWithIndifferentAccess.new(options_json.merge(options)) | |||
| Thor::CoreExt::HashWithIndifferentAccess.new(options.merge(options_json)) | |||
There was a problem hiding this comment.
This will fix the "Thor defaults incorrectly overriding the JSON options" problem, but it introduces the "JSON options overriding user-provided Thor CLI flags" problem. And the comment on line 122 indicates the intention - flags on the CLI should override the JSON.
I think we may have to remove default assignments in Thor and handle them on our own:
- remove all defaults in Thor option declarations
- build a hash of all the options that have defaults, and assign their default values
- merge in the JSON options
- merge in the Thor options
test/unit/base_cli_test.rb
Outdated
|
|
||
| describe 'merge_options' do | ||
| it 'json options override cli' do | ||
| json = { "backend_cache":true } |
There was a problem hiding this comment.
read_config doesn't return JSON but rather parsed JSON. Can we change the variable name to parsed_json so it's clear? initially I thought you were trying to represent actual JSON here and thought you forgot the quotes around the whole string. 🙂
Also, this Hash has a key of a symbol named :backend_cache with the way you've written it. JSON.parse won't return it like that, so we should probably change this to:
parsed_json = { 'backend_cache' => true }
test/unit/base_cli_test.rb
Outdated
| json = { "backend_cache":true } | ||
| cli_options = { 'json_config' => 'dummy', 'backend_cache' => false } | ||
| cli.instance_variable_set(:@options, cli_options) | ||
| cli.expects(:read_config).returns(json) |
There was a problem hiding this comment.
Maybe add a .with to this so we show how the options plumb all the way to the config reading?
cli.expects(:read_config).with('dummy').returns(json)Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/merge_json_cli_options
branch
from
55fcbb6 to
5c7b178
Compare
When testing the backend_cache via json-config we noticed that the Thor defaults were overriding the json-config settings. This was due to a hash merge using the wrong hash as the source. Rearranging the hash merge and using the json-config options as the override hash fixes this issue.
Along with this change it was requested for debug lines to ensure backend_cache was enabled or not.
Signed-off-by: Jared Quick [email protected]