[Ovirt-devel] [PATCH] Ruby interface for Cobbler XML-RPC APIs.
David Lutterkort
dlutter at redhat.com
Fri Aug 1 02:21:52 UTC 2008
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
> + at hostname = nil
> + at 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 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.
> +
> +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 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.
> +
> +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 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.
> +
> +
> +$:.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
More information about the ovirt-devel
mailing list