[Ovirt-devel] [PATCH server 07/10] use service layer for Network controller.

[David Lutterkort]

On Tue, 2009-05-19 at 14:23 +0000, Scott Seago wrote:
> There's still room for additional refactoring as we may want to
> eventually split out nic, bonding, and ip address manipulation into
> their own controller/svc module, but that would be a much more
> extensive redesign than we want to deal with for the overall service
> layer creation refactoring.
> 
> Signed-off-by: Scott Seago <sseago at redhat.com>
> ---
>  src/app/controllers/network_controller.rb |  521 ++++++-----------------------
>  src/app/models/ip_address.rb              |   11 +
>  src/app/models/network.rb                 |   12 +
>  src/app/models/physical_network.rb        |    4 +
>  src/app/models/vlan.rb                    |    5 +
>  src/app/services/network_service.rb       |  392 ++++++++++++++++++++++
>  6 files changed, 534 insertions(+), 411 deletions(-)
>  create mode 100644 src/app/services/network_service.rb

ACK with some minor changes; given the number of variations for
networking, I only spot-tested some of them. Seems to work fine, though.

> diff --git a/src/app/controllers/network_controller.rb b/src/app/controllers/network_controller.rb
> index 244d041..5af0a73 100644
> --- a/src/app/controllers/network_controller.rb
> +++ b/src/app/controllers/network_controller.rb
> @@ -18,471 +18,170 @@
>  #
>  
>  class NetworkController < ApplicationController

> +  def list
> +    svc_list()

Drop the '()' - looks very unrubyish.
 
> -   def new
> +  def new
> +    svc_new()

Drop the '()' here, too.

> +  def delete
> +    network_ids = params[:network_ids].split(",")
> +    successes = []
> +    failures = {}
> +    network_ids.each do |network_id|
> +      begin
> +        svc_destroy(network_id)
> +        successes << @network
> +      rescue PermissionError => perm_error
> +        failures[@network] = perm_error.message
> +      rescue ActionError => ex
> +        failures[@network] = ex.message
> +      rescue Exception => ex
> +        failures[@network] = ex.message
> +      end

This assumes that svc_destroy always sets @network; and since we rescue
Exception, there's no need for the other rescue clauses.

> diff --git a/src/app/models/ip_address.rb b/src/app/models/ip_address.rb
> index 5d2e6af..4e4c7d3 100644
> --- a/src/app/models/ip_address.rb
> +++ b/src/app/models/ip_address.rb
> @@ -24,4 +24,15 @@ class IpAddress < ActiveRecord::Base
>     belongs_to :network
>     belongs_to :nic
>     belongs_to :bonding
> +
> +  def self.factory(params = {})
> +    case params[:type]
> +    when "IpV4Address"
> +      return IpV4Address.new(params)
> +    when "IpV6Address"
> +      return IpV6Address.new(params)
> +    else
> +      return nil
> +    end
> +  end

It would be better to raise a noisy exception if the address type is
unknown (since it's a programming error).

>  end
> diff --git a/src/app/models/network.rb b/src/app/models/network.rb
> index 0ad38bb..1c6fe97 100644
> --- a/src/app/models/network.rb
> +++ b/src/app/models/network.rb
> @@ -31,4 +31,16 @@ class Network < ActiveRecord::Base
>    validates_presence_of :boot_type_id,
>      :message => 'A boot type must be specified.'
>  
> +  def self.factory(params = {})
> +    case params[:type]
> +    when 'PhysicalNetwork'
> +      return PhysicalNetwork.new(params)
> +    when 'Vlan'
> +      return Vlan.new(params)
> +    else
> +      return nil
> +    end
> +  end

Here, too, a noisy death would be preferrable.

> diff --git a/src/app/services/network_service.rb b/src/app/services/network_service.rb
> new file mode 100644
> index 0000000..551b60c
> --- /dev/null
> +++ b/src/app/services/network_service.rb

> +  # Load a new Network for creating
> +  #
> +  # === Instance variables
> +  # [<tt>@network</tt>] loads a new Network object into memory
> +  # === Required permissions
> +  # [<tt>Privilege::MODIFY</tt>] on default HW pool
> +  def svc_new()
> +    authorize
> +  end

@network is never set here. The UI doesn't seem to need it, so it's
enough to drop the comment about setting @network.

> +  # Load the IP addresses for object of type +parent_type+ and +id+
> +  #
> +  # === Instance variables
> +  # [<tt>@parent_type</tt>] +parent_type+
> +  # [<tt>@ip_addresses</tt>] IP addresses for selected object
> +  # [<tt>@network</tt>] network with +id+ if <tt>@parent_type</tt> is network
> +  # [<tt>@nic</tt>] nic with +id+ if <tt>@parent_type</tt> is nic
> +  # [<tt>@bonding</tt>] bonding with +id+ if <tt>@parent_type</tt> is bonding

Please put "network", "nic" and "bonding" in quotes to make it clear
that parent_type should be a string.

David