[Ovirt-devel] [PATCH server] Introduces a new IP address model. Moves IP addressing to a separate

[Darryl Pierce]

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

Mohammed Morsi wrote:
> NACK. Applies nicely (albeit with whitespace complaints) and everything
> seems to work with a few caveats as described below / inline.
<snip>
>>  src/db/migrate/025_create_ip_addresses.rb          |   46 +++++++++
>>   
> It seems since you've sent this out, migration 025 has been committed,
> so you'll need to start at 026

Fixed.

<snip>
>> +# +IpAddress+ is the base class for all address related classes.
>> +#
>> +class IpAddress < ActiveRecord::Base
>> +
>> +end
>>   
> Why isn't there a "has_many :nics" and a "has_many :bonding"
> relationship in the IpAddress table. We wont be able to lookup nics /
> bondings from an ip address otherwise.

An IpAddress will only belong to either one Nic or one Bonding, so a
has_many didn't seem logical here. And I didn't know of a scenario where
there would be a search on IP address that couldn't be done from the nic
or bonding level; i.e., "find all nics where ip_address_id in (find id
from ip_addresses where ...)" So to keep the IpAddress clean I left it out.

Since it doesn't require a DB change to do, if we do find a needed
reason for modeling on the IpAddress side, we can introduce it then.

<snip>

>> diff --git a/src/host-browser/host-browser.rb b/src/host-browser/host-browser.rb
>> index 79d34e8..bbfbf3b 100755
>> --- a/src/host-browser/host-browser.rb
>> +++ b/src/host-browser/host-browser.rb
>> @@ -283,10 +283,10 @@ class HostBrowser
>>  
>>                      updated_nic = Nic.find_by_id(nic.id)
>>  
>> -                    updated_nic.bandwidth = detail['BANDWIDTH']
>>   
> Did you really mean to remove bandwidth here? I still see it in the db
> table after the migrations and this seems to be the only place that a
> change to it occurs.

Weird, but the above's not missing. I didn't remove it intentionally,
and it's not removed from the code in my repo. I'll send out an updated
patch to make sure something didn't get munged.

<snip>

> I ran these tests and everything succeeded flawlessly. For the most
> part, besides the mentioned items, everything seemed to come together
> and work nicely. Good job.

I'm sending an updated patch. Please review it as soon as you can.

- --
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

iEYEARECAAYFAkkAZmIACgkQjaT4DmxOfxuCvgCdF71JAbGLhmp0giKyVI9LhhMv
SjEAnRnLx70haGOa0vBlEgClkBVo3UfF
=1lxu
-----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/20081023/1631d238/attachment.vcf>