Make include_default_source nil by default and change implied logic.

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

3 files changed, 136 insertions, 15 deletions

diff --git a/lib/chef/provider/package/rubygems.rb b/lib/chef/provider/package/rubygems.rb
index abe270c8f8..7416b9e28b 100644
--- a/lib/chef/provider/package/rubygems.rb
+++ b/lib/chef/provider/package/rubygems.rb
@@ -483,9 +483,23 @@ class Chef
                                       end
         end
 
+        ##
+        # TODO: Fix comment
+        # If `clear_sources` is nil, clearing sources is implied if a `source`
+        # was added or if the global rubygems URL is set. If `clear_sources`
+        # is not nil, it has been set explicitly on the resource and its value
+        # should be used.
+        def include_default_source?
+          if new_resource.include_default_source.nil?
+            !!Chef::Config[:rubygems_url] || !(new_resource.source || new_resource.clear_sources)
+          else
+            new_resource.include_default_source
+          end
+        end
+
         def gem_sources
           srcs = [ new_resource.source ]
-          srcs << (Chef::Config[:rubygems_url] || "https://rubygems.org") if new_resource.include_default_source
+          srcs << (Chef::Config[:rubygems_url] || "https://rubygems.org") if include_default_source?
           srcs.flatten.compact
         end
 
diff --git a/lib/chef/resource/gem_package.rb b/lib/chef/resource/gem_package.rb
index fdf2e65441..5cd83d25fd 100644
--- a/lib/chef/resource/gem_package.rb
+++ b/lib/chef/resource/gem_package.rb
@@ -48,9 +48,9 @@ class Chef
       property :gem_binary, String, desired_state: false,
         description: "The path of a gem binary to use for the installation. By default, the same version of Ruby that is used by the #{Chef::Dist::CLIENT} will be installed."
 
-      property :include_default_source, [ TrueClass, FalseClass ],
+      property :include_default_source, [ TrueClass, FalseClass, nil ],
         description: "Set to 'false' to not include 'Chef::Config[:rubygems_url]'' in the sources.",
-        default: true, introduced: "13.0"
+        default: nil, introduced: "13.0"
 
       property :options, [ String, Hash, Array, nil ],
         description: "Options for the gem install, either a Hash or a String. When a hash is given, the options are passed to Gem::DependencyInstaller.new, and the gem will be installed via the gems API. When a String is given, the gem will be installed by shelling out to the gem command. Using a Hash of options with an explicit gem_binary will result in undefined behavior.",
diff --git a/spec/unit/provider/package/rubygems_spec.rb b/spec/unit/provider/package/rubygems_spec.rb
index d2a480cbb0..8c578370c2 100644
--- a/spec/unit/provider/package/rubygems_spec.rb
+++ b/spec/unit/provider/package/rubygems_spec.rb
@@ -363,7 +363,7 @@ describe Chef::Provider::Package::Rubygems do
   let(:bindir) { "/usr/bin" }
   let(:options) { nil }
   let(:source) { nil }
-  let(:include_default_source) { true }
+  let(:include_default_source) { nil }
 
   let(:new_resource) do
     new_resource = Chef::Resource::GemPackage.new(gem_name)
@@ -585,7 +585,7 @@ describe Chef::Provider::Package::Rubygems do
 
         it "determines the candidate version by querying the remote gem servers" do
           expect(provider.gem_env).to receive(:candidate_version_from_remote)
-            .with(gem_dep, source, "https://rubygems.org")
+            .with(gem_dep, source)
             .and_return(Gem::Version.new(target_version))
           expect(provider.candidate_version).to eq(target_version)
         end
@@ -605,7 +605,7 @@ describe Chef::Provider::Package::Rubygems do
 
         it "determines the candidate version by querying the remote gem servers" do
           expect(provider.gem_env).to receive(:candidate_version_from_remote)
-            .with(gem_dep, *[source, "https://rubygems.org" ].flatten)
+            .with(gem_dep, *source)
             .and_return(Gem::Version.new(target_version))
           expect(provider.candidate_version).to eq(target_version)
         end
@@ -645,7 +645,7 @@ describe Chef::Provider::Package::Rubygems do
 
       before do
         expected_source = [ source ]
-        expected_source << "https://rubygems.org" if include_default_source
+        expected_source << "https://rubygems.org" if provider.include_default_source?
         allow(provider.gem_env).to receive(:candidate_version_from_remote).with(gem_dep, *expected_source.flatten.compact).and_return(version)
       end
 
@@ -660,7 +660,7 @@ describe Chef::Provider::Package::Rubygems do
           let(:source) { "http://gems.example.org" }
 
           it "installs the gem via the gems api" do
-            expect(provider.gem_env).to receive(:install).with(gem_dep, sources: [source, "https://rubygems.org"])
+            expect(provider.gem_env).to receive(:install).with(gem_dep, sources: [source])
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
@@ -728,9 +728,9 @@ describe Chef::Provider::Package::Rubygems do
         context "when the Chef::Config[:rubygems_url] option is provided" do
           let(:gem_binary) { "/foo/bar" }
 
-          it "installs the gem with rubygems.org as an added source" do
+          it "installs the gem" do
             Chef::Config[:rubygems_url] = "https://mirror1"
-            expect(provider.gem_env).to receive(:candidate_version_from_remote).with(gem_dep, Chef::Config[:rubygems_url]).and_return(version)
+            expect(provider.gem_env).to receive(:candidate_version_from_remote).with(gem_dep, "https://mirror1").and_return(version)
             expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=https://mirror1"
             expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
@@ -742,13 +742,24 @@ describe Chef::Provider::Package::Rubygems do
           let(:source) { "http://mirror.ops.rhcloud.com/mirror/ruby" }
           let(:gem_binary) { "/foo/bar" }
 
-          it "installs the gem with rubygems.org as an added source" do
-            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=#{source} --source=https://rubygems.org"
+          it "installs the gem" do
+            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=#{source}"
             expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
 
+          context "with include_default_source true" do
+            let(:include_default_source) { true }
+
+            it "ignores the Chef::Config setting" do
+              expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=#{source} --source=https://rubygems.org"
+              expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
+              provider.run_action(:install)
+              expect(new_resource).to be_updated_by_last_action
+            end
+          end
+
           context "with include_default_source false" do
             let(:include_default_source) { false }
 
@@ -767,12 +778,23 @@ describe Chef::Provider::Package::Rubygems do
           let(:gem_binary) { "/foo/bar" }
 
           it "installs the gem with an array as an added source" do
-            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=https://mirror1 --source=https://mirror2 --source=https://rubygems.org"
+            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=https://mirror1 --source=https://mirror2"
             expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
 
+          context "with include_default_source true" do
+            let(:include_default_source) { true }
+
+            it "installs the gem with rubygems as a source" do
+            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=https://mirror1 --source=https://mirror2 --source=https://rubygems.org"
+              expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
+              provider.run_action(:install)
+              expect(new_resource).to be_updated_by_last_action
+            end
+          end
+
           context "with include_default_source false" do
             let(:include_default_source) { false }
 
@@ -792,7 +814,7 @@ describe Chef::Provider::Package::Rubygems do
 
           it "installs the gem" do
             new_resource.clear_sources(true)
-            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=#{source} --source=https://rubygems.org"
+            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --clear-sources --source=#{source}"
             expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
@@ -805,7 +827,7 @@ describe Chef::Provider::Package::Rubygems do
 
           it "installs the gem" do
             new_resource.clear_sources(false)
-            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --source=#{source} --source=https://rubygems.org"
+            expected = "#{gem_binary} install rspec-core -q --no-document -v \"#{target_version}\" --source=#{source}"
             expect(provider).to receive(:shell_out_compacted!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
@@ -1057,3 +1079,88 @@ describe Chef::Provider::Package::Rubygems, "clear_sources?" do
     end
   end
 end
+
+describe Chef::Provider::Package::Rubygems, "include_default_source?" do
+  let(:new_resource) do
+    Chef::Resource::GemPackage.new("foo")
+  end
+
+  let(:provider) do
+    run_context = Chef::RunContext.new(Chef::Node.new, {}, Chef::EventDispatch::Dispatcher.new)
+    Chef::Provider::Package::Rubygems.new(new_resource, run_context)
+  end
+
+  it "is true when include_default_source is unset" do
+    expect(provider.include_default_source?).to be true
+  end
+
+  it "is false when include_default_source is set false" do
+    new_resource.include_default_source(false)
+    expect(provider.include_default_source?).to be false
+  end
+
+  it "is true when include_default_source is set true" do
+    new_resource.include_default_source(true)
+    expect(provider.include_default_source?).to be true
+  end
+
+  context "when a source is set" do
+    before do
+      new_resource.source("http://mirror.ops.rhcloud.com/mirror/ruby")
+    end
+
+    it "is false when include_default_source is unset" do
+      expect(provider.include_default_source?).to be false
+    end
+
+    it "is false when include_default_source is set false" do
+      new_resource.include_default_source(false)
+      expect(provider.include_default_source?).to be false
+    end
+
+    it "is true when include_default_source is set true" do
+      new_resource.include_default_source(true)
+      expect(provider.include_default_source?).to be true
+    end
+  end
+
+  context "when Chef::Config[:rubygems_url] is set" do
+    before do
+      Chef::Config.rubygems_url = "https://example.com/"
+    end
+
+    it "is true when include_default_source is unset" do
+      expect(provider.include_default_source?).to be true
+    end
+
+    it "is false when include_default_source is set false" do
+      new_resource.include_default_source(false)
+      expect(provider.include_default_source?).to be false
+    end
+
+    it "is true when include_default_source is set true" do
+      new_resource.include_default_source(true)
+      expect(provider.include_default_source?).to be true
+    end
+  end
+
+  context "when clear_sources is set" do
+    before do
+      new_resource.clear_sources(true)
+    end
+
+    it "is false when include_default_source is unset" do
+      expect(provider.include_default_source?).to be false
+    end
+
+    it "is false when include_default_source is set false" do
+      new_resource.include_default_source(false)
+      expect(provider.include_default_source?).to be false
+    end
+
+    it "is true when include_default_source is set true" do
+      new_resource.include_default_source(true)
+      expect(provider.include_default_source?).to be true
+    end
+  end
+end