[V2V] Set default address for conversion host #18577
Conversation
|
@miq-bot bot add_label refactoring, transformation, hammer/yes |
djberg96
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@miq-bot add_label transformation, refactoring, hammer/yes |
|
Checked commits https://github.com/djberg96/manageiq/compare/383212bebfd681102cc200cb9dfe4f52d755d1c4~...70d4b72121dea7b8b2f13ba3eda7a037d696f4a0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/conversion_host.rb
|
|
@djberg96 the point of conversion_host.address was for an external conversion host (e.g. installed on a physical machine) where there isn't a linked resource. I don't like auto-setting this from the underlying resource because it will never change if the host/vm ip address changes. And since it will try self.address first in def ipaddresses it will always return the old address. Why set a default value here at all? |
| # Set the default address to the first IP address associated with the resource. | ||
| # | ||
| def default_address | ||
| self.address ||= self.ipaddress |
There was a problem hiding this comment.
I'm going to NAK based on this, we should be able to handle address not being set and this will break if the vm/host ipaddress changes.
There was a problem hiding this comment.
I guess the thing to do then is to not make the ipaddress address field mandatory, though it makes me wonder why we have it at all then.
There was a problem hiding this comment.
Why we have which field? :address?
There was a problem hiding this comment.
Yes. Seems like it should just be a virtual attribute. But, whatever, that's neither here nor there for now I suppose.
Well, originally it was because of the validation that it had to be present if the underlying resource had at least one ipaddress. But, since the UI doesn't let you set an IP it became a bit problematic. |
|
Closing in favor of #18610 |
Something of a followup to #18516, this again is based on feedback from the V2V team where it was decided that the address should default to the first IP address of the associated resource. There was already an internal
ipaddressmethod, so we just use that.Since the UI doesn't (currently) allow users to specify an IP, attempts to connect to the conversion host will fail without this atm.
https://bugzilla.redhat.com/show_bug.cgi?id=1622728