pass options hash sans target key #1646
Conversation
Signed-off-by: Jeremy J. Miller <jm@chef.io>
| @@ -89,7 +89,7 @@ def initialize(source_reader, options = {}) | |||
| @writable = options[:writable] || false | |||
| @profile_id = options[:id] | |||
| @cache = options[:cache] || Cache.new | |||
| @backend = options[:backend] || Inspec::Backend.create(options) | |||
| @backend = options[:backend] || Inspec::Backend.create(options.select { |k, _| k != 'target' }) | |||
There was a problem hiding this comment.
The fact that we hit (read: I created) this bug in the first place is because we blindly pass a hash of options that is created by the caller rather than curating a hash of options we actually need. Rather than deleting an option we don't need, I'd rather we only pass the options we do need. Otherwise, people like me will see calls to #delete without knowing why, there won't be a comment as to why, and then bugs are created. 🙂
It's possible that we need to be actually creating the hash we need in Inspec::Backend rather than here.
@chris-rock / @arlimus - thoughts?
There was a problem hiding this comment.
To me this area of the code is a bit opaque (regarding passing of a :target) without knowing what the message spec from Profile -> Backend should look like under different circumstances. I agree that we should determine what the message(s) should contain and be able to enforce that on the receiving end.
There was a problem hiding this comment.
I think we need to make the separation more clear. The problem with the options we do need is that the backend is getting most of the cli options, since all the ssh or winrm options are just passed through. I think we should create an issue to solve that in another case. The real issue I am seeing here is that we need to run integration tests with Chef Automate for every PR, since a change should break the build.
So we need to do two things:
- Integration testing with Chef Compliance / Chef Automate
- Make the interface between inspec / train more clear
There was a problem hiding this comment.
Agreed that additional testing is always a good thing. I don't agree that that's the "real issue." We have a situation in the codebase where A is creating an options hash, B is passing it to C, but not all the options are needed by C and, when they are all included, we break. So even if "all the ssh and winrm options are just passed through," we're not doing a good job with the separation. Testing will catch it, but the lack of testing is not the cause here.
There was a problem hiding this comment.
I confirmed that issue. LGTM. Thank you for spotting that issue @jeremymv2
| @@ -89,7 +89,7 @@ def initialize(source_reader, options = {}) | |||
| @writable = options[:writable] || false | |||
| @profile_id = options[:id] | |||
| @cache = options[:cache] || Cache.new | |||
| @backend = options[:backend] || Inspec::Backend.create(options) | |||
| @backend = options[:backend] || Inspec::Backend.create(options.select { |k, _| k != 'target' }) | |||
There was a problem hiding this comment.
I think we need to make the separation more clear. The problem with the options we do need is that the backend is getting most of the cli options, since all the ssh or winrm options are just passed through. I think we should create an issue to solve that in another case. The real issue I am seeing here is that we need to run integration tests with Chef Automate for every PR, since a change should break the build.
So we need to do two things:
- Integration testing with Chef Compliance / Chef Automate
- Make the interface between inspec / train more clear
When passing options to `Train` we need to ensure we do not pass the `'target'` key. See: #1646 NOTE: When running `inspec exec` that key is `:target` so it is not removed, but since `options[:backend]` already exists this is not an issue.
When passing options to `Train` we need to ensure we do not pass the `'target'` key. See: #1646 NOTE: When running `inspec exec` that key is `:target` so it is not removed, but since `options[:backend]` already exists this is not an issue. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
When passing options to `Train` we need to ensure we do not pass the `'target'` key. See: #1646 NOTE: When running `inspec exec` the key is `:target` so it is not removed, but since `options[:backend]` already exists this is not an issue. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
When passing options to `Train` we need to ensure we do not pass the `'target'` key. See: #1646 NOTE: When running `inspec exec` the key is `:target` so it is not removed, but since `options[:backend]` already exists this is not an issue. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
This PR fixes #1631
Originally, the
:targetkey was deleted from theoptionshash in theinitializemethod ofInspec::Profile, before being passed toInspec::Backend.create()Recently, in the commit below, the:targetdelete method was removed so that the key still existed in the hash: d0bc085#diff-d1eede4085683a605d9b8892a198f642L85This change introduced an unintended behavior: when train goes off to validate in the
validate_backendmethod, it's raising an error since:targetis notnil: https://github.com/chef/train/blob/master/lib/train.rb#L121-L124This PR simply reverts the original behavior by passing the
optionshash toInspec::Backend.create()without the:targetkey in it. With this, the functionality works as before and preserves the desired effect from the commit above.Signed-off-by: Jeremy J. Miller jm@chef.io