Fixes #30627 - mac address is not required for virtual resources #7895
Conversation
domitea
changed the title
app/models/nic/base.rb
Outdated
| @@ -45,6 +45,8 @@ class Base < ApplicationRecord | |||
| validates :subnet, :belongs_to_host_taxonomy => { :taxonomy => :organization } | |||
| validates :subnet6, :belongs_to_host_taxonomy => { :taxonomy => :organization } | |||
|
|
|||
| before_create check_blank_mac_for_virtual_resources | |||
There was a problem hiding this comment.
I don't think adding validation error on before_create will work, alsow, you're running the method right away, instead of using the symbol.
Why dont you use following?
| before_create check_blank_mac_for_virtual_resources | |
| validate :check_blank_mac_for_virtual_resources |
app/models/nic/base.rb
Outdated
| @@ -358,6 +360,12 @@ def interface_attribute_uniqueness(attr, base = Nic::Base.where(nil)) | |||
| db_candidates = db_candidates.select { |c| c.id != id && in_memory_candidates.map(&:id).include?(c.id) } | |||
| errors.add(attr, :taken) if db_candidates.present? | |||
| end | |||
|
|
|||
| def check_blank_mac_for_virtual_resources | |||
| if host.compute_provides?(:mac) && mac | |||
There was a problem hiding this comment.
I'd prefer to be more verbose here:
EDIT: see Tomer's suggestion (I'd prefer his suggestion - present? over mine - !emtpy?)
There was a problem hiding this comment.
| if host.compute_provides?(:mac) && mac | |
| if host.compute_provides?(:mac) && mac.present? |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
app/models/nic/base.rb
Outdated
|
|
||
| def check_blank_mac_for_virtual_resources | ||
| if host.compute_provides?(:mac) && mac.present? | ||
| errors.add(:mac, _("can't be set for this compute resource because it's provided")) |
There was a problem hiding this comment.
| errors.add(:mac, _("can't be set for this compute resource because it's provided")) | |
| errors.add(:mac, _("can't be set for this interface because it's provided by the compute resource")) |
There was a problem hiding this comment.
@domitea could you change this, please?
|
Oh, the test failures are related, the factory |
|
[test foreman] |
|
tests are broken due to #7926 we need to wait for that and rebase on top of it. |
domitea
force-pushed
the
30627_fix
branch
3 times, most recently
from
5bcfe18
to
53bdf53
Compare
| } | ||
| end | ||
|
|
||
|
|
There was a problem hiding this comment.
Layout/EmptyLines: Extra blank line detected.
|
@ezr-ondrej after struggling with tests I think that PR is ready to merge. |
This fix is complementary to one of previous PR (#7366) where the field for enter MAC address in interface form of host detail is locked due to compute resource that is virtual resource (like AWS, Openstack... in that case MC address is provided).
So this PR only extends this behavior to API (and Hammer) related creation of host or interface. It add error where the mac address is set by request but it's not required due type of resource.