Modules now can find their own paths

Previously, when you created a module you had to specify
the path.  Now Module instances can use the module path
to look up their paths, and there are methods for determining
whether the module is present (if the path is present).

Also cleaned up the methods for figuring out what's in
the module (plugins, etc.).

Signed-off-by: Luke Kanies <luke@madstop.com>

lib/puppet/file_serving/mount/plugins.rb

@@ -16,7 +16,7 @@ class Puppet::FileServing::Mount::Plugins < Puppet::FileServing::Mount
     def search(relative_path, options = {})
         # We currently only support one kind of search on plugins - return
         # them all.
-        paths = environment(options[:node]).modules.find_all { |mod|  mod.plugins? }.collect { |mod| mod.plugins }
+        paths = environment(options[:node]).modules.find_all { |mod|  mod.plugins? }.collect { |mod| mod.plugin_directory }
         return nil if paths.empty?
         return paths
     end

lib/puppet/module.rb

@@ -25,7 +25,7 @@ class Puppet::Module
 
                 yielded[name] = true
 
-                yield Puppet::Module.new(name, module_path)
+                yield Puppet::Module.new(name)
             end
         end
     end
@@ -41,49 +41,91 @@ class Puppet::Module
     # absolute, or if there is no module whose name is the first component
     # of +path+, return +nil+
     def self.find(modname, environment = nil)
+        return nil unless modname
         Puppet::Node::Environment.new(environment).module(modname)
     end
 
-    attr_reader :name, :path
-    def initialize(name, path)
+    attr_reader :name, :environment
+    def initialize(name, environment = nil)
         @name = name
-        @path = path
+        if environment.is_a?(Puppet::Node::Environment)
+            @environment = environment
+        else
+            @environment = Puppet::Node::Environment.new(environment)
+        end
     end
 
     FILETYPES.each do |type|
-        # Create a method for returning the full path to a given
-        # file type's directory.
-        define_method(type.to_s) do
-            File.join(path, type.to_s)
+        # A boolean method to let external callers determine if
+        # we have files of a given type.
+        define_method(type +'?') do
+            return false unless path
+            return false unless FileTest.exist?(subpath(type))
+            return true
         end
 
-        # Create a boolean method for testing whether our module has
-        # files of a given type.
-        define_method(type.to_s + "?") do
-            FileTest.exist?(send(type))
-        end
-
-        # Finally, a method for returning an individual file
+        # A method for returning a given file of a given type.
+        # e.g., file = mod.manifest("my/manifest.pp")
+        #
+        # If the file name is nil, then the base directory for the
+        # file type is passed; this is used for fileserving.
         define_method(type.to_s.sub(/s$/, '')) do |file|
+            return nil unless path
+
+            # If 'file' is nil then they're asking for the base path.
+            # This is used for things like fileserving.
             if file
-                path = File.join(send(type), file)
+                full_path = File.join(subpath(type), file)
             else
-                path = send(type)
+                full_path = subpath(type)
             end
-            return nil unless FileTest.exist?(path)
-            return path
+
+            return nil unless FileTest.exist?(full_path)
+            return full_path
         end
     end
 
+    def exist?
+        ! path.nil?
+    end
+
+    # Find the first 'files' directory.  This is used by the XMLRPC fileserver.
+    def file_directory
+        subpath("files")
+    end
+
     # Return the list of manifests matching the given glob pattern,
     # defaulting to 'init.pp' for empty modules.
     def match_manifests(rest)
+        return find_init_manifest unless rest # Use init.pp
+
         rest ||= "init.pp"
         p = File::join(path, MANIFESTS, rest)
-        files = Dir.glob(p).reject { |f| FileTest.directory?(f) }
-        if files.size == 0
-            files = Dir.glob(p + ".pp")
+        result = Dir.glob(p).reject { |f| FileTest.directory?(f) }
+        if result.size == 0 and rest !~ /\.pp$/
+            result = Dir.glob(p + ".pp")
         end
-        return files
+        result.flatten.compact
+    end
+
+    # Find this module in the modulepath.
+    def path
+        self.class.modulepath.collect { |path| File.join(path, name) }.find { |d| FileTest.exist?(d) }
+    end
+
+    # Find all plugin directories.  This is used by the Plugins fileserving mount.
+    def plugin_directory
+        subpath("plugins")
+    end
+
+    private
+
+    def find_init_manifest
+        return [] unless file = manifest("init.pp")
+        return [file]
+    end
+
+    def subpath(type)
+        File.join(path, type)
     end
 end

lib/puppet/network/handler/fileserver.rb

@@ -254,8 +254,8 @@ class Puppet::Network::Handler
             end
 
             # And use the environment to look up the module.
-            if mod = Puppet::Node::Environment.new(env).module(module_name)
-                return @mounts[MODULES].copy(mod.name, mod.files)
+            if mod = Puppet::Node::Environment.new(env).module(module_name) and mod.files?
+                return @mounts[MODULES].copy(mod.name, mod.file_directory)
             else
                 return nil
             end
@@ -744,7 +744,7 @@ class Puppet::Network::Handler
 
             private
             def valid_modules(client)
-                Puppet::Node::Environment.new.modules.find_all { |mod| mod.plugins? }
+                Puppet::Node::Environment.new.modules.find_all { |mod| mod.exist? }
             end
             
             def add_to_filetree(f, filetree)

lib/puppet/node/environment.rb

@@ -30,11 +30,9 @@ class Puppet::Node::Environment
     end
 
     def module(name)
-        Puppet::Module.each_module(modulepath) do |mod|
-            return mod if mod.name == name
-        end
-
-        return nil
+        mod = Puppet::Module.new(name, self)
+        return nil unless mod.exist?
+        return mod
     end
 
     def modulepath

lib/puppet/parser/files.rb

@@ -16,16 +16,16 @@ module Puppet::Parser::Files
     def find_manifests(start, options = {})
         cwd = options[:cwd] || Dir.getwd
         module_name, pattern = split_file_path(start)
-        if module_name and mod = Puppet::Module.find(module_name, options[:environment])
+        if mod = Puppet::Module.find(module_name, options[:environment])
             return mod.match_manifests(pattern)
-        else
-            abspat = File::expand_path(start, cwd)
-            files = Dir.glob(abspat).reject { |f| FileTest.directory?(f) }
-            if files.size == 0
-                files = Dir.glob(abspat + ".pp").reject { |f| FileTest.directory?(f) }
-            end
-            return files
         end
+
+        abspat = File::expand_path(start, cwd)
+        files = Dir.glob(abspat).reject { |f| FileTest.directory?(f) }
+        if files.size == 0
+            files = Dir.glob(abspat + ".pp").reject { |f| FileTest.directory?(f) }
+        end
+        return files
     end
 
     # Find the concrete file denoted by +file+. If +file+ is absolute,

spec/unit/file_serving/mount/plugins.rb

@@ -61,8 +61,8 @@ describe Puppet::FileServing::Mount::Plugins, "when searching for files" do
     end
 
     it "should return the plugin paths for each module that has plugins" do
-        one = stub 'module', :plugins? => true, :plugins => "/one"
-        two = stub 'module', :plugins? => true, :plugins => "/two"
+        one = stub 'module', :plugins? => true, :plugin_directory => "/one"
+        two = stub 'module', :plugins? => true, :plugin_directory => "/two"
 
         @environment.expects(:modules).returns [one, two]
         @mount.search("foo/bar").should == %w{/one /two}

spec/unit/module.rb

@@ -3,40 +3,137 @@
 require File.dirname(__FILE__) + '/../spec_helper'
 
 describe Puppet::Module do
+    it "should have a class method that returns a named module from a given environment" do
+        env = mock 'module'
+        env.expects(:module).with("mymod").returns "yep"
+        Puppet::Node::Environment.expects(:new).with("myenv").returns env
+
+        Puppet::Module.find("mymod", "myenv").should == "yep"
+    end
+
+    it "should return nil if asked for a named module that doesn't exist" do
+        env = mock 'module'
+        env.expects(:module).with("mymod").returns nil
+        Puppet::Node::Environment.expects(:new).with("myenv").returns env
+
+        Puppet::Module.find("mymod", "myenv").should be_nil
+    end
+
+    it "should return nil if asked for a module whose name is 'nil'" do
+        Puppet::Module.find(nil, "myenv").should be_nil
+    end
+
+    it "should require a name at initialization" do
+        lambda { Puppet::Module.new }.should raise_error(ArgumentError)
+    end
+
+    it "should convert an environment name into an Environment instance" do
+        Puppet::Module.new("foo", "prod").environment.should be_instance_of(Puppet::Node::Environment)
+    end
+
+    it "should accept an environment at initialization" do
+        Puppet::Module.new("foo", :prod).environment.name.should == :prod
+    end
+
+    it "should use the default environment if none is provided" do
+        env = Puppet::Node::Environment.new
+        Puppet::Module.new("foo").environment.should equal(env)
+    end
+
+    it "should use any provided Environment instance" do
+        env = Puppet::Node::Environment.new
+        Puppet::Module.new("foo", env).environment.should equal(env)
+    end
+
+    it "should return the path to the first found instance in its module paths as its path" do
+        mod = Puppet::Module.new("foo")
+        paths = %w{/a /b /c}
+        Puppet::Module.stubs(:modulepath).returns paths
+
+        FileTest.expects(:exist?).with("/a/foo").returns false
+        FileTest.expects(:exist?).with("/b/foo").returns true
+        FileTest.expects(:exist?).with("/c/foo").never
+
+        mod.path.should == "/b/foo"
+    end
+
+    it "should be considered existent if it exists in at least one module path" do
+        mod = Puppet::Module.new("foo")
+        mod.expects(:path).returns "/a/foo"
+        mod.should be_exist
+    end
+
+    it "should be considered nonexistent if it does not exist in any of the module paths" do
+        mod = Puppet::Module.new("foo")
+        mod.expects(:path).returns nil
+        mod.should_not be_exist
+    end
+
     [:plugins, :templates, :files, :manifests].each do |filetype|
-        it "should be able to indicate whether it has #{filetype}" do
-            Puppet::Module.new("foo", "/foo/bar").should respond_to(filetype.to_s + "?")
-        end
-
-        it "should correctly detect when it has #{filetype}" do
-            FileTest.expects(:exist?).with("/foo/bar/#{filetype}").returns true
-            Puppet::Module.new("foo", "/foo/bar").send(filetype.to_s + "?").should be_true
-        end
-
-        it "should correctly detect when it does not have #{filetype}" do
-            FileTest.expects(:exist?).with("/foo/bar/#{filetype}").returns false
-            Puppet::Module.new("foo", "/foo/bar").send(filetype.to_s + "?").should be_false
-        end
-
-        it "should have a method for returning the full path to the #{filetype}" do
-            Puppet::Module.new("foo", "/foo/bar").send(filetype.to_s).should == File.join("/foo/bar", filetype.to_s)
-        end
-
         it "should be able to return individual #{filetype}" do
-            path = File.join("/foo/bar", filetype.to_s, "my/file")
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns "/a/foo"
+            path = File.join("/a/foo", filetype.to_s, "my/file")
             FileTest.expects(:exist?).with(path).returns true
-            Puppet::Module.new("foo", "/foo/bar").send(filetype.to_s.sub(/s$/, ''), "my/file").should == path
+            mod.send(filetype.to_s.sub(/s$/, ''), "my/file").should == path
+        end
+
+        it "should consider #{filetype} to be present if their base directory exists" do
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns "/a/foo"
+            path = File.join("/a/foo", filetype.to_s)
+            FileTest.expects(:exist?).with(path).returns true
+            mod.send(filetype.to_s + "?").should be_true
+        end
+
+        it "should consider #{filetype} to be absent if their base directory does not exist" do
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns "/a/foo"
+            path = File.join("/a/foo", filetype.to_s)
+            FileTest.expects(:exist?).with(path).returns false
+            mod.send(filetype.to_s + "?").should be_false
+        end
+
+        it "should consider #{filetype} to be absent if the module base directory does not exist" do
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns nil
+            mod.send(filetype.to_s + "?").should be_false
         end
 
         it "should return nil if asked to return individual #{filetype} that don't exist" do
-            FileTest.expects(:exist?).with(File.join("/foo/bar", filetype.to_s, "my/file")).returns false
-            Puppet::Module.new("foo", "/foo/bar").send(filetype.to_s.sub(/s$/, ''), "my/file").should be_nil
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns "/a/foo"
+            path = File.join("/a/foo", filetype.to_s, "my/file")
+            FileTest.expects(:exist?).with(path).returns false
+            mod.send(filetype.to_s.sub(/s$/, ''), "my/file").should be_nil
+        end
+
+        it "should return nil when asked for individual #{filetype} if the module does not exist" do
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns nil
+            mod.send(filetype.to_s.sub(/s$/, ''), "my/file").should be_nil
         end
 
         it "should return the base directory if asked for a nil path" do
-            path = File.join("/foo/bar", filetype.to_s)
-            FileTest.expects(:exist?).with(path).returns true
-            Puppet::Module.new("foo", "/foo/bar").send(filetype.to_s.sub(/s$/, ''), nil).should == path
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns "/a/foo"
+            base = File.join("/a/foo", filetype.to_s)
+            FileTest.expects(:exist?).with(base).returns true
+            mod.send(filetype.to_s.sub(/s$/, ''), nil).should == base
+        end
+    end
+
+    %w{plugins files}.each do |type|
+        short = type.sub(/s$/, '')
+        it "should be able to return the #{short} directory" do
+            Puppet::Module.new("foo").should respond_to(short + "_directory")
+        end
+
+        it "should return the path to the #{short} directory" do
+            mod = Puppet::Module.new("foo")
+            mod.stubs(:path).returns "/a/foo"
+
+            mod.send(short + "_directory").should == "/a/foo/#{type}"
         end
     end
 end
@@ -99,8 +196,8 @@ describe Puppet::Module, "when yielding each module in a list of directories" do
         one = mock 'one'
         two = mock 'two'
 
-        Puppet::Module.expects(:new).with("f1", "/one/f1").returns one
-        Puppet::Module.expects(:new).with("f2", "/one/f2").returns two
+        Puppet::Module.expects(:new).with("f1").returns one
+        Puppet::Module.expects(:new).with("f2").returns two
 
         result = []
         Puppet::Module.each_module("/one") do |mod|
@@ -116,8 +213,8 @@ describe Puppet::Module, "when yielding each module in a list of directories" do
 
         one = mock 'one'
 
-        Puppet::Module.expects(:new).with("f1", "/one/f1").returns one
-        Puppet::Module.expects(:new).with("f1", "/two/f1").never
+        Puppet::Module.expects(:new).with("f1").returns one
+        Puppet::Module.expects(:new).with("f1").never
 
         result = []
         Puppet::Module.each_module("/one", "/two") do |mod|
@@ -146,27 +243,48 @@ describe Puppet::Module, " when building its search path" do
     end
 end
 
-describe Puppet::Module, " when searching for modules" do
-    it "should use the current environment to find the specified module if no environment is provided" do
-        env = mock 'env'
-        env.expects(:module).with("foo").returns "yay"
-        Puppet::Node::Environment.expects(:new).with(nil).returns env
-
-        Puppet::Module.find("foo").should == "yay"
+describe Puppet::Module, "when finding matching manifests" do
+    before do
+        @mod = Puppet::Module.new("mymod")
+        @mod.stubs(:path).returns "/a"
     end
 
-    it "should use the specified environment to find the specified module if an environment is provided" do
-        env = mock 'env'
-        env.expects(:module).with("foo").returns "yay"
-        Puppet::Node::Environment.expects(:new).with("myenv").returns env
+    it "should return all manifests matching the glob pattern" do
+        Dir.expects(:glob).with("/a/manifests/yay/*.pp").returns(%w{foo bar})
 
-        Puppet::Module.find("foo", "myenv").should == "yay"
-    end
-end
-
-describe Puppet::Module, " when returning files" do
-    it "should return the path to the module's 'files' directory" do
-        mod = Puppet::Module.send(:new, "mymod", "/my/mod")
-        mod.files.should == "/my/mod/files"
+        @mod.match_manifests("yay/*.pp").should == %w{foo bar}
+    end
+
+    it "should not return directories" do
+        Dir.expects(:glob).with("/a/manifests/yay/*.pp").returns(%w{foo bar})
+
+        FileTest.expects(:directory?).with("foo").returns false
+        FileTest.expects(:directory?).with("bar").returns true
+        @mod.match_manifests("yay/*.pp").should == %w{foo}
+    end
+
+    it "should default to the 'init.pp' file if no glob pattern is specified" do
+        FileTest.stubs(:exist?).returns true
+
+        @mod.match_manifests(nil).should == %w{/a/manifests/init.pp}
+    end
+
+    it "should return all manifests matching the glob pattern in all existing paths" do
+        Dir.expects(:glob).with("/a/manifests/yay/*.pp").returns(%w{a b})
+
+        @mod.match_manifests("yay/*.pp").should == %w{a b}
+    end
+
+    it "should match the glob pattern plus '.pp' if no results are found" do
+        Dir.expects(:glob).with("/a/manifests/yay/foo").returns([])
+        Dir.expects(:glob).with("/a/manifests/yay/foo.pp").returns(%w{yay})
+
+        @mod.match_manifests("yay/foo").should == %w{yay}
+    end
+
+    it "should return an empty array if no manifests matched" do
+        Dir.expects(:glob).with("/a/manifests/yay/*.pp").returns([])
+
+        @mod.match_manifests("yay/*.pp").should == []
     end
 end

spec/unit/node/environment.rb

@@ -94,28 +94,24 @@ describe Puppet::Node::Environment do
             env.modules.should == %w{mod1 mod2}
         end
 
-        it "should be able to return an individual module by matching the module name" do
+        it "should be able to return an individual module that exists in its module path" do
             env = Puppet::Node::Environment.new("testing")
-            module_path = %w{/one /two}.join(File::PATH_SEPARATOR)
-            env.expects(:modulepath).returns module_path
 
-            one = stub 'one', :name => 'one'
-            two = stub 'two', :name => 'two'
-            Puppet::Module.expects(:each_module).with(module_path).multiple_yields(one, two)
+            mod = mock 'module'
+            Puppet::Module.expects(:new).with("one", env).returns mod
+            mod.expects(:exist?).returns true
 
-            env.module("two").should equal(two)
+            env.module("one").should equal(mod)
         end
 
-        it "should return nil if asked for a module that is not in its path" do
+        it "should return nil if asked for a module that does not exist in its path" do
             env = Puppet::Node::Environment.new("testing")
-            module_path = %w{/one /two}.join(File::PATH_SEPARATOR)
-            env.expects(:modulepath).returns module_path
 
-            one = stub 'one', :name => 'one'
-            two = stub 'two', :name => 'two'
-            Puppet::Module.expects(:each_module).with(module_path).multiple_yields(one, two)
+            mod = mock 'module'
+            Puppet::Module.expects(:new).with("one", env).returns mod
+            mod.expects(:exist?).returns false
 
-            env.module("three").should be_nil
+            env.module("one").should be_nil
         end
     end
 end

spec/unit/parser/files.rb

@@ -149,10 +149,6 @@ describe Puppet::Parser::Files do
     end
 
     describe "when searching for manifests in a found module" do
-        before do
-            @module = Puppet::Module.new("mymod", "/one")
-        end
-
         it "should return the manifests from the first found module" do
             mod = mock 'module'
             Puppet::Node::Environment.new.expects(:module).with("mymod").returns mod
@@ -167,28 +163,6 @@ describe Puppet::Parser::Files do
             Puppet::Parser::Files.find_manifests("mymod/init.pp", :environment => "myenv").should == ["/one/mymod/manifests/init.pp"]
         end
 
-        it "should return all manifests matching the glob pattern" do
-            File.stubs(:directory?).returns(true)
-            Dir.expects(:glob).with("/one/manifests/yay/*.pp").returns(%w{/one /two})
-
-            @module.match_manifests("yay/*.pp").should == %w{/one /two}
-        end
-
-        it "should not return directories" do
-            Dir.expects(:glob).with("/one/manifests/yay/*.pp").returns(%w{/one /two})
-
-            FileTest.expects(:directory?).with("/one").returns false
-            FileTest.expects(:directory?).with("/two").returns true
-
-            @module.match_manifests("yay/*.pp").should == %w{/one}
-        end
-
-        it "should default to the 'init.pp' file in the manifests directory" do
-            Dir.expects(:glob).with("/one/manifests/init.pp").returns(%w{/init.pp})
-
-            @module.match_manifests(nil).should == %w{/init.pp}
-        end
-
         after { Puppet.settings.clear }
     end
 end

test/language/parser.rb

@@ -1222,7 +1222,7 @@ file { "/tmp/yayness":
 
         # We use an exception to cut short the processing to simplify our stubbing
         #Puppet::Module.expects(:find_manifests).with("test", {:cwd => ".", :environment => "something"}).raises(Puppet::ParseError)
-        Puppet::Module.expects(:find_manifests).with("test", {:cwd => ".", :environment => "something"}).returns([])
+        Puppet::Parser::Files.expects(:find_manifests).with("test", {:cwd => ".", :environment => "something"}).returns([])
 
         assert_raise(Puppet::ImportError) do
             parser.import("test")