Skip to content
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

Improve error messages from compliance fetcher #1126

Merged
merged 1 commit into from
Sep 26, 2016

Conversation

stevendanna
Copy link
Contributor

Signed-off-by: Steven Danna steve@chef.io

@chris-rock
Copy link
Contributor

I am not convinced that a fetcher should exit the application. It may throw an exception, but it should never exit the application. If used as a library (as in audit cookbook), the fetcher is not executed as cli application. From my perspective only the CLI application should decide if an exit code should be thrown or not

module UI
def self.exit_with_error(message)
$stderr.puts message
exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly advice against to exit application from an internal method.

@@ -24,11 +25,23 @@ def self.resolve(target)

# check if we have a compliance token
config = Compliance::Configuration.new
return nil if config['token'].nil?
if config['token'].nil?
Inspec::UI.exit_with_error <<EOF
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a great idea to add better information about an error, but that should be encapsulated in exceptions

@stevendanna
Copy link
Contributor Author

@chris-rock Yeah, I am not thrilled with exiting from here either. One of the issues with the exception is not dumping the stack trace which obscures the error message.

@stevendanna
Copy link
Contributor Author

stevendanna commented Sep 23, 2016

Updated per code review

Signed-off-by: Steven Danna <steve@chef.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants