More cookbook loader cleanup and documentation

Removes a bit more cruft left over from removing cookbook merging.

Tries to document what the loader actually is and what it does.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

3 files changed, 64 insertions, 30 deletions

diff --git a/lib/chef/cookbook/cookbook_version_loader.rb b/lib/chef/cookbook/cookbook_version_loader.rb
index c864c30505..66e3d536c5 100644
--- a/lib/chef/cookbook/cookbook_version_loader.rb
+++ b/lib/chef/cookbook/cookbook_version_loader.rb
@@ -7,6 +7,16 @@ require "find"
 
 class Chef
   class Cookbook
+    # This class is only used drectly from the Chef::CookbookLoader and from chef-fs,
+    # so it only affects legacy-mode chef-client runs and knife.  It is not used by
+    # server or zolo/zero modes.
+    #
+    # This seems to be mostly a glorified factory method for creating CookbookVersion
+    # objects now, with creating Metadata objects bolted onto the side?  It used
+    # to be also responsible for the merging of multiple objects when creating
+    # shadowed/merged cookbook versions from multiple sources.  It also handles
+    # Chefignore files.
+    #
     class CookbookVersionLoader
 
       UPLOADED_COOKBOOK_VERSION_FILE = ".uploaded-cookbook-version.json".freeze
@@ -162,18 +172,6 @@ class Chef
         cookbook_settings.values.all? { |files_hash| files_hash.empty? } && metadata_filenames.size == 0
       end
 
-      def merge!(other_cookbook_loader)
-        other_cookbook_settings = other_cookbook_loader.cookbook_settings
-        cookbook_settings.each do |file_type, file_list|
-          file_list.merge!(other_cookbook_settings[file_type])
-        end
-        metadata_filenames.concat(other_cookbook_loader.metadata_filenames)
-        @cookbook_paths += other_cookbook_loader.cookbook_paths
-        @frozen = true if other_cookbook_loader.frozen
-        @metadata = nil # reset metadata so it gets reloaded and all metadata files applied.
-        self
-      end
-
       def chefignore
         @chefignore ||= Chefignore.new(File.basename(cookbook_path))
       end
diff --git a/lib/chef/cookbook_loader.rb b/lib/chef/cookbook_loader.rb
index ed6f9342df..1a7dec8b03 100644
--- a/lib/chef/cookbook_loader.rb
+++ b/lib/chef/cookbook_loader.rb
@@ -25,42 +25,72 @@ require "chef/cookbook_version"
 require "chef/cookbook/chefignore"
 require "chef/cookbook/metadata"
 
-#
-# CookbookLoader class loads the cookbooks lazily as read
-#
 class Chef
+  # This class is used by knife, cheffs and legacy chef-solo modes.  It is not used by the server mode
+  # of chef-client or zolo/zero modes.
+  #
+  # This class implements orchestration around producing a single cookbook_version for a cookbook or
+  # loading a Mash of all cookbook_versions, using the cookbook_version_loader class, and doing
+  # lazy-access and memoization to only load each cookbook once on demand.
+  #
+  # This implements a key-value style each which makes it appear to be a Hash of String => CookbookVersion
+  # pairs where the String is the cookbook name.  The use of Enumerable combined with the Hash-style
+  # each is likely not entirely sane.
+  #
+  # This object is also passed and injected into the CookbookCollection object where it is converted
+  # to a Mash that looks almost exactly like the cookbook_by_name Mash in this object.
+  #
   class CookbookLoader
-    # FIXME: doc public api
-    attr_reader :cookbook_paths
+    # @return [Array<String>] the array of repo paths containing cookbook dirs
+    attr_reader :repo_paths
 
+    # XXX: this is highly questionable combined with the Hash-style each method
     include Enumerable
 
+    # @param repo_paths [Array<String>] the array of repo paths containing cookbook dirs
     def initialize(*repo_paths)
       @repo_paths = repo_paths.flatten.map { |p| File.expand_path(p) }
-      raise ArgumentError, "You must specify at least one cookbook repo path" if @repo_paths.empty?
+      raise ArgumentError, "You must specify at least one cookbook repo path" if repo_paths.empty?
     end
 
+    # The primary function of this class is to build this Mash mapping cookbook names as a string to
+    # the CookbookVersion objects for them.  Callers must call "load_cookbooks" first.
+    #
+    # @return [Mash<String, Chef::CookbookVersion>]
     def cookbooks_by_name
       @cookbooks_by_name ||= Mash.new
     end
 
+    # This class also builds a mapping of cookbook names to their Metadata objects.  Callers must call
+    # "load_cookbooks" first.
+    #
+    # @return [Mash<String, Chef::Cookbook::Metadata>]
     def metadata
       @metadata ||= Mash.new
     end
 
+    # Loads all cookbooks across all repo_paths
+    #
+    # @return [Mash<String, Chef::CookbookVersion>] the cookbooks_by_name Mash
     def load_cookbooks
-      cookbook_loaders.each_key do |cookbook_name|
+      cookbook_version_loaders.each_key do |cookbook_name|
         load_cookbook(cookbook_name)
       end
       cookbooks_by_name
     end
 
+    # Loads a single cookbook by its name.
+    #
+    # @param [String]
+    # @return [Chef::CookbookVersion]
     def load_cookbook(cookbook_name)
-      return nil unless cookbook_loaders.key?(cookbook_name)
+      unless cookbook_version_loaders.key?(cookbook_name)
+        raise Exceptions::CookbookNotFoundInRepo, "Cannot find a cookbook named #{cookbook_name}; did you forget to add metadata to a cookbook? (https://docs.chef.io/config_rb_metadata.html)"
+      end
 
       return cookbooks_by_name[cookbook_name] if cookbooks_by_name.key?(cookbook_name)
 
-      loader = cookbook_loaders[cookbook_name]
+      loader = cookbook_version_loaders[cookbook_name]
 
       loader.load
 
@@ -71,11 +101,7 @@ class Chef
     end
 
     def [](cookbook)
-      if cookbooks_by_name.key?(cookbook.to_sym) || load_cookbook(cookbook.to_sym)
-        cookbooks_by_name[cookbook.to_sym]
-      else
-        raise Exceptions::CookbookNotFoundInRepo, "Cannot find a cookbook named #{cookbook}; did you forget to add metadata to a cookbook? (https://docs.chef.io/config_rb_metadata.html)"
-      end
+      load_cookbook(cookbook)
     end
 
     alias :fetch :[]
@@ -113,6 +139,11 @@ class Chef
 
     private
 
+    # Helper method to lazily create and remember the chefignore object
+    # for a given repo_path.
+    #
+    # @param [String] repo_path the full path to the cookbook directory of the repo
+    # @return [Chef::Cookbook::Chefignore] the chefignore object for the repo_path
     def chefignore(repo_path)
       @chefignores ||= {}
       @chefignores[repo_path] ||= Cookbook::Chefignore.new(repo_path)
@@ -126,14 +157,17 @@ class Chef
     def all_files_in_repo_paths
       @all_files_in_repo_paths ||=
         begin
-          @repo_paths.inject([]) do |all_children, repo_path|
+          repo_paths.inject([]) do |all_children, repo_path|
             all_children + Dir[File.join(Chef::Util::PathHelper.escape_glob_dir(repo_path), "*")]
           end
         end
     end
 
-    def cookbook_loaders
-      @cookbook_loaders ||=
+    # This method creates a Mash of the CookbookVersionLoaders for each cookbook.
+    #
+    # @return [Mash<String, Cookbook::CookbookVersionLoader>]
+    def cookbook_version_loaders
+      @cookbook_version_loaders ||=
         begin
           mash = Mash.new
           all_directories_in_repo_paths.each do |cookbook_path|
diff --git a/spec/unit/cookbook_loader_spec.rb b/spec/unit/cookbook_loader_spec.rb
index 7c15cc9030..ddb4f00f35 100644
--- a/spec/unit/cookbook_loader_spec.rb
+++ b/spec/unit/cookbook_loader_spec.rb
@@ -196,7 +196,9 @@ describe Chef::CookbookLoader do
     end
 
     it "should not load the cookbook again when accessed" do
-      expect(cookbook_loader).not_to receive("load_cookbook")
+      cookbook_loader.send(:cookbook_version_loaders).each do |cbv_loader|
+        expect(cbv_loader).not_to receive(:load)
+      end
       cookbook_loader["openldap"]
     end