[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Ovirt-devel] [PATCH] Ruby interface for Cobbler XML-RPC APIs.



On Thu, 2008-07-31 at 17:25 -0400, Darryl L. Pierce wrote:
> The interface is stored in a module named Cobbler.
> Examples for using the interface are stored in ./examples.

Neat

> Still looking for some feedback from anybody with some experience with Ruby.
> I'm now working on the save functionality. I'm playing with having each child
> class declare a saving script and passing it to the cobbler_save_method code
> generator. I'd like some input from anybody on that path.

Why not just decalre a save method straight up in each class ? I don't
see what the metaprogramming there buys you.

>  ruby/rubygem-cobbler/Rakefile                      |   30 ++++
>  ruby/rubygem-cobbler/cobbler.gemspec               |   37 +++++

Generally, people put hte gemspec right in the Rakefile; for an example
see ruby-libvirt's Rakefile

> new file mode 100755
> index 0000000..ead6609
> --- /dev/null
> +++ b/ruby/rubygem-cobbler/examples/create_system.rb
> @@ -0,0 +1,52 @@
> +#!/usr/bin/ruby -W0

What do you set the warning level ? That flag shouldn't be needed.

> --- /dev/null
> +++ b/ruby/rubygem-cobbler/examples/has_profile.rb

> + hostname = nil
> + profile  = nil

Don't use instance variables here .. just simple 'hostname' and
'profile' are better in a simple script. Same comment for the other
examples.

> diff --git a/ruby/rubygem-cobbler/lib/cobbler.rb b/ruby/rubygem-cobbler/lib/cobbler.rb
> new file mode 100644
> index 0000000..df78813
> --- /dev/null
> +++ b/ruby/rubygem-cobbler/lib/cobbler.rb
> @@ -0,0 +1,25 @@
> +# cobbler.rb - Cobbler module declaration.
> +# 
> +# Copyright (C) 2008 Red Hat, Inc.
> +# Written by Darryl L. Pierce <dpierce 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.
> +
> +require 'cobbler/system'

Commonly, the toplevel file in a ruby library requires all the files in
subdirectories, so that you can say "require 'cobbler'" and get systems,
profiles, distros etc.
 
> +module Cobbler
> +    
> +end

Nuke that ... it has no effect.

> diff --git a/ruby/rubygem-cobbler/lib/cobbler/base.rb b/ruby/rubygem-cobbler/lib/cobbler/base.rb
> new file mode 100644
> index 0000000..fdd696d
> --- /dev/null
> +++ b/ruby/rubygem-cobbler/lib/cobbler/base.rb
> @@ -0,0 +1,150 @@
> +# base.rb
> +# 
> +# Copyright (C) 2008 Red Hat, Inc.
> +# Written by Darryl L. Pierce <dpierce 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.
> + 
> +require 'xmlrpc/client'
> +require 'pp'
> +
> +module Cobbler
> +  include XMLRPC
> +  
> +  # +Base+ represents a remote Cobbler server.
> +  # 
> +  # Child classes can define fields that will be retrieved from Cobbler by 
> +  # using the +cobbler_field+ method. For example:
> +  # 
> +  #   class Farkle < Base
> +  #       cobbler_field :name, findable => 'get_farkle'
> +  #       cobbler_field :owner
> +  #   end
> +  #   
> +  # declares a class named Farkle that contains two fields. The first, "name",
> +  # will be one that is searchable on Cobbler; i.e., a method named "find_by_name"
> +  # will be generated and will use the "get_farkle" remote method to retrieve
> +  # that instance from Cobbler. 
> +  # 
> +  # The second field, "owner", will simply be a field named Farkle.owner that 
> +  # returns a character string.
> +  #
> +  # +Base+ provides some common functionality for all child classes:
> +  #
> +  #  
> +  class Base
> +    
> +    @@connection = nil
> +    @@hostname   = nil
> +    
> +    @attrs 
> +
> +    def attributes(name)
> +      return @attrs ? @attrs[name] : nil
> +    end

Attributes is kinda an overloaded name in Ruby - how about fields or
similar ? Also, should @attrs really be an instance variable rather than
a class variable ?

> +    # Makes a remote call to the Cobbler server.
> +    #
> +    def self.make_call(*args)
> +      conn = connection(false)
> +      
> +      conn.call(*args)
> +    end
> +    
> +    def self.connection=(connection)
> +      @@connection = connection
> +    end
> +    
> +    def self.connection(writable = false)
> +      result = @@connection
> +      
> +      unless result
> +        result = XMLRPC::Client.new2("http://#{@@hostname}/cobbler_api#{writable ? '_rw' : ''}")
> +      end
> +      
> +      return result
> +    end

Don't you want to cache the connection here, i.e. more something like

  def self.connection(writable = false)
     unless @@connection
       @@connection = XMLRPC::Client.new2("http://#{@@hostname}/cobbler_api#{writable ? '_rw' : ''}")
     end
      
     return @@connection
  end

It would be cleaner if you passed the connection into the constructor
(possibly wrapped in some convenience class);  having it as a class
variable is really strange - especially since it limits you to talking
to one cobbler server for the lifetime of the Ruby interpreter.

> +    class << self

My head is spinning: you should stick to one idiom or the other (i.e.,
either 'def self.foo' or 'class << self') Nice metaprogramming though.

> diff --git a/ruby/rubygem-cobbler/nbproject/private/private.properties b/ruby/rubygem-cobbler/nbproject/private/private.properties

That should not be in the patch.

> diff --git a/ruby/rubygem-cobbler/test/test_profile.rb b/ruby/rubygem-cobbler/test/test_profile.rb
> new file mode 100644
> index 0000000..01a8184
> --- /dev/null
> +++ b/ruby/rubygem-cobbler/test/test_profile.rb
> @@ -0,0 +1,83 @@
> +# test_profile.rb - Tests the Profile class.
> +# 
> +# Copyright (C) 2008 Red Hat, Inc.
> +# Written by Darryl L. Pierce <dpierce 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.
> + 
> +
> +$:.unshift File.join(File.dirname(__FILE__),'..','lib')
> +
> +require 'test/unit'
> +require 'flexmock/test_unit'
> +require 'cobbler/base'
> +require 'cobbler/profile'
> +
> +module Cobbler
> +  class TestProfile < Test::Unit::TestCase
> +    def setup
> +      @connection = flexmock('connection')
> +      Base.connection = @connection
> +      Base.hostname   = "localhost"
> +      
> +      @profiles = Array.new
> +      @profiles[0] = Hash.new
> +      @profiles[0]['profile']         = 'Fedora-9-i386'
> +      @profiles[0]['distro']          = 'Fedora-9-i386'
> +      @profiles[0]['dhcp tag']        = 'default'
> +      @profiles[0]['kernel options']  = {}
> +      @profiles[0]['kickstart']       = '/etc/cobbler/sample_end.ks'
> +      @profiles[0]['ks metadata']     = {}
> +      @profiles[0]['owners']          = ['admin']
> +      @profiles[0]['repos']           = []
> +      @profiles[0]['server']          = '<<inherit>>'
> +      @profiles[0]['virt bridge']     = 'xenbr0'
> +      @profiles[0]['virt cpus']       = '1'
> +      @profiles[0]['virt file size']  = '5'
> +      @profiles[0]['virt path']       = ''
> +      @profiles[0]['virt ram']        = '512'
> +      @profiles[0]['virt type']       = 'xenpv'

This can be written more compactly as

        @profiles << {
          'profile' => 'Fedora-9-i386',
          'distro' => ...
        }
        
David



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]