Hi,
A several minor comments below.
On 29/01/10 21:34, Jesse Wolfe wrote:
> This patch re-implements the status() remote procedure as a REST interface.
>
> A running server returns key-value pairs, currently the only implemented
> key is "is_alive" which will always be set to true.
>
> Some future tool will consume this by:
> Puppet::Status.indirection.terminus_class = :rest
> Puppet::Status.find('https://puppet:8140/production/status/default')
>
> Now with unit tests.
> plus fixes a typo.
> plus integration test and default security setting.
>
> Signed-off-by: Jesse Wolfe <[email protected]>
> ---
> lib/puppet.rb | 1 +
> lib/puppet/indirector/status.rb | 3 +++
> lib/puppet/indirector/status/local.rb | 7 +++++++
> lib/puppet/indirector/status/rest.rb | 5 +++++
> lib/puppet/network/http/api/v1.rb | 10 +++++++++-
> lib/puppet/network/rest_authconfig.rb | 1 +
> lib/puppet/status.rb | 20 ++++++++++++++++++++
> spec/integration/bin/puppetmasterd.rb | 12 +++++++++++-
> spec/unit/status.rb | 23 +++++++++++++++++++++++
> 9 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 lib/puppet/indirector/status.rb
> create mode 100644 lib/puppet/indirector/status/local.rb
> create mode 100644 lib/puppet/indirector/status/rest.rb
> create mode 100644 lib/puppet/status.rb
> create mode 100644 spec/unit/status.rb
>
> diff --git a/lib/puppet.rb b/lib/puppet.rb
> index a78853a..8fe7853 100644
> --- a/lib/puppet.rb
> +++ b/lib/puppet.rb
> @@ -171,6 +171,7 @@ require 'puppet/ssl'
> require 'puppet/module'
> require 'puppet/util/storage'
> require 'puppet/parser/interpreter'
> +require 'puppet/status'
>
> if Puppet[:storeconfigs]
> require 'puppet/rails'
> diff --git a/lib/puppet/indirector/status.rb b/lib/puppet/indirector/status.rb
> new file mode 100644
> index 0000000..f40bbc4
> --- /dev/null
> +++ b/lib/puppet/indirector/status.rb
> @@ -0,0 +1,3 @@
> +# A stub class, so our constants work.
> +class Puppet::Indirector::Status
> +end
> diff --git a/lib/puppet/indirector/status/local.rb
> b/lib/puppet/indirector/status/local.rb
> new file mode 100644
> index 0000000..377be89
> --- /dev/null
> +++ b/lib/puppet/indirector/status/local.rb
> @@ -0,0 +1,7 @@
> +require 'puppet/indirector/status'
> +
> +class Puppet::Indirector::Status::Local < Puppet::Indirector::Code
> + def find( *anything )
> + return model.new
> + end
> +end
> diff --git a/lib/puppet/indirector/status/rest.rb
> b/lib/puppet/indirector/status/rest.rb
> new file mode 100644
> index 0000000..22e7042
> --- /dev/null
> +++ b/lib/puppet/indirector/status/rest.rb
> @@ -0,0 +1,5 @@
> +require 'puppet/indirector/status'
> +require 'puppet/indirector/rest'
> +
> +class Puppet::Indirector::Status::Rest < Puppet::Indirector::REST
> +end
The other indirector rest implementation also have (very simple) unit
tests, just checking the class currently inherits Puppet::Indirector::REST.
> diff --git a/lib/puppet/network/http/api/v1.rb
> b/lib/puppet/network/http/api/v1.rb
> index 13df7c3..6a5ff15 100644
> --- a/lib/puppet/network/http/api/v1.rb
> +++ b/lib/puppet/network/http/api/v1.rb
> @@ -34,7 +34,7 @@ module Puppet::Network::HTTP::API::V1
> end
>
> def indirection2uri(request)
> - indirection = request.method == :search ?
> request.indirection_name.to_s + "s" : request.indirection_name.to_s
> + indirection = request.method == :search ?
> pluralize(request.indirection_name.to_s) : request.indirection_name.to_s
>
> "/#{request.environment.to_s}/#{indirection}/#{request.escaped_key}#{request.query_string}"
> end
>
> @@ -50,12 +50,20 @@ module Puppet::Network::HTTP::API::V1
> return method
> end
>
> + def pluralize(indirection)
> + return "statuses" if indirection == "status"
> + return indirection + "s"
> + end
> +
> def plurality(indirection)
> # NOTE This specific hook for facts is ridiculous, but it's a
> *many*-line
> # fix to not need this, and our goal is to move away from the
> complication
> # that leads to the fix being too long.
> return :singular if indirection == "facts"
>
> + # "status" really is singular
> + return :singular if indirection == "status"
> +
> result = (indirection =~ /s$/) ? :plural : :singular
>
> indirection.sub!(/s$/, '') if result
I know the code is simple, but covering this with a test would prevent
some issues in the future.
> diff --git a/lib/puppet/network/rest_authconfig.rb
> b/lib/puppet/network/rest_authconfig.rb
> index 635ed1b..01ed412 100644
> --- a/lib/puppet/network/rest_authconfig.rb
> +++ b/lib/puppet/network/rest_authconfig.rb
> @@ -15,6 +15,7 @@ module Puppet
> { :acl => "/certificate/ca", :method => :find, :authenticated =>
> false },
> { :acl => "/certificate/", :method => :find, :authenticated =>
> false },
> { :acl => "/certificate_request", :method => [:find, :save],
> :authenticated => false },
> + { :acl => "/status", :method => [:find], :authenticated => true
> },
> ]
You should modify spec/unit/network/rest_authconfig.rb too.
> def self.main
> diff --git a/lib/puppet/status.rb b/lib/puppet/status.rb
> new file mode 100644
> index 0000000..f587a5a
> --- /dev/null
> +++ b/lib/puppet/status.rb
> @@ -0,0 +1,20 @@
> +require 'puppet/indirector'
> +
> +class Puppet::Status
> + extend Puppet::Indirector
> + indirects :status, :terminus_class => :local
> +
> + attr :status, true
> +
> + def initialize( status = nil )
> + @status = status || {"is_alive" => true}
> + end
> +
> + def to_pson
> + @status.to_pson
> + end
> +
> + def self.from_pson( pson )
> + self.new( pson )
> + end
> +end
> diff --git a/spec/integration/bin/puppetmasterd.rb
> b/spec/integration/bin/puppetmasterd.rb
> index f1d77ef..45504c4 100755
> --- a/spec/integration/bin/puppetmasterd.rb
> +++ b/spec/integration/bin/puppetmasterd.rb
> @@ -85,7 +85,17 @@ describe "puppetmasterd" do
> FileTest.exist?(@pidfile).should be_true
> end
>
> - it "should be serving status information over REST"
> + it "should be serving status information over REST" do
> + start
> + sleep 6
> +
> + Puppet::Status.indirection.terminus_class = :rest
> + status =
> Puppet::Status.find("https://localhost:#{@@port}/production/status/default";)
> +
> + status.status["is_alive"].should == true
> +
> + Puppet::Status.indirection.terminus_class = :local
> + end
>
> it "should be serving status information over xmlrpc" do
> start
> diff --git a/spec/unit/status.rb b/spec/unit/status.rb
> new file mode 100644
> index 0000000..b13b246
> --- /dev/null
> +++ b/spec/unit/status.rb
> @@ -0,0 +1,23 @@
> +#!/usr/bin/env ruby
> +
> +require File.dirname(__FILE__) + '/../spec_helper'
> +
> +describe Puppet::Status do
> + it "should implement find" do
> + Puppet::Status.find( :default ).should be_is_a(Puppet::Status)
> + Puppet::Status.find( :default ).status["is_alive"].should == true
> + end
> +
> + it "should default to is_alive is true" do
> + Puppet::Status.new.status["is_alive"].should == true
> + end
> +
> + it "should return a pson hash" do
> + Puppet::Status.new.status.to_pson.should == '{"is_alive":true}'
> + end
> +
> + it "should accept a hash from pson" do
> + status = Puppet::Status.new( { "is_alive" => false } )
> + status.status.should == { "is_alive" => false }
> + end
> +end
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
