[Ovirt-devel] [PATCH server] network integration into ovirt server db and wui
Scott Seago
sseago at redhat.com
Fri Oct 31 14:59:05 UTC 2008
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.
>
> - 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.
Scott
More information about the ovirt-devel
mailing list