Fixes #16881 - Pass information from proxy error response #3935
Conversation
| @@ -3,12 +3,20 @@ class ProxyException < ::Foreman::WrappedException | |||
| attr_reader :url | |||
|
|
|||
| def initialize(url, exception, message, *params) | |||
| message += append_exception_message(exception) | |||
There was a problem hiding this comment.
Why append the message in this location and not in the message method? I think this will affect the error codes generated, which will now depend on the passed message and any text supplied in the smart proxy response.
There was a problem hiding this comment.
Shouldn't the error codes depend on this? Otherwise you're getting one error code for what could be very different proxy errors - here you could have errors coming from things as varied as MS DNS, libvirt, DHCP, BMC... which are different errors that require different solutions.
There was a problem hiding this comment.
Each proxy feature should already have a unique set of error codes (e.g. ProxyExceptions are unique within each ProxyAPI class), but I agree there are different errors that the error code wouldn't reflect.
I'm concerned though that the text from the proxy may not be standardised - if it's used to generate the error code then the codes may not be reproducible at all, which is probably worse than them being too broad.
e.g. if the proxy returns an error stating it can't contact 192.168.0.100, then an error code generated based on that IP isn't useful to anybody except that one user, or ditto with file paths (cannot open file /srv/tftpboot/ or /var/lib/tftpboot).
There was a problem hiding this comment.
Hmmm fair enough. I just realized after your comment that the error I did this for had to do with MAC addresses so it wouldn't be useful to have a different code per MAC 😄 I'll fix it, thanks @domcleal
domcleal
added
Waiting on contributor
Not yet reviewed
and removed
Not yet reviewed
Waiting on contributor
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Updated so that the message is added at runtime |
| :code => 400) | ||
| proxy_exception = ProxyAPI::ProxyException.new( | ||
| 'fakeurl', | ||
| OpenStruct.new(:message => 'some error'), |
There was a problem hiding this comment.
Shouldn't this second parameter be the fake_exception, rather than using expects(:exception) below?
| @@ -8,7 +8,15 @@ def initialize(url, exception, message, *params) | |||
| end | |||
|
|
|||
| def message | |||
| super + ' ' + _('for proxy') + ' ' + url | |||
| super + ' ' + proxy_exception_response(exception) + | |||
There was a problem hiding this comment.
Nitpick, remove the extra space before proxy_exception_response as it'll cause a space before the colon and double-space otherwise.
| OpenStruct.new(:message => 'some error'), | ||
| 'unable to do something') | ||
| proxy_exception.expects(:exception).returns(fake_exception) | ||
| assert_match(/useful message from proxy/, proxy_exception.message) |
There was a problem hiding this comment.
It'd be useful if this could test the entire message - or more of the concatenation, not only the proxy_exception_response bit.
| assert_match(/useful message from proxy/, proxy_exception.message) | ||
| end | ||
|
|
||
| test 'does not fail if response body is not available' do |
There was a problem hiding this comment.
I think this needs to test #message, not just #url.
Certain information that could be important during creation of hosts should be displayed - currently the ProxyAPI abstracts the entire exception message away. This commit appends the message that comes from the proxy if there is one to show it whenever a ProxyAPI::ProxyException is thrown
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@domcleal I've updated this according to your comments, thanks for the review! |
| private | ||
|
|
||
| def proxy_exception_response(exception) | ||
| return '' unless exception.respond_to?(:response) |
There was a problem hiding this comment.
I think exception here is either the same or a new instance of this exception. I think this needs to be wrapped_exception.
|
|
||
| class ProxyApiExceptionTest < ActiveSupport::TestCase | ||
| test 'exception response body is added if available' do | ||
| fake_exception = OpenStruct.new(:response => 'useful message from proxy', |
There was a problem hiding this comment.
If you change this message here and in the expectation, e.g. s/message/response/ then the test will fail:
Expected /ERF.*ProxyException.*unable to do something.*useful response from proxy.*/ to match "ERF12-2902 [ProxyAPI::ProxyException]: unable to do something ([OpenStruct]: useful message from proxy) for proxy fakeurl".
The response isn't in the message.
|
Thank you for your contribution, @dLobatog! This PR has been inactive for 6 months, closing for now. |
Certain information that could be important during creation of hosts
should be displayed - currently the ProxyAPI abstracts the entire
exception message away.
This commit appends the message that comes from the proxy if there is
one to show it whenever a ProxyAPI::ProxyException is thrown