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

http_rescue not required with tempfile #59

Merged
merged 2 commits into from Jun 9, 2016

Conversation

Projects
None yet
7 participants
@Anirudh-Gupta
Contributor

Anirudh-Gupta commented Jun 7, 2016

Issues Resolved

Throwing error at line 40(libraries->helper.rb) because its not a http request(when integrating with chef server). No response code.

@mhedgpeth

This comment has been minimized.

Contributor

mhedgpeth commented Jun 7, 2016

I'm having this problem as well.

@jejohns

This comment has been minimized.

jejohns commented Jun 9, 2016

+1 Thank you!

@cheeseplus

This comment has been minimized.

Collaborator

cheeseplus commented Jun 9, 2016

+1 - adding some detail here from a user's stacktrace that may be related

https://gist.github.com/cheeseplus/83662a441df97ee6f7b5c15454df2405

@smurawski

This comment has been minimized.

Contributor

smurawski commented Jun 9, 2016

Thanks for the PR @Anirudh-Gupta and the confirmations @mhedgpeth @jejohns and @cheeseplus

@smurawski smurawski merged commit 0c69c35 into chef-cookbooks:master Jun 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeremymv2

This comment has been minimized.

Contributor

jeremymv2 commented Jun 9, 2016

I don't believe this should have been merged as is. The problem is that it short-circuits the error response code messages by no longer using the with_http_rescue method, which handles errors in the handle_http_error_code method. Now the error handling will no longer occur.

https://github.com/chef-cookbooks/audit/blob/master/libraries/helper.rb#L40

@itmustbejj

This comment has been minimized.

itmustbejj commented Jun 9, 2016

@jeremymv2 As I understand the issue that was addressed, rest.binmode_streaming_request returns a Tempfile object, not an HttpResponse object (probably butchering actual class names). So there is no code attribute to check on the Tempfile object. I think with_http_rescue should not have been there in the first place.

@itmustbejj

This comment has been minimized.

itmustbejj commented Jun 9, 2016

@jeremymv2 Looks like it was introduced unintentionally when the code was rewritten here: 0eeefc5 and de81f6f

@smurawski

This comment has been minimized.

Contributor

smurawski commented Jun 9, 2016

#62 re-introduces the with_http_rescue since there is an http call in the mix - however, it checks the output object to see if it has a response code before dropping into the specific http response handling code. This should return the file object unimpeded and maintain the troubleshooting benefits of helpful http errors.

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