From 28be4df7aadea1e6f40bc9ea630b2efa30f7639b Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 5 Apr 2017 22:17:55 -0700 Subject: Chef-13: better solution to gem_package source issues 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 --- RELEASE_NOTES.md | 19 ++++---- chef-config/lib/chef-config/config.rb | 2 +- lib/chef/provider/package/rubygems.rb | 10 ++--- lib/chef/resource/gem_package.rb | 3 ++ spec/unit/provider/package/rubygems_spec.rb | 70 ++++++++++++++++------------- 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 -- cgit v1.2.1