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

More meaningful error when including controls from a missing dependency #3770

Merged
merged 2 commits into from Jan 31, 2019

Conversation

Projects
None yet
3 participants
@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii commented Jan 30, 2019

This changes the error message from using a bad reference in
include_controls from:

NoMethodError: undefined method `profile' for nil:NilClass

To one detailing that the profile cannot be loaded since it isn't listed
as a dependency.

This closes #3769.

Fix `undefined method` error from `inspec check`
This changes the error message from using a bad reference in
`include_controls` from:

```
NoMethodError: undefined method `profile' for nil:NilClass
```

To one detailing that the profile cannot be loaded since it isn't listed
as a dependency.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

A few thoughts on testing, but nothing to block the merge.

invalid_profile = File.join(profile_path, 'invalid-include-controls')
out = inspec('check ' + invalid_profile)
out.exit_status.must_equal 1
out.stderr.must_match /Cannot load 'invalid_name'/

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

I'd like to check for the string 'is not listed as a dependency' in the error message, since that is main concern of this change.

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 30, 2019

Author Contributor

I can add another line 👍

@@ -0,0 +1,3 @@
# Invalid Include Controls

This profile contains an `include_controls` line with a invalid reference

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

Personally, I prefer to delete the README (which is optional) and place the description of the test profile in the profile metadata (which is "less optional").

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 30, 2019

Author Contributor

I agree on both, been a while since I've created one of these.

# encoding: utf-8
# copyright: 2018, The Authors

include_controls 'invalid_name'

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

Not a big deal, but you might consider 'no_such_profile' (or similar) rather than 'invalid_name' - directly communicate what is being tested. (There is nothing invalid about 'invalid_name' - you could have a profile named that.)

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 30, 2019

Author Contributor

Nope, solid feedback. I couldn't find a good name. Will use no_such_profile.

@clintoncwolfe clintoncwolfe changed the title Fix `undefined method` error from `inspec check` More meaningful error when including controls from a missing dependency Jan 30, 2019

Respond to feedback
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Thanks, @jerryaldrichiii!

@miah

miah approved these changes Jan 31, 2019

@clintoncwolfe clintoncwolfe merged commit 761944b into master Jan 31, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@clintoncwolfe clintoncwolfe deleted the ja/fix-bad-name-check branch Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment