only return response from Exceptions that have it

[evgeni]

Only RestClient::ExceptionWithResponse has .response, while a "normal"
RestClient::Exception does not.
But by catching RestClient::Exception we might swallow errors that would
be useful for users in the logs, like
RestClient::SSLCertificateNotVerified

[evgeni]

compare to e.g.

foreman_rh_cloud/lib/foreman_inventory_upload/async/remove_insights_hosts_job.rb

Lines 45 to 56 in d642c75

	def delete_page(host_uuids, organization)
	execute_cloud_request(
	organization: organization,
	method: :delete,
	url: ForemanInventoryUpload.hosts_by_ids_url(host_uuids),
	headers: {
	content_type: :json,
	}
	)
	rescue RestClient::ExceptionWithResponse => error_response
	error_response.response
	end

where this is done correctly

[ShimShtein]

Sounds good, but we need to take into account the proper reporting of errors without a response.

I suppose we will need to modify the reporting block:

foreman_rh_cloud/app/controllers/insights_cloud/api/machine_telemetries_controller.rb

Line 28 in d642c75

if @cloud_response.code >= 300

[evgeni]

Is that so?

That block is the whole reason I came here, as when you get a RestClient::SSLCertificateNotVerified, that block still calls .code but on nil (as error_response.response that we return here is nil)

After the patch, it correctly raises the exception and makes it "someone elses problem" ;)

[ShimShtein]

It does, but the error becomes 500 which is not correct.

We need to relay the original failure to the client. That's why this block exists.
The idea is that the method always returns a result and the forwarder relays the response to the client.

I think @chris1984 is working on this already.

[evgeni]

Oooh you mean catching that exception and reporting it to the user?
yeah, that'd improve the UX, sure.

(today, you too get a 500 error, for the undefined .code on nil, but with the patch at least the log is useful, which it wasn't before)