Fixing up the parsedtypes, fixing Type.eachtype to ignore structure types

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@822 980ebf18-57e1-0310-9a29-db15c13687c0

11 files changed, 197 insertions, 75 deletions

diff --git a/bin/puppetdoc b/bin/puppetdoc
index 00106ff72..4d4dba082 100755
--- a/bin/puppetdoc
+++ b/bin/puppetdoc
@@ -94,17 +94,22 @@ Meta-Parameters
 ---------------
 
 }
-params = []
-Puppet::Type.eachmetaparam { |param|
-    params << param
-}
+begin
+    params = []
+    Puppet::Type.eachmetaparam { |param|
+        params << param
+    }
 
-params.sort { |a,b|
-    a.to_s <=> b.to_s
-}.each { |param|
-    puts "- **" + param.to_s + "**"
-    puts tab(1) + Puppet::Type.metaparamdoc(param).gsub(/\n\s*/,' ')
-}
+    params.sort { |a,b|
+        a.to_s <=> b.to_s
+    }.each { |param|
+        puts "- **" + param.to_s + "**"
+        puts tab(1) + Puppet::Type.metaparamdoc(param).gsub(/\n\s*/,' ')
+    }
+rescue => detail
+    puts "type %s had incorrect params: %s" % detail
+    exit(1)
+end
 
 puts %{
 -----
diff --git a/lib/puppet/filetype.rb b/lib/puppet/filetype.rb
index 77d356f2a..5f505e9bf 100755
--- a/lib/puppet/filetype.rb
+++ b/lib/puppet/filetype.rb
@@ -38,12 +38,12 @@ module Puppet # :nodoc:
                     begin
                         val = real_read()
                         @loaded = Time.now
-                        return val
+                        return val.gsub(/# HEADER.*\n/,'')
                     rescue Puppet::Error => detail
                         raise
                     rescue => detail
-                        raise Puppet::Error, "%s could not read %s" %
-                            [self.class, @path]
+                        raise Puppet::Error, "%s could not read %s: %s" %
+                            [self.class, @path, detail]
                     end
                 end
 
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index eb0cc8282..9c5f17ac9 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -49,8 +49,6 @@ class Type < Puppet::Element
 
     # again, silence the tests; the :notused has to be there because it's
     # the namevar
-    @states = []
-    @parameters = [:notused]
     
     # @paramdoc = Hash.new
     
@@ -128,7 +126,7 @@ class Type < Puppet::Element
     def self.eachtype
         @@typeary.each do |type|
             # Only consider types that have names
-            if type.name
+            if ! type.parameters.empty? or ! type.validstates.empty?
                 yield type 
             end
         end
@@ -153,6 +151,10 @@ class Type < Puppet::Element
         @objects = Hash.new
         @aliases = Hash.new
 
+        unless defined? @parameters
+            @parameters = []
+        end
+
         @validstates = {}
 
         @paramdoc = Hash.new { |hash,key|
@@ -339,6 +341,7 @@ class Type < Puppet::Element
         unless defined? @validstates
             @validstates = Hash.new(false)
         end
+        return unless defined? @states
         @states.each { |stateklass|
             name = stateklass.name
             if @validstates.include?(name) 
@@ -357,6 +360,7 @@ class Type < Puppet::Element
     # Find the namevar
     def self.namevar
         unless defined? @namevar
+            return nil unless defined? @parameters and ! @parameters.empty?
             @namevar = @parameters.find { |name, param|
                 param.isnamevar?
                 unless param
@@ -463,6 +467,7 @@ class Type < Puppet::Element
 
     # Return the parameter names
     def self.parameters
+        return [] unless defined? @parameters
         @parameters.collect { |klass| klass.name }
     end
 
@@ -510,6 +515,7 @@ class Type < Puppet::Element
 
     # Return the list of validstates
     def self.validstates
+        return {} unless defined? @states
         unless @validstates.length == @states.length
             self.buildstatehash
         end
diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb
index eca07c10e..bcb5d56ae 100755
--- a/lib/puppet/type/cron.rb
+++ b/lib/puppet/type/cron.rb
@@ -240,10 +240,10 @@ module Puppet
         # Return the header placed at the top of each generated file, warning
         # users that modifying this file manually is probably a bad idea.
         def self.header
-%{#This file was autogenerated at #{Time.now} by puppet.  While it
-# can still be managed manually, it is definitely not recommended.
-# Note particularly that the comments starting with 'Puppet Name' should
-# not be deleted, as doing so could cause duplicate cron jobs.\n}
+%{# HEADER This file was autogenerated at #{Time.now} by puppet.  While it
+# HEADER can still be managed manually, it is definitely not recommended.
+# HEADER Note particularly that the comments starting with 'Puppet Name' should
+# HEADER not be deleted, as doing so could cause duplicate cron jobs.\n}
         end
 
         # Store a new instance of a cron job.  Called from Cron#initialize.
diff --git a/lib/puppet/type/parsedtype.rb b/lib/puppet/type/parsedtype.rb
index c4c64c021..14563cf57 100755
--- a/lib/puppet/type/parsedtype.rb
+++ b/lib/puppet/type/parsedtype.rb
@@ -9,14 +9,20 @@ module Puppet
         # the 'should' value to the 'is' value and to do support the right logging
         # and such.
         class ParsedParam < Puppet::State
-            @name = :parsedparam
+            # Fix things so that the fields have to match exactly, instead
+            # of only kinda
+            def insync?
+                self.is == self.should
+            end
+
             # Normally this would retrieve the current value, but our state is not
             # actually capable of doing so.
             def retrieve
                 # If we've synced, then just copy the values over and return.
                 # This allows this state to behave like any other state.
                 if defined? @synced and @synced
-                    @is = self.should
+                    # by default, we only copy over the first value.
+                    @is = @synced
                     @synced = false
                     return
                 end
@@ -32,30 +38,36 @@ module Puppet
             def sync(nostore = false)
                 ebase = @parent.class.name.to_s
                 if @is == :notfound
-                    @is = self.should
+                    #@is = self.should
                     tail = "created"
 
                     # If we're creating it, then sync all of the other states
                     # but tell them not to store (we'll store just once,
                     # at the end).
-                    @parent.eachstate { |state|
-                        next if state == self
-                        state.sync(true)
-                    }
+                    unless nostore
+                        @parent.eachstate { |state|
+                            next if state == self
+                            state.sync(true)
+                        }
+                    end
                 elsif self.should == :notfound
                     @parent.remove(true)
                     tail = "deleted"
+                #elsif @is == @should
                 elsif self.insync?
+                    self.info "already in sync"
                     return nil
                 else
                     #@is = self.should
                     # Mark that we've synced it, but don't copy the value, because
                     # that will make the 'change' log inscrutable.
-                    @synced = true
                     tail = "changed"
                 end
+                @synced = self.should
 
-                unless nostore
+                if nostore
+                    self.retrieve
+                else
                     @parent.store
                 end
                 
@@ -113,8 +125,8 @@ module Puppet
             # Return the header placed at the top of each generated file, warning
             # users that modifying this file manually is probably a bad idea.
             def self.header
-%{# This file was autogenerated at #{Time.now} by puppet.  While it
-# can still be managed manually, it is definitely not recommended.\n\n}
+%{# HEADER: This file was autogenerated at #{Time.now} by puppet.  While it
+# HEADER: can still be managed manually, it is definitely not recommended.\n}
             end
 
             # Store a new instance of a host.  Called from Host#initialize.
diff --git a/lib/puppet/type/parsedtype/host.rb b/lib/puppet/type/parsedtype/host.rb
index 4b60e7142..69c1dbe06 100755
--- a/lib/puppet/type/parsedtype/host.rb
+++ b/lib/puppet/type/parsedtype/host.rb
@@ -10,8 +10,8 @@ module Puppet
         end
 
         newstate(:aliases) do
-            desc "Any aliases the host might have.  Values can be either an array
-                or a comma-separated list."
+            desc "Any aliases the host might have.  Multiple values must be
+                specified as an array."
 
             # We have to override the feeding mechanism; it might be nil or 
             # white-space separated
@@ -31,14 +31,10 @@ module Puppet
                 @should
             end
 
-            munge do |values|
-                unless values.is_a?(Array)
-                    values = [values]
+            validate do |value|
+                if value =~ /\s/
+                    raise Puppet::Error, "Aliases cannot include whitespace"
                 end
-                # Split based on comma, then flatten the whole thing
-                values.collect { |values|
-                    values.split(/,\s*/)
-                }.flatten
             end
         end
 
@@ -100,7 +96,7 @@ module Puppet
         end
 
         # Convert the current object into a host-style string.
-        def to_str
+        def to_s
             str = "%s\t%s" % [self.state(:ip).should, self[:name]]
 
             if state = self.state(:alias)
diff --git a/lib/puppet/type/parsedtype/port.rb b/lib/puppet/type/parsedtype/port.rb
index 2080e1086..65549d7da 100755
--- a/lib/puppet/type/parsedtype/port.rb
+++ b/lib/puppet/type/parsedtype/port.rb
@@ -7,18 +7,19 @@ module Puppet
     newtype(:port, Puppet::Type::ParsedType) do
         newstate(:protocols) do
             desc "The protocols the port uses.  Valid values are *udp* and *tcp*.
-                Most services have both protocols, but not all."
+                Most services have both protocols, but not all.  If you want
+                both protocols, you must specify that; Puppet replaces the
+                current values, it does not merge with them.  If you specify
+                multiple protocols they must be as an array."
 
             def is=(proto)
                 unless proto.is_a?(Array)
-                    proto = [proto]
+                    proto = [proto.split(/\s+/)].flatten
                 end
-                #self.info "setting to %s" % proto.inspect
                 @is = proto
             end
 
             def is
-                #self.notice "returning is %s" % @is.inspect
                 @is
             end
 
@@ -27,6 +28,12 @@ module Puppet
             def should
                 @should
             end
+
+            validate do |value|
+                unless value == "udp" or value == "tcp"
+                    raise Puppet::Error, "Protocols can be either 'udp' or 'tcp'"
+                end
+            end
         end
 
         newstate(:number) do
@@ -38,8 +45,8 @@ module Puppet
         end
 
         newstate(:aliases) do
-            desc "Any aliases the port might have.  Values can be either an array
-                or a comma-separated list."
+            desc "Any aliases the port might have.  Multiple values must be specified
+                as an array."
 
             # We have to override the feeding mechanism; it might be nil or 
             # white-space separated
@@ -59,14 +66,10 @@ module Puppet
                 @should
             end
 
-            munge do |values|
-                unless values.is_a?(Array)
-                    values = [values]
+            validate do |value|
+                if value =~ /\s/
+                    raise Puppet::Error, "Aliases cannot have whitespace in them"
                 end
-                # Split based on comma, then flatten the whole thing
-                values.collect { |values|
-                    values.split(/,\s*/)
-                }.flatten
             end
         end
 
@@ -167,12 +170,12 @@ module Puppet
             end
 
             if hash[:protocols]
+                # The protocol can be a symbol, so...
+                if proto.is_a?(Symbol)
+                    proto = []
+                end
                 # Check to see if it already includes our proto
-                if proto.include?(hash[:protocols])
-                    Puppet.warning(
-                        "There is already a port with our name and protocols"
-                    )
-                else
+                unless proto.include?(hash[:protocols])
                     # We are missing their proto
                     proto << hash[:protocols]
                     #Puppet.info "new proto is %s" % proto.inspect
@@ -190,19 +193,24 @@ module Puppet
         end
 
         # Convert the current object into a host-style string.
-        def to_str
-            str = "%s\t%s/%s" % [self[:name], self.state(:number).should,
-                self.state(:protocols).should]
-
-            if state = self.state(:alias)
-                str += "\t%s" % state.should.join(" ")
-            end
+        def to_s
+            self.state(:protocols).should.collect { |proto|
+                str = "%s\t%s/%s" % [self[:name], self.state(:number).should,
+                    proto]
 
-            if state = self.state(:description)
-                str += "\t# %s" % state.should
-            end
+                if state = self.state(:alias)
+                    str += "\t%s" % state.should.join(" ")
+                else
+                    str += "\t"
+                end
 
-            str
+                if state = self.state(:description)
+                    str += "\t# %s" % state.should
+                else
+                    str += "\t"
+                end
+                str
+            }.join("\n")
         end
     end
 end
diff --git a/lib/puppet/type/state.rb b/lib/puppet/type/state.rb
index 6bd1d3b07..5ce9718b0 100644
--- a/lib/puppet/type/state.rb
+++ b/lib/puppet/type/state.rb
@@ -198,7 +198,7 @@ class State < Puppet::Parameter
     end
 
     def should_to_s
-        @should
+        @should.join(" ")
     end
 
     def to_s
diff --git a/test/types/host.rb b/test/types/host.rb
index a1fedcd07..4c3c6d142 100755
--- a/test/types/host.rb
+++ b/test/types/host.rb
@@ -32,6 +32,19 @@ class TestHost < Test::Unit::TestCase
         @hosttype.filetype = Puppet::FileType.filetype(:ram)
     end
 
+    def mkhost
+        host = nil
+        assert_nothing_raised {
+            host = Puppet.type(:host).create(
+                :name => "culain",
+                :ip => "192.168.0.3",
+                :aliases => "puppet"
+            )
+        }
+
+        return host
+    end
+
     def test_simplehost
         mkfaketype
         host = nil
@@ -51,7 +64,12 @@ class TestHost < Test::Unit::TestCase
         }
 
         assert_nothing_raised {
-            assert_equal(Puppet.type(:host).fileobj.read, Puppet.type(:host).to_file)
+            assert(
+                Puppet.type(:host).to_file.include?(
+                    Puppet.type(:host).fileobj.read
+                ),
+                "File does not include all of our objects"
+            )
         }
     end
 
@@ -60,6 +78,29 @@ class TestHost < Test::Unit::TestCase
             Puppet.type(:host).retrieve
         }
     end
+
+    def test_moddinghost
+        mkfaketype
+        host = mkhost()
+
+        assert_events([:host_created], host)
+
+        host.retrieve
+
+        # This was a hard bug to track down.
+        assert_instance_of(String, host.is(:ip))
+
+        host[:aliases] = %w{madstop kirby yayness}
+
+        assert_events([:host_changed], host)
+    end
+
+    def test_multivalues
+        host = mkhost
+        assert_raise(Puppet::Error) {
+            host[:aliases] = "puppetmasterd yayness"
+        }
+    end
 end
 
 # $Id$
diff --git a/test/types/port.rb b/test/types/port.rb
index 5225f27ce..74a445a94 100755
--- a/test/types/port.rb
+++ b/test/types/port.rb
@@ -32,6 +32,21 @@ class TestPort < Test::Unit::TestCase
         @porttype.filetype = Puppet::FileType.filetype(:ram)
     end
 
+    def mkport
+        port = nil
+        assert_nothing_raised {
+            port = Puppet.type(:port).create(
+                :name => "puppet",
+                :number => 8139,
+                :protocols => "tcp",
+                :description => "The port that Puppet runs on",
+                :aliases => "coolness"
+            )
+        }
+
+        return port
+    end
+
     def test_simplehost
         mkfaketype
         host = nil
@@ -40,7 +55,7 @@ class TestPort < Test::Unit::TestCase
         }
 
         assert_nothing_raised {
-            host = Puppet.type(:port).create(
+            port = Puppet.type(:port).create(
                 :name => "puppet",
                 :number => 8139,
                 :protocols => "tcp",
@@ -53,11 +68,16 @@ class TestPort < Test::Unit::TestCase
         }
 
         assert_nothing_raised {
-            assert_equal(Puppet.type(:port).fileobj.read, Puppet.type(:port).to_file)
+            assert(
+                Puppet.type(:port).to_file.include?(
+                    Puppet.type(:port).fileobj.read
+                ),
+                "File does not include all of our objects"
+            )
         }
     end
 
-    def test_hostsparse
+    def test_portsparse
         assert_nothing_raised {
             Puppet.type(:port).retrieve
         }
@@ -71,6 +91,35 @@ class TestPort < Test::Unit::TestCase
             assert(dns.is(:protocols).include?(v), "DNS did not include proto %s" % v)
         }
     end
+
+    def test_moddingport
+        mkfaketype
+        port = nil
+        assert_nothing_raised {
+            port = Puppet.type(:port).create(
+                :name => "puppet",
+                :number => 8139,
+                :protocols => "tcp",
+                :description => "The port that Puppet runs on"
+            )
+        }
+
+        assert_events([:port_created], port)
+
+        port[:protocols] = %w{tcp udp}
+
+        assert_events([:port_changed], port)
+    end
+
+    def test_multivalues
+        port = mkport
+        assert_raise(Puppet::Error) {
+            port[:protocols] = "udp tcp"
+        }
+        assert_raise(Puppet::Error) {
+            port[:aliases] = "puppetmasterd yayness"
+        }
+    end
 end
 
 # $Id$
diff --git a/test/types/type.rb b/test/types/type.rb
index fd551c4fd..5426ec669 100644
--- a/test/types/type.rb
+++ b/test/types/type.rb
@@ -30,6 +30,11 @@ class TestType < Test::Unit::TestCase
                 "Failed to retrieve %s by name" % name
             )
 
+            # Skip types with no parameters or valid states
+            #unless ! type.parameters.empty? or ! type.validstates.empty?
+            #    next
+            #end
+
             assert(
                 type.namevar,
                 "Failed to retrieve namevar for %s" % name