Merge pull request #2872 from chef/lcg/chef-gem-config-option

Lcg/chef gem config option

10 files changed, 186 insertions, 28 deletions

diff --git a/DOC_CHANGES.md b/DOC_CHANGES.md
index 4e11a6f957..e1b90d304d 100644
--- a/DOC_CHANGES.md
+++ b/DOC_CHANGES.md
@@ -44,3 +44,11 @@ checking is off.
 The `package` provider has been extended to support multiple packages. This
 support is new and and not all subproviders yet support it. Full support for
 `apt` and `yum` has been implemented.
+
+## Add compile_time option to chef_gem
+
+This option defaults to true, which is deprecated, and setting this to false
+will stop chef_gem from automatically installing at compile_time.  False is
+the recommended setting as long as the gem is only used in provider code (a
+best practice) and not used directly in recipe code.
+
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 3c94bf21a9..9fc573fcd5 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -73,6 +73,55 @@ The `package` provider has been extended to support multiple packages. This
 support is new and and not all subproviders yet support it. Full support for
 `apt` and `yum` has been implemented.
 
+## chef_gem deprecation of installation at compile time
+
+A `compile_time` flag has been added to the chef_gem resource to control if it is installed at compile_time or not.  The prior behavior has been that this
+resource forces itself to install at compile_time which is problematic since if the gem is native it forces build_essentials and other dependent libraries
+to have to be installed at compile_time in an escalating war of forcing compile time execution.  This default was engineered before it was understood that a better
+approach was to lazily require gems inside of provider code which only ran at converge time and that requiring gems in recipe code was bad practice.
+
+The default behavior has not changed, but every chef_gem resource will now emit out a warning:
+
+```
+[2015-02-06T13:13:48-08:00] WARN: chef_gem[aws-sdk] chef_gem compile_time installation is deprecated
+[2015-02-06T13:13:48-08:00] WARN: chef_gem[aws-sdk] Please set `compile_time false` on the resource to use the new behavior.
+[2015-02-06T13:13:48-08:00] WARN: chef_gem[aws-sdk] or set `compile_time true` on the resource if compile_time behavior is required.
+```
+
+The preferred way to fix this is to make every chef_gem resource explicit about compile_time installation (keeping in mind the best-practice to default to false
+unless there is a reason):
+
+```ruby
+chef_gem 'aws-sdk' do
+  compile_time false
+end
+```
+
+There is also a Chef::Config[:chef_gem_compile_time] flag which has been added.  If this is set to true (not recommended) then chef will only emit a single
+warning at the top of the chef-client run:
+
+```
+[2015-02-06T13:27:35-08:00] WARN: setting chef_gem_compile_time to true is deprecated
+```
+
+It will behave like Chef 10 and Chef 11 and will default chef_gem to compile_time installations and will suppress
+subsequent warnings in the chef-client run.
+
+If this setting is changed to 'false' then it will adopt Chef-13 style behavior and will default all chef_gem installs to not run at compile_time by default.  This
+may break existing cookbooks.
+
+* All existing cookbooks which require compile_time true MUST be updated to be explicit about this setting.
+* To be considered high quality, cookbooks which require compile_time true MUST be rewritten to avoid this setting.
+* All existing cookbooks which do not require compile_time true SHOULD be updated to be explicit about this setting.
+
+For cookbooks that need to maintain backwards compatibility a `respond_to?` check should be used:
+
+```
+chef_gem 'aws-sdk' do
+  compile_time false if respond_to?(:compile_time)
+end
+```
+
 # Chef Client Release Notes 12.0.0:
 
 # Internal API Changes in this Release
diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index 5a67fc9091..6c85f951f6 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -49,6 +49,7 @@ class Chef
       configure_logging
       configure_proxy_environment_variables
       configure_encoding
+      emit_warnings
     end
 
     # Get this party started
@@ -372,6 +373,12 @@ class Chef
       ENV
     end
 
+    def emit_warnings
+      if Chef::Config[:chef_gem_compile_time]
+        Chef::Log.deprecation "setting chef_gem_compile_time to true is deprecated"
+      end
+    end
+
     class << self
       def debug_stacktrace(e)
         message = "#{e.class}: #{e}\n#{e.backtrace.join("\n")}"
diff --git a/lib/chef/config.rb b/lib/chef/config.rb
index 1a4ec06d98..c9e0914c0b 100644
--- a/lib/chef/config.rb
+++ b/lib/chef/config.rb
@@ -627,6 +627,13 @@ class Chef
     #
     default :no_lazy_load, true
 
+    # Default for the chef_gem compile_time attribute.  Nil is the same as false but will emit
+    # warnings on every use of chef_gem prompting the user to be explicit.  If the user sets this to
+    # true then the user will get backcompat behavior but with a single nag warning that cookbooks
+    # may break with this setting in the future.  The false setting is the recommended setting and
+    # will become the default.
+    default :chef_gem_compile_time, nil
+
     # A whitelisted array of attributes you want sent over the wire when node
     # data is saved.
     # The default setting is nil, which collects all data. Setting to [] will not
diff --git a/lib/chef/deprecation/warnings.rb b/lib/chef/deprecation/warnings.rb
index 22b28f93b0..34f468ff53 100644
--- a/lib/chef/deprecation/warnings.rb
+++ b/lib/chef/deprecation/warnings.rb
@@ -24,9 +24,11 @@ class Chef
         method_names.each do |name|
           m = instance_method(name)
           define_method(name) do |*args|
-            Chef::Log.warn "Method '#{name}' of '#{self.class}' is deprecated. It will be removed in Chef 12."
-            Chef::Log.warn "Please update your cookbooks accordingly. Accessed from:"
-            caller[0..3].each {|l| Chef::Log.warn l}
+            message = []
+            message << "Method '#{name}' of '#{self.class}' is deprecated. It will be removed in Chef 12."
+            message << "Please update your cookbooks accordingly. Accessed from:"
+            caller[0..3].each {|l| message << l}
+            Chef::Log.deprecation message
             super(*args)
           end
         end
@@ -35,4 +37,3 @@ class Chef
     end
   end
 end
-
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index 78d77e3778..38ba984ea7 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -212,7 +212,11 @@ class Chef
 
     class NoProviderAvailable < RuntimeError; end
 
-    class DeprecatedFeatureError < RuntimeError; end
+    class DeprecatedFeatureError < RuntimeError;
+      def initalize(message)
+        super("#{message} (raising error due to treat_deprecation_warnings_as_errors being set)")
+      end
+    end
 
     class MissingRole < RuntimeError
       NULL = Object.new
diff --git a/lib/chef/log.rb b/lib/chef/log.rb
index 131d706a5e..682afcea4b 100644
--- a/lib/chef/log.rb
+++ b/lib/chef/log.rb
@@ -19,6 +19,7 @@
 
 require 'logger'
 require 'chef/monologger'
+require 'chef/exceptions'
 require 'mixlib/log'
 
 class Chef
@@ -34,6 +35,14 @@ class Chef
       end
     end
 
+    def self.deprecation(msg=nil, &block)
+      if Chef::Config[:treat_deprecation_warnings_as_errors]
+        error(msg, &block)
+        raise Chef::Exceptions::DeprecatedFeatureError.new(msg)
+      else
+        warn(msg, &block)
+      end
+    end
+
   end
 end
-
diff --git a/lib/chef/resource/chef_gem.rb b/lib/chef/resource/chef_gem.rb
index 126e3d56c3..59f575a524 100644
--- a/lib/chef/resource/chef_gem.rb
+++ b/lib/chef/resource/chef_gem.rb
@@ -28,7 +28,7 @@ class Chef
       def initialize(name, run_context=nil)
         super
         @resource_name = :chef_gem
-        @compile_time = nil
+        @compile_time = Chef::Config[:chef_gem_compile_time]
         @gem_binary = RbConfig::CONFIG['bindir'] + "/gem"
       end
 
@@ -53,9 +53,9 @@ class Chef
         # Chef::Resource.run_action: Caveat: this skips Chef::Runner.run_action, where notifications are handled
         # Action could be an array of symbols, but probably won't (think install + enable for a package)
         if compile_time.nil?
-          Chef::Log.warn "The chef_gem installation at compile time is deprecated and this behavior will change in the future."
-          Chef::Log.warn "Please set `compile_time false` on the resource to use the new behavior and suppress this warning,"
-          Chef::Log.warn "or you may set `compile_time true` on the resource if compile_time behavior is necessary."
+          Chef::Log.deprecation "#{self} chef_gem compile_time installation is deprecated"
+          Chef::Log.deprecation "#{self} Please set `compile_time false` on the resource to use the new behavior."
+          Chef::Log.deprecation "#{self} or set `compile_time true` on the resource if compile_time behavior is required."
         end
 
         if compile_time || compile_time.nil?
diff --git a/spec/unit/deprecation_spec.rb b/spec/unit/deprecation_spec.rb
index 9bd081a6c4..f824cb7c76 100644
--- a/spec/unit/deprecation_spec.rb
+++ b/spec/unit/deprecation_spec.rb
@@ -59,27 +59,40 @@ describe Chef::Deprecation do
     end
   end
 
-  context 'deprecation warning messages' do
-    before(:each) do
-      @warning_output = [ ]
-      allow(Chef::Log).to receive(:warn) { |msg| @warning_output << msg }
+  context 'when Chef::Config[:treat_deprecation_warnings_as_errors] is off' do
+    before do
+      Chef::Config[:treat_deprecation_warnings_as_errors] = false
     end
 
-    it 'should be enabled for deprecated methods' do
-      TestClass.new.deprecated_method(10)
-      expect(@warning_output).not_to be_empty
+    context 'deprecation warning messages' do
+      before(:each) do
+        @warning_output = [ ]
+        allow(Chef::Log).to receive(:warn) { |msg| @warning_output << msg }
+      end
+
+      it 'should be enabled for deprecated methods' do
+        TestClass.new.deprecated_method(10)
+        expect(@warning_output).not_to be_empty
+      end
+
+      it 'should contain stack trace' do
+        TestClass.new.deprecated_method(10)
+        expect(@warning_output.join("").include?(".rb")).to be_truthy
+      end
     end
 
-    it 'should contain stack trace' do
-      TestClass.new.deprecated_method(10)
-      expect(@warning_output.join("").include?(".rb")).to be_truthy
+    it 'deprecated methods should still be called' do
+      test_instance = TestClass.new
+      test_instance.deprecated_method(10)
+      expect(test_instance.get_value).to eq(10)
     end
   end
 
-  it 'deprecated methods should still be called' do
+  it 'should raise when deprecation warnings are treated as errors' do
+    # rspec should set this
+    expect(Chef::Config[:treat_deprecation_warnings_as_errors]).to be true
     test_instance = TestClass.new
-    test_instance.deprecated_method(10)
-    expect(test_instance.get_value).to eq(10)
+    expect { test_instance.deprecated_method(10) }.to raise_error(Chef::Exceptions::DeprecatedFeatureError)
   end
 
 end
diff --git a/spec/unit/resource/chef_gem_spec.rb b/spec/unit/resource/chef_gem_spec.rb
index 657713d54f..7352a8f5fe 100644
--- a/spec/unit/resource/chef_gem_spec.rb
+++ b/spec/unit/resource/chef_gem_spec.rb
@@ -63,7 +63,12 @@ describe Chef::Resource::ChefGem, "gem_binary" do
       Chef::Recipe.new("hjk", "test", run_context)
     end
 
-    let(:resource) { Chef::Resource::ChefGem.new("foo", run_context) }
+    let(:chef_gem_compile_time) { nil }
+
+    let(:resource) do
+      Chef::Config[:chef_gem_compile_time] = chef_gem_compile_time
+      Chef::Resource::ChefGem.new("foo", run_context)
+    end
 
     before do
       expect(Chef::Resource::ChefGem).to receive(:new).and_return(resource)
@@ -71,20 +76,20 @@ describe Chef::Resource::ChefGem, "gem_binary" do
 
     it "runs the install at compile-time by default", :chef_lt_13_only do
       expect(resource).to receive(:run_action).with(:install)
-      expect(Chef::Log).to receive(:warn).at_least(:once)
+      expect(Chef::Log).to receive(:deprecation).at_least(:once)
       recipe.chef_gem "foo"
     end
 
     # the default behavior will change in Chef-13
     it "does not runs the install at compile-time by default", :chef_gte_13_only do
       expect(resource).not_to receive(:run_action).with(:install)
-      expect(Chef::Log).not_to receive(:warn)
+      expect(Chef::Log).not_to receive(:deprecation)
       recipe.chef_gem "foo"
     end
 
     it "compile_time true installs at compile-time" do
       expect(resource).to receive(:run_action).with(:install)
-      expect(Chef::Log).not_to receive(:warn)
+      expect(Chef::Log).not_to receive(:deprecation)
       recipe.chef_gem "foo" do
         compile_time true
       end
@@ -92,10 +97,65 @@ describe Chef::Resource::ChefGem, "gem_binary" do
 
     it "compile_time false does not install at compile-time" do
       expect(resource).not_to receive(:run_action).with(:install)
-      expect(Chef::Log).not_to receive(:warn)
+      expect(Chef::Log).not_to receive(:deprecation)
       recipe.chef_gem "foo" do
         compile_time false
       end
     end
+
+    describe "when Chef::Config[:chef_gem_compile_time] is explicitly true" do
+      let(:chef_gem_compile_time) { true }
+
+      before do
+        expect(Chef::Log).not_to receive(:deprecation)
+      end
+
+      it "by default installs at compile-time" do
+        expect(resource).to receive(:run_action).with(:install)
+        recipe.chef_gem "foo"
+      end
+
+      it "compile_time true installs at compile-time" do
+        expect(resource).to receive(:run_action).with(:install)
+        recipe.chef_gem "foo" do
+          compile_time true
+        end
+      end
+
+      it "compile_time false does not install at compile-time" do
+        expect(resource).not_to receive(:run_action).with(:install)
+        recipe.chef_gem "foo" do
+          compile_time false
+        end
+      end
+    end
+
+    describe "when Chef::Config[:chef_gem_compile_time] is explicitly false" do
+
+      let(:chef_gem_compile_time) { false }
+
+      before do
+        expect(Chef::Log).not_to receive(:deprecation)
+      end
+
+      it "by default does not install at compile-time" do
+        expect(resource).not_to receive(:run_action).with(:install)
+        recipe.chef_gem "foo"
+      end
+
+      it "compile_time true installs at compile-time" do
+        expect(resource).to receive(:run_action).with(:install)
+        recipe.chef_gem "foo" do
+          compile_time true
+        end
+      end
+
+      it "compile_time false does not install at compile-time" do
+        expect(resource).not_to receive(:run_action).with(:install)
+        recipe.chef_gem "foo" do
+          compile_time false
+        end
+      end
+    end
   end
 end