-
Notifications
You must be signed in to change notification settings - Fork 682
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
fail gracefully on inspec compliance profiles when bad token is provided #930
Conversation
bcb62f3
to
7a054f0
Compare
@@ -36,6 +36,7 @@ def self.profiles(config) | |||
end | |||
end.flatten | |||
else | |||
puts '401 Unauthorized. Please check your token.' |
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.
Are we sure the only case where data == ''
is a 401? My first impression when reading this code is that I'm a bit surprised that this isn't a case statement on the http response code. Also, if possible it feels like this is a case where we would want to exit with a non-zero exit status.
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.
good point, thanks!! ya, i'll rework this a bit later -- a case statement on the http response code sounds like the right thing to do there
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.
agree with @stevendanna we should check the http code
puts '401 Unauthorized. Please check your token.' | ||
[] | ||
else | ||
puts response_code |
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.
We should add a message that indicates an error. I assume most users do not know http codes by heart ;-)
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.
oops, ya! i'll add it :)
i'm getting some odd travis failures on ubuntu.... :( |
b5d34ee
to
f1d9650
Compare
else | ||
puts response_code, 'An error occured' |
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.
is this ok? @chris-rock or were you thinking something else/more specific?
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 not familiar with the compliance API, but response.message
will typically be filled with the error message that the server sent back, so you might try something like:
puts "An unexpected error occurred (HTTP #{response_code}): #{response.message}"
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.
Cool idea @stevendanna
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.
In addition to that the API class should not do any output. Only the cli class should do that. therefore we need to return the message as the other methods do it.
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.
Only the cli class should do that. therefore we need to return the message as the other methods do it.
👍 On this. I was going to potentially recommending creating a custom exception that you could raise in the case of an HTTP error. The exception can contain the message the cli class should print on rescue. However, I noticed that this code base stays away from exceptions for the most part.
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.
oooh nice! thanks! i'll add the message stuff and have the api class the return the message so the cli class can take care of the output :)
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.
@stevendanna the whole API implementation needs a refactor, because I want to merge it with the content in our audit cookbook. But lets not address this in that iteration
6dbb61b
to
b42f2ac
Compare
b42f2ac
to
caeff9e
Compare
caeff9e
to
2ca3a13
Compare
2ca3a13
to
d4afa0b
Compare
d4afa0b
to
cf784de
Compare
just a quick fix to handle inspec compliance profiles when an incorrect token has been previously provided.
@chris-rock