[Ovirt-devel] [PATCH server] network integration into ovirt server db and wui

[Darryl Pierce]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Mohammed Morsi wrote:
>> Some of the first things I've noticed is that, by removing columns from
>> the bondings and nics tables into the separate IP-related tables,
>> updates need to be made to the ManagedNodeConfiguration class. Since the
>> data itself is the same and just relocated, refactoring the
>> configuration class to get data from the appropriate objects should take
>> care of things initially.
>>
>> Before this patch can be pushed, it needs to fix that class. The unit
>> test itself does not need changing since the data ought to still look
>> the same once encoded. There may be changes needed for vlans, but that
>> will be additional tests.
>>
> Did you say on irc you were going to take care of this
> ManagedNodeConfiguration bit or did I mishear?

We cam do it in two stages. First you can fix the existing functionality
to use the new domain modeling. That way we lose nothing on that end and
maintain our current feature set.

Then, afterward, I can take on the new functionality for vlans.

>>> +  def validate
>>> +    errors.add("name", "must be specified") unless name
>>> +    errors.add("interface_name", "must be specified") unless
>> interface_name
>>> +    errors.add("bonding_type_id", "must be specified") unless
>> bonding_type_id
>>
>> This is duplicating the above validation with the validates_presence_of
>> check.
> The thing I noticed with validates_presence_of is that I found that it
> doesn't seem to trigger the validation components of the interface. For
> example if name isn't specified in the wui which is caught with a
> validate_presence_of instead of in the validate function, the
> corresponding errors is never added to the 'errors' array and thus the
> 'name' textbox on the interface is never highlighted so that the user
> knows that he/she must provide that information.

Not sure I follow this. Can you try rephrasing it?

>>> +  protected
>>> +   def validate
>>> +    unless address and address =~ ADDRESS_TEST
>>> +      errors.add("address", "is of incorrect format")
>>> +    end
>>> +    unless !netmask or netmask == "" or netmask =~ ADDRESS_TEST
>>> +      errors.add("netmask", "is of incorrect format")
>>> +    end
>>> +    unless !gateway or gateway == "" or gateway =~ ADDRESS_TEST
>>> +      errors.add("gateway", "is of incorrect format")
>>> +    end
>>> +    unless !broadcast or broadcast == "" or broadcast =~ ADDRESS_TEST
>>> +      errors.add("broadcast", "is of incorrect format")
>>> +    end
>> In the original patch I submitted each of these fields was tested using
>> validates_format_of.
> Thing is, since ip address is now being used for network, bonding, and
> nic, some of those fields won't apply in all cases. Specifically only
> the 'address' field should be set for ip addresses associated with nics
> / bondings (setting the others won't break anything, they just won't be
> used) and only when the nic is associated with a statically assigned
> network. The other fields are only used when an ip address is associated
> with a network, thus it doesn't work to validate them for every instance.

validates_presence_of has an :if modifier that accepts a Proc and can be
used to toggle whether that validation is checked. So that will allow
you to check the relationships prior to enforcing the validation.

>> See note in the IpV4Address regarding validations. The Rails pattern is
>> to use validates_format_of for checking content.
> It is also a valid rails approach to use the validate method for
> validations.

True, but we should be consistent in our code.

>>> diff --git a/src/app/models/nic.rb b/src/app/models/nic.rb
>>> index baf7095..25d3ac2 100644
>>> --- a/src/app/models/nic.rb
>>> +++ b/src/app/models/nic.rb
>>> @@ -19,7 +19,19 @@
>>>  class Nic < ActiveRecord::Base
>>>    belongs_to :host
>>> -  belongs_to :boot_type
>>> +  belongs_to :physical_network
>>> +  has_many :ip_addresses, :dependent => :destroy
>>> -  has_and_belongs_to_many :bonding, :join_table => 'bondings_nics'
>>> +  has_and_belongs_to_many :bondings, :join_table => 'bondings_nics'
>>> +
>>> +  protected
>>> +   def validate
>>> +     errors.add("host_id", "must be specified") unless host_id
>>> +     errors.add("physical_network_id", "must be specified") unless
>> physical_network_id
>>
>> These should be done with validates_presence_of to keep the code
>> consistent throughout the project. The error message added here is the
>> default from Rails, so need not be specified.
>>
>>> +     if physical_network.boot_type.id ==
>>> +         BootType.find(:first, :conditions => { :proto => 'static'
>> }).id and
>>
>> You can test this without the second DB call using:
>>
>> if physical_network.boot_type.proto == static'
> 
> Good catch on this boot_type bit.
> 
> [snip]
> 
> I'll see what I can change based on this feedback. Once again I'm not
> sure that the validation bit will be able to be changed due to the
> aforementioned reasons.

Check out the validation docs [1] for some details on using them in
models. I think we can get what you need without being inconsistent.

[1] -
http://api.rubyonrails.com/classes/ActiveRecord/Validations/ClassMethods.html

- --
Darryl L. Pierce <dpierce at redhat.com> : GPG KEYID: 6C4E7F1B
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkkLIPEACgkQjaT4DmxOfxuzlgCdHMR26G+UR7ns6SHp+7iPkqZ2
lmMAniA/tuTPC/OHHv5O8JhPlgZl8l8e
=+LlI
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dpierce.vcf
Type: text/x-vcard
Size: 319 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/ovirt-devel/attachments/20081031/3b6b7580/attachment.vcf>