Chef-13: better solution to gem_package source issueslcg/chef-13-gem-package-take-2

I think this is better

- Chef::Config[:rubygems_uri] is nearly always applied (but
  could be set to nil if someone really wanted to opt-out)
- the per-resources sources are additive on top of the
  global config (lets cookbooks be additive to global config
  rather than override it by default).
- there's a new flag for if you want to opt-out of the global
  config completely on a per-resource basis.

So I think this gets the most commmon and the next most common
use cases correct, while still allow people with arbitrarily
crazy needs to write resources that do it.

The RELEASE_NOTES also have somewhat better user stories in them,
which I think suggests that I'm on the right track here.

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

5 files changed, 59 insertions, 45 deletions

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index b16550b8e6..0ccd86637c 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -243,16 +243,19 @@ property :x, default: lazy { {} }
 
 ### Rubygems provider sources behavior changed.
 
-The default behavior of the `gem_package` and `chef_gem` resources is now to inherit whatever settings are in the external environment
-that chef is running in.  Chef no longer forces `https://rubygems.org`.  The `Chef::Config[:rubygems_uri]` default has been changed to
-nil.  It can now be set to either a string URI or to an array of string URIs.  The behavior of setting the source on an individual
-resource now overrides the source setting completely and does not inherit the global setting.
+The behavior of `gem_package` and `chef_gem` is now to always apply the `Chef::Config[:rubygems_uri]` sources, which may be a
+String uri or an Array of Strings.  If additional sources are put on the resource with the `source` property those are added
+to the configured `:rubygems_uri` sources.
 
-Users that previously relied on the source setting always being additive to "https://rubygmes.org" will find that they need to use
-the array form and explicitly add "https://rubygems.org" to their resources.  Users can now more easily remove "https://rubygems.org"
-either globally or on a resource case-by-case basis.
+This should enable easier setup of rubygems mirrors particularly in "airgapped" environments through the use of the global config
+variable.  It also means that an admin may force all rubygems.org traffic to an internal mirror, while still being able to
+consume external cookbooks which have resources which add other mirrors unchanged (in a non-airgapped environment).
 
-The behavior of the `clear_sources` property is now to only add `--clear-sources` and has no side effects on the source options.
+In the case where a resource must force the use of only the specified source(s), then the `include_default_source` property
+has been added -- setting it to false will remove the `Chef::Config[:rubygems_url]` setting from the list of sources for
+that resource.
+
+The behavior of the `clear_sources` property is now to only add `--clear-sources` and has no magic side effects on the source options.
 
 ### `knife cookbook site vendor` has been removed
 
diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 57f55fdce4..c63098930b 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -1055,7 +1055,7 @@ module ChefConfig
     default :ruby_encoding, Encoding::UTF_8
 
     # can be set to a string or array of strings for URIs to set as rubygems sources
-    default :rubygems_url, nil
+    default :rubygems_url, "https://www.rubygems.org"
 
     # If installed via an omnibus installer, this gives the path to the
     # "embedded" directory which contains all of the software packaged with
diff --git a/lib/chef/provider/package/rubygems.rb b/lib/chef/provider/package/rubygems.rb
index cdb2269c73..69936b3d58 100644
--- a/lib/chef/provider/package/rubygems.rb
+++ b/lib/chef/provider/package/rubygems.rb
@@ -470,8 +470,9 @@ class Chef
         end
 
         def gem_sources
-          srcs = new_resource.source || Chef::Config[:rubygems_url]
-          srcs ? Array(srcs) : nil
+          srcs = [ new_resource.source ]
+          srcs << Chef::Config[:rubygems_url] if new_resource.include_default_source
+          srcs.flatten.compact
         end
 
         def load_current_resource
@@ -540,10 +541,7 @@ class Chef
             name = new_resource.source
           else
             src << "--clear-sources" if new_resource.clear_sources
-            srcarry = [ new_resource.source || Chef::Config[:rubygems_url] ].flatten.compact
-            srcarry.each do |s|
-              src << "--source=#{s}"
-            end
+            src += gem_sources.map { |s| "--source=#{s}" }
           end
           src_str = src.empty? ? "" : " #{src.join(" ")}"
           if !version.nil? && !version.empty?
diff --git a/lib/chef/resource/gem_package.rb b/lib/chef/resource/gem_package.rb
index bcbe6d37b3..fc162a6033 100644
--- a/lib/chef/resource/gem_package.rb
+++ b/lib/chef/resource/gem_package.rb
@@ -39,6 +39,9 @@ class Chef
       # Sets a custom gem_binary to run for gem commands.
       property :gem_binary, String, desired_state: false
 
+      # set to false to avoid including Chef::Config[:rubygems_url] in the sources
+      property :include_default_source, [ TrueClass, FalseClass ], default: true
+
       ##
       # 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
diff --git a/spec/unit/provider/package/rubygems_spec.rb b/spec/unit/provider/package/rubygems_spec.rb
index 08dd888a2b..c40eed50cf 100644
--- a/spec/unit/provider/package/rubygems_spec.rb
+++ b/spec/unit/provider/package/rubygems_spec.rb
@@ -341,6 +341,7 @@ describe Chef::Provider::Package::Rubygems do
   let(:bindir) { "/usr/bin/ruby" }
   let(:options) { nil }
   let(:source) { nil }
+  let(:include_default_source) { true }
 
   let(:new_resource) do
     new_resource = Chef::Resource::GemPackage.new(gem_name)
@@ -348,6 +349,7 @@ describe Chef::Provider::Package::Rubygems do
     new_resource.gem_binary(gem_binary) if gem_binary
     new_resource.options(options) if options
     new_resource.source(source) if source
+    new_resource.include_default_source(include_default_source)
     new_resource
   end
 
@@ -549,12 +551,13 @@ 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)
+            .with(gem_dep, source, Chef::Config[:rubygems_url])
             .and_return(Gem::Version.new(target_version))
           expect(provider.candidate_version).to eq(target_version)
         end
 
         it "overwrites the config variable" do
+          new_resource.include_default_source false
           Chef::Config[:rubygems_url] = "https://overridden"
           expect(provider.gem_env).to receive(:candidate_version_from_remote)
             .with(gem_dep, source)
@@ -568,12 +571,13 @@ 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)
+            .with(gem_dep, *[source, Chef::Config[:rubygems_url] ].flatten)
             .and_return(Gem::Version.new(target_version))
           expect(provider.candidate_version).to eq(target_version)
         end
 
         it "overwrites the config variable" do
+          new_resource.include_default_source false
           Chef::Config[:rubygems_url] = "https://overridden"
           expect(provider.gem_env).to receive(:candidate_version_from_remote)
             .with(gem_dep, *source)
@@ -606,16 +610,14 @@ describe Chef::Provider::Package::Rubygems do
       let(:version) { Gem::Version.new(candidate_version) }
 
       before do
-        if source
-          allow(provider.gem_env).to receive(:candidate_version_from_remote).with(gem_dep, *source).and_return(version)
-        else
-          allow(provider.gem_env).to receive(:candidate_version_from_remote).with(gem_dep).and_return(version)
-        end
+        expected_source = [ source ]
+        expected_source << Chef::Config[:rubygems_url] if include_default_source
+        allow(provider.gem_env).to receive(:candidate_version_from_remote).with(gem_dep, *expected_source.flatten.compact).and_return(version)
       end
 
       describe "in the current gem environment" do
         it "installs the gem via the gems api when no explicit options are used" do
-          expect(provider.gem_env).to receive(:install).with(gem_dep, sources: nil)
+          expect(provider.gem_env).to receive(:install).with(gem_dep, sources: [ Chef::Config[:rubygems_url] ])
           provider.run_action(:install)
           expect(new_resource).to be_updated_by_last_action
         end
@@ -624,7 +626,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])
+            expect(provider.gem_env).to receive(:install).with(gem_dep, sources: [source, Chef::Config[:rubygems_url]])
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
@@ -655,7 +657,7 @@ describe Chef::Provider::Package::Rubygems do
         it "installs the gem via the gems api, when the package has no file separator characters in it, but a matching file exists in cwd" do
           allow(::File).to receive(:exist?).and_return(true)
           new_resource.package_name("rspec-core")
-          expect(provider.gem_env).to receive(:install).with(gem_dep, sources: nil)
+          expect(provider.gem_env).to receive(:install).with(gem_dep, sources: [ Chef::Config[:rubygems_url] ])
           provider.run_action(:install)
           expect(new_resource).to be_updated_by_last_action
         end
@@ -664,7 +666,7 @@ describe Chef::Provider::Package::Rubygems do
           let(:options) { "-i /alt/install/location" }
 
           it "installs the gem by shelling out when options are provided as a String" do
-            expected = "gem install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" #{options}"
+            expected = "gem install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=https://www.rubygems.org #{options}"
             expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
@@ -689,18 +691,22 @@ describe Chef::Provider::Package::Rubygems do
           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-rdoc --no-ri -v \"#{target_version}\" --source=#{source}"
+            expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=#{source} --source=https://www.rubygems.org"
             expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
 
-          it "ignores the Chef::Config setting" do
-            Chef::Config[:rubygems_url] = "https://ignored"
-            expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=#{source}"
-            expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
-            provider.run_action(:install)
-            expect(new_resource).to be_updated_by_last_action
+          context "with include_default_source false" do
+            let(:include_default_source) { false }
+
+            it "ignores the Chef::Config setting" do
+              Chef::Config[:rubygems_url] = "https://ignored"
+              expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=#{source}"
+              expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
+              provider.run_action(:install)
+              expect(new_resource).to be_updated_by_last_action
+            end
           end
         end
 
@@ -709,18 +715,22 @@ 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-rdoc --no-ri -v \"#{target_version}\" --source=https://mirror1 --source=https://mirror2"
+            expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=https://mirror1 --source=https://mirror2 --source=https://www.rubygems.org"
             expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
 
-          it "ignores the Chef::Config setting" do
-            Chef::Config[:rubygems_url] = "https://ignored"
-            expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=https://mirror1 --source=https://mirror2"
-            expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
-            provider.run_action(:install)
-            expect(new_resource).to be_updated_by_last_action
+          context "with include_default_source false" do
+            let(:include_default_source) { false }
+
+            it "ignores the Chef::Config setting" do
+              Chef::Config[:rubygems_url] = "https://ignored"
+              expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=https://mirror1 --source=https://mirror2"
+              expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
+              provider.run_action(:install)
+              expect(new_resource).to be_updated_by_last_action
+            end
           end
         end
 
@@ -730,7 +740,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-rdoc --no-ri -v \"#{target_version}\" --clear-sources --source=#{source}"
+            expected = "#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --clear-sources --source=#{source} --source=https://www.rubygems.org"
             expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
@@ -742,7 +752,7 @@ describe Chef::Provider::Package::Rubygems do
           let(:options) { "-i /alt/install/location" }
 
           it "installs the gem by shelling out when options are provided but no version is given" do
-            expected = "gem install rspec-core -q --no-rdoc --no-ri -v \"#{candidate_version}\" #{options}"
+            expected = "gem install rspec-core -q --no-rdoc --no-ri -v \"#{candidate_version}\" --source=https://www.rubygems.org #{options}"
             expect(provider).to receive(:shell_out!).with(expected, env: nil, timeout: 900)
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
@@ -753,7 +763,7 @@ describe Chef::Provider::Package::Rubygems do
           let(:options) { { install_dir: "/alt/install/location" } }
 
           it "installs the gem via the gems api when options are given as a Hash" do
-            expect(provider.gem_env).to receive(:install).with(gem_dep, { sources: nil }.merge(options))
+            expect(provider.gem_env).to receive(:install).with(gem_dep, { sources: [ Chef::Config[:rubygems_url] ] }.merge(options))
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
@@ -763,7 +773,7 @@ describe Chef::Provider::Package::Rubygems do
           let(:target_version) { "9000.0.2" }
 
           it "installs the gem via the gems api" do
-            expect(provider.gem_env).to receive(:install).with(gem_dep, sources: nil)
+            expect(provider.gem_env).to receive(:install).with(gem_dep, sources: [ Chef::Config[:rubygems_url] ] )
             provider.run_action(:install)
             expect(new_resource).to be_updated_by_last_action
           end
@@ -806,7 +816,7 @@ describe Chef::Provider::Package::Rubygems do
         let(:gem_binary) { "/usr/weird/bin/gem" }
 
         it "installs the gem by shelling out to gem install" do
-          expect(provider).to receive(:shell_out!).with("#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\"", env: nil, timeout: 900)
+          expect(provider).to receive(:shell_out!).with("#{gem_binary} install rspec-core -q --no-rdoc --no-ri -v \"#{target_version}\" --source=https://www.rubygems.org", env: nil, timeout: 900)
           provider.run_action(:install)
           expect(new_resource).to be_updated_by_last_action
         end