[Ovirt-devel] [PATCH server] network integration into ovirt server db and wui
Mohammed Morsi
mmorsi at redhat.com
Fri Oct 31 15:19:51 UTC 2008
Scott Seago wrote:
>
> Mohammed Morsi wrote:
>> diff --git a/src/app/controllers/host_controller.rb
>> b/src/app/controllers/host_controller.rb
>> index a40d297..120fe2a 100644
>> --- a/src/app/controllers/host_controller.rb
>> +++ b/src/app/controllers/host_controller.rb
>> @@ -156,6 +156,16 @@ class HostController < ApplicationController
>> render :json => @json_hash
>> end
>>
>> + def edit_network
>> + render :layout => 'popup'
>> + end
>> +
>> + def bondings_json
>> + bondings = Host.find(params[:id]).bondings
>> + bondings_json = []
>> + bondings.each{ |x| bondings_json.push({:id => x.id, :name =>
>> x.name}) }
>> + render :json => bondings_json
>> + end
>>
> This is a really minor comment, but this could be written more
> compactly as:
>
> + def bondings_json
> + bondings = Host.find(params[:id]).bondings
> + render :json => bondings.collect{ |x| {:id => x.id, :name =>
> x.name} + end
>
>>
>>
>> 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
>
>>
>> - has_and_belongs_to_many :nics,
>> - :join_table => 'bondings_nics',
>> - :foreign_key => :bonding_id
>>
>> end
>> diff --git a/src/app/models/physical_network.rb
>> b/src/app/models/physical_network.rb
>> new file mode 100644
>> index 0000000..cba696a
>> --- /dev/null
>> +++ b/src/app/models/physical_network.rb
>> @@ -0,0 +1,27 @@
>> +# Copyright (C) 2008 Red Hat, Inc.
>> +# Written by Mohammed Morsi <mmorsi at redhat.com>
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; version 2 of the License.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write to the Free Software
>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> +# MA 02110-1301, USA. A copy of the GNU General Public License is
>> +# also available at http://www.gnu.org/copyleft/gpl.html.
>> +
>> +class PhysicalNetwork < Network
>> + has_many :nics
>> +
>> + protected
>> + def validate
>> + errors.add("name", "must be specified") unless name
>> + errors.add("boot_type_id", "must be specified") unless boot_type_id
>> + end
>> +end
>>
> Why not use validates_presence_of for these?
>
>> diff --git a/src/app/models/vlan.rb b/src/app/models/vlan.rb
>> new file mode 100644
>> index 0000000..2b96502
>> --- /dev/null
>> +++ b/src/app/models/vlan.rb
>> @@ -0,0 +1,30 @@
>> +# Copyright (C) 2008 Red Hat, Inc.
>> +# Written by Mohammed Morsi <mmorsi at redhat.com>
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; version 2 of the License.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write to the Free Software
>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> +# MA 02110-1301, USA. A copy of the GNU General Public License is
>> +# also available at http://www.gnu.org/copyleft/gpl.html.
>> +
>> +class Vlan < Network
>> + has_many :bondings
>> +
>> + validates_presence_of :number
>> +
>> + protected
>> + def validate
>> + errors.add("name", "must be specified") unless name
>> + errors.add("number", "must be specified") unless number
>> + errors.add("boot_type_id", "must be specified") unless boot_type_id
>> + end
>> +end
>>
> same comment on validates_presence_of here
>
>> 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
More information about the ovirt-devel
mailing list