-
Notifications
You must be signed in to change notification settings - Fork 683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate default profile names, fix bug when using multiple flat profiles #1488
Conversation
…rofiles When running InSpec with multiple profiles, and two or more of the profiles are read in using the "Flat" SourceReader (i.e. they are not actual profiles with a metadata file like inspec.yml, but rather just a folder containing .rb files with controls and tests in them), InSpec would throw a NilClass error when building the necessary objects for the formatter. The cause was in `#profile_contains_example` in the formatter code which checks to see if the profile name is the same as the profile_id in the given example. However, if both of those were nil, it would potentially match the wrong Flat-read profile. This change fixes this in two ways: refusing to match if the profile name or example profile ID is nil, and adding a default name to a profile if it doesn't have a title or name. This will solve the matching issue and also clean up the formatter output so users can more easily tell what tests are from which profile/path. Signed-off-by: Adam Leff <adam@leff.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix and the improved test coverage. I am not sure we should support a fallback to title
.
return unless metadata.params[:name].nil? | ||
|
||
# if there's a title, there is no need to set a name too | ||
return unless metadata.params[:title].nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we should allow to set the name by title
. This may be confusing. Do we have any additional value by supporting that? If the inspec.yml
is available, but name
is not specified, we could also display (not specified)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this comment. We don't actually set the name by title
. This simply does not set a default name
if there's already a title
field because it would be counter-intuitive to our users.
Example, if I had a profile inspec.yml with title: My Profile
, without this line, we would leave the title alone AND create a name
containing "tests from tests/foo"... and then you'd have CLI formatter output looking like this:
Profile: My Profile (tests from tests/foo)
... which might be fine but felt like added functionality out of the scope of this bug fix. Therefore, I opted not to add a name
if there was already a title
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, makes sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement @adamleff
return unless metadata.params[:name].nil? | ||
|
||
# if there's a title, there is no need to set a name too | ||
return unless metadata.params[:title].nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, makes sense to me
When running InSpec with multiple profiles, and two or more of the profiles
are read in using the "Flat" SourceReader (i.e. they are not actual profiles
with a metadata file like inspec.yml, but rather just a folder containing
.rb files with controls and tests in them), InSpec would throw a NilClass
error when building the necessary objects for the formatter.
The cause was in
#profile_contains_example
in the formatter code whichchecks to see if the profile name is the same as the profile_id in the given
example. However, if both of those were nil, it would potentially match the
wrong Flat-read profile. By refusing to match if either are nil, we fix the
syntax issue, but the output is confusing:
The tests now run and are outputted, but we erroneously show targets that have no tests run, and the tests from the two profiles are jumbled together. By assigning a default name to a profile that has no title/name, we can clean up the output and provide our user more helpful information about their tests.
Fixes #1456.
Signed-off-by: Adam Leff adam@leff.co