Fixes #14330 - Provide option in hammer to change display type... #3363
Conversation
|
|
||
| test "should not update display_type for non-Libvirt compute resource" do | ||
| cr = compute_resources(:openstack) | ||
| put :update, { :id => cr.id, :compute_resource => { :display_type => 'SPICE' } } |
There was a problem hiding this comment.
This test is silently failing, as it's missing an assert_response :success (ditto the ones above, actually). The difference between it and set_console_password is that the latter has a no-op method in ComputeResource, while display_type doesn't so it'll fail with an unknown method/attr error.
There was a problem hiding this comment.
fixed - not sure why github didn't hide this comment already
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@adamruzicka Thanks for this fix! It works fine, however, can you validate the user input is correct and add a test for it? I just tried |
| test "should not update display_type for non-Libvirt compute resource" do | ||
| cr = compute_resources(:openstack) | ||
| put :update, { :id => cr.id, :compute_resource => { :display_type => 'SPICE' } } | ||
| assert_response :error |
There was a problem hiding this comment.
This is an internal server error isn't it? Not a bad request response. Shouldn't it have the same behaviour as set_console_password?
libvirt compute resource
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@dLobatog good point, fixed it. |
| @@ -262,6 +262,15 @@ def set_console_password=(setpw) | |||
| self.attrs[:setpw] = nil | |||
| end | |||
|
|
|||
| # this method is overwritten for Libvirt | |||
There was a problem hiding this comment.
If this method is only used in libvirt, why do you include it here? You can also put the attr_accessible just in the Libvirt model
There was a problem hiding this comment.
to make setting display_type behave in a same way as setting set_console_password (successfully do nothing when you try to update the attribute on unsupported resource)
|
Merged as e52bc50, thanks @adamruzicka! |
...for libvirt compute resource