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

[Scott Seago]

Mohammed Morsi wrote:
> Scott Seago wrote:
>   
>> Mohammed Morsi wrote:
>>     
>>
>>>  
>>>  
>>> diff --git a/src/app/models/bonding.rb b/src/app/models/bonding.rb
>>> index 006c261..a3ad150 100644
>>> --- a/src/app/models/bonding.rb
>>> +++ b/src/app/models/bonding.rb
>>> @@ -30,6 +30,16 @@
>>>  # interface. They can be ignored if not used.
>>>  #
>>>  class Bonding < ActiveRecord::Base
>>> +  belongs_to :host
>>> +  belongs_to :bonding_type
>>> +  belongs_to :vlan
>>> +  has_many :ip_addresses, :dependent => :destroy
>>> +
>>> +  has_and_belongs_to_many :nics,
>>> +    :join_table  => 'bondings_nics',
>>> +    :foreign_key => :bonding_id
>>> +
>>> +
>>>    validates_presence_of :name,
>>>      :message => 'A name is required.'
>>>  
>>> @@ -42,18 +52,17 @@ class Bonding < ActiveRecord::Base
>>>    validates_presence_of :interface_name,
>>>      :message => 'An interface name is required.'
>>>  
>>> -  validates_presence_of :boot_type_id,
>>> -    :message => 'A boot type must be specified.'
>>> -
>>>    validates_presence_of :bonding_type_id,
>>>      :message => 'A bonding type must be specified.'
>>>  
>>> -  belongs_to :host
>>> -  belongs_to :bonding_type
>>> -  belongs_to :boot_type
>>> + protected
>>> +  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
>>> +    errors.add("host_id", "must be specified") unless host_id
>>> +    errors.add("vlan_id", "must be specified") unless vlan_id
>>> +  end
>>>   
>>>       
>> Why not just use validates_presence_of for these? This seems like the
>> standard out-of-the-box use case for validates_presence_of. Also, it
>> looks like name and interface_name have the valiadition in both places
>> -- validates_presence_of and validate.
>>     
> See my comments to Darryl's response to my patch. I am just concerned
> the 'validates_presence_of' doesn't add anything to the errors array
> which is needed by the wui. If this concern is unfound, then I can
> remove the validations and simply use validates_presence_of
>
>
>   
It definitely adds it to the errors array, so both ways of doing this 
should be equivalent.


>
>>     
>>> diff --git a/src/app/views/dashboard/index.html.erb
>>> b/src/app/views/dashboard/index.html.erb
>>> index 92cb4da..8815ebc 100644
>>> --- a/src/app/views/dashboard/index.html.erb
>>> +++ b/src/app/views/dashboard/index.html.erb
>>> @@ -13,6 +13,13 @@
>>>              </table>
>>>            </div>
>>>  
>>> +         <% if @can_modify %>
>>> +          <h3>Networks</h3>
>>> +          <a href="<%= url_for :controller => 'network', :action =>
>>> 'list' %>">
>>> +             View / Edit
>>> +          </a>
>>> +         <% end %>
>>> +
>>>            </div> <!-- end #tools -->
>>>    </td>   
>>>       
>> I'm not sure adding this to the currently-empty dashboard page is the
>> right place -- although I realize the discussions on what to do with
>> this were somewhat inconclusive. Here's another thought. Since it
>> doesn't really belong to a "dashboard" per se -- and we're not sure
>> that the dashboard needs multiple tabs, could we rename the nav column
>> to something more generic (like "Home" or something). and then have
>> Dashboard as ths first tab, Networks as another tab (if you have
>> permissions on them), etc.?
>>
>> although this doesn't necessarily have to be done for this patch since
>> it's more of a general UI cleanup task.
>>
>> The other issue here is that this breaks the left nav state since it's
>> a standard http link instead of an ajax request. We should probably
>> address that at the same time we place this on a tab in the UI
>>     
>>>  
>>> diff --git a/src/app/views/host/edit_network.rhtml
>>> b/src/app/views/host/edit_network.rhtml
>>> new file mode 100644
>>> index 0000000..7ec3180
>>> --- /dev/null
>>> +++ b/src/app/views/host/edit_network.rhtml
>>>   
>>>       
>> For edit, we should remove the ability to change the type.
>>
>> Also the network details pane needs to show a list of associated IP
>> addresses, Number (for vlans) and usages.
>>
>> And the 'new network' screen doesn't allow you to set usages either.
>>
>>     
>>> diff --git a/src/app/views/host/show.rhtml
>>> b/src/app/views/host/show.rhtml
>>> index d1b05ad..bc39a62 100644
>>> --- a/src/app/views/host/show.rhtml
>>> +++ b/src/app/views/host/show.rhtml
>>> @@ -17,6 +17,10 @@
>>>          <%= image_tag "icon_x.png" %> Clear VMs
>>>        </a>      <% end -%>
>>> +    <%= link_to image_tag("icon_edit.png") +"Edit Network",
>>> +                 {:controller => 'host',
>>> +                  :action => 'edit_network', :id => @host.id},
>>> +                 :rel=>"facebox[.bolder]",
>>> :class=>"selection_facebox" %>
>>>    <%- end -%>
>>>  <%- end -%>
>>>  <script type="text/javascript">
>>>   
>>>       
>> The edit network link here lets me add new bonded interfaces and edit
>> existing ones. The 'NICs' pulldown doesn't have an option to create a
>> new one.
>> Also the details pane doesn't show details on NICs or Bonded interfaces.
>>     
> Is creating a new nic valid? I was under the impression that nics were
> physical devices, eg a nic gets added when its actually attached to the
> host and the node communicates to the server that a new nic has  been
> added and should be added to the db.
>
>    -Mo
>   
Ahh, good point. host-browser should be putting in the NICs. Never mind 
the 'new NIC' comment then.

Scott