fixes #5896 - unable to set compute-resource's 'Console passwords' option in API

[isratrade]

No description provided.

[dLobatog]

👍

I think it'd be necessary to include a validation of attrs[:setpw] to force it to be nil when the Compute Resource is not Libvirt or Vmware and a test for that, or maybe .
I'm thinking users that set this value to true through the API accidentally on other Compute Resources where this doesn't work might be facing issues if we provide support for this on these Compute Resources later on.

[isratrade]

@elobato,
https://github.com/theforeman/foreman/blob/develop/app/models/compute_resource.rb#L220-221

If I force attrs[:setpw] to nil then, this returns true

  def set_console_password?
    self.attrs[:setpw] == 1 || self.attrs[:setpw].nil?
  end

Any reason why? It seems a bug in set_console_password?

[dLobatog]

It's not a bug, but it's certainly weird behavior. set_console_password? returns true when attrs[:setpw] = nil because when a new ComputeResource object is created, this trick 'ticks' the checkbox "Console Password" by default.

Force it to 0 instead which is apparently what submitting the form with it disabled does and seems to be the convention.

[isratrade]

@elobato, I refactored to remove the strange behavior when set_console_password? returns true if attrs[:setpw] is nil. The checkboxes are still 'ticked' in the form, but please confirm that all works as it should.

[domcleal]

This will cause an error if somebody passes a boolean into the API (which is what the API doc says), as to_i isn't defined on true/falseclass.

[dLobatog]

This isn't called through API calls, update_attributes updates self.attrs directly without calling this setter method.

[dLobatog]

I tested this and it works well in the UI, however since API calls won't call to the setter method you wrote*, you need to either make API calls check that setpw is 0 or 1 (and change it in the docs) or only use true/false, and convert the 0/1 coming from the checkbox to true/false. I think the latter would be cleaner and we would get rid of the getter/setter.
It would still require you make sure only true/false can be passed through API calls.

• I think it's because :setpw is inside attrs so it just 'mass-assigns' it. You can put whatever value in :setpw, even a string, and see it won't be checked.

[isratrade]

@elobato, I submitted a new commit (to be squashed) that addresses your commits and added functional api tests. A user can pass true/false in the API for attribute set_console_password which matches the documentation. However, any of these will work ['true', true, '1', 1]

[isratrade]

alias_method was also needed in STI class to work correctly, even though it appears in compute_resource.rb

[dLobatog]

Thanks, I tested it and this way it works well both through the API and the UI. One thing that concerns me is the duplication of code between libvirt.rb and vmware.rb.
I think we would benefit from making a module for shared code like that (or self.model_name, for instance, try to look for these duplicated methods, setup_key_pair in ec2.rb and openstack.rb...) and including it in all shared classes. It's probably ready to merge after that.

[isratrade]

@elobato, I re-committed with the common code re-factored in a mixin (need to squash commit)

[dLobatog]

Thanks, this is looking and working well. I meant to put a bit more shared stuff in mixins but that's for another PR. Merging in 3.. 2.. 1..

[dLobatog]

Merged as 553a0be , thanks @isratrade !