Merge pull request #264 from jgawor/log_file

Disable logging to a file by default

config/logging.yml

@@ -15,4 +15,5 @@
 
 # Logging configuration
 ---
-default_log_level: INFO
+default_log_level: INFO
+enable_log_file: false

lib/liberty_buildpack/diagnostics/logger_factory.rb

@@ -17,6 +17,7 @@
 require 'fileutils'
 require 'liberty_buildpack/diagnostics'
 require 'liberty_buildpack/diagnostics/common'
+require 'liberty_buildpack/util/configuration_utils'
 require 'logger'
 require 'monitor'
 require 'yaml'
@@ -30,9 +31,7 @@ class LoggerFactory
     # @param [String] app_dir the root directory for diagnostics
     # @return [Logger] the created Logger instance
     def self.create_logger(app_dir)
-      diagnostics_directory = LibertyBuildpack::Diagnostics.get_diagnostic_directory app_dir
-      FileUtils.mkdir_p diagnostics_directory
-      log_file = File.join(diagnostics_directory, LibertyBuildpack::Diagnostics::LOG_FILE_NAME)
+      configuration = LibertyBuildpack::Util::ConfigurationUtils.load('logging', false)
 
       if (defined? @@logger) && (@@logger != nil)
         logger_recreated = true
@@ -41,13 +40,18 @@ def self.create_logger(app_dir)
         logger_recreated = false
       end
 
+      delegates = [$stderr]
+      if configuration['enable_log_file']
+        log_file = log_file(app_dir)
+        delegates << File.open(log_file, 'a')
+      end
+
       @@monitor.synchronize do
-        @@logger = Logger.new(LogSplitter.new(File.open(log_file, 'a'), $stderr))
+        @@logger = Logger.new(LogSplitter.new(*delegates))
       end
 
-      set_log_level
+      set_log_level(configuration)
 
-      @@logger.debug(log_file)
       if logger_recreated
         @@logger.warn("Logger was re-created by #{caller[0]}")
       end
@@ -85,8 +89,7 @@ def self.get_logger
 
     @@monitor = Monitor.new
 
-    def self.set_log_level
-      logging_configuration = get_configuration
+    def self.set_log_level(logging_configuration)
       switched_log_level = $VERBOSE || $DEBUG ? DEBUG_SEVERITY_STRING : nil
       log_level = (ENV[LOG_LEVEL_ENVIRONMENT_VARIABLE] || switched_log_level || logging_configuration[DEFAULT_LOG_LEVEL_CONFIGURATION_KEY]).upcase
 
@@ -106,9 +109,11 @@ def self.set_log_level
                                end
     end
 
-    def self.get_configuration
-      expanded_path = File.expand_path(LOGGING_CONFIG, File.dirname(__FILE__))
-      YAML.load_file(expanded_path)
+    def self.log_file(app_dir)
+      diagnostics_directory = LibertyBuildpack::Diagnostics.get_diagnostic_directory app_dir
+      FileUtils.mkdir_p diagnostics_directory
+      log_file = File.join(diagnostics_directory, LibertyBuildpack::Diagnostics::LOG_FILE_NAME)
+      log_file
     end
 
     def self.close

spec/liberty_buildpack/buildpack_spec.rb

@@ -37,12 +37,11 @@ module LibertyBuildpack
     let(:stub_buildpack_version) { double('stub-buildpack-version', detect: nil, compile: nil) }
 
     before do
+      YAML.stub(:load_file).and_call_original
+
       version_config_path = Pathname.new(File.expand_path('../../config/version.yml', File.dirname(__FILE__))).freeze
       YAML.stub(:load_file).with(version_config_path).and_return({})
 
-      YAML.stub(:load_file).with(File.expand_path('config/logging.yml')).and_return(
-          'default_log_level' => 'DEBUG'
-      )
       YAML.stub(:load_file).with(File.expand_path('config/licenses.yml')).and_return(nil)
 
       allow(LibertyBuildpack::Util::ConfigurationUtils).to receive(:load).and_call_original
@@ -76,7 +75,7 @@ module LibertyBuildpack
     end
 
     it 'should not write VCAP_SERVICES credentials as debug info',
-       log_level: 'DEBUG' do
+       log_level: 'DEBUG',  enable_log_file: true do
       ENV['VCAP_SERVICES'] = '{"type":[{"credentials":"VERY SECRET PHRASE","plain":"PLAIN DATA"}]}'
       log_content = with_buildpack do |buildpack|
         app_dir = File.dirname buildpack.instance_variable_get(:@lib_directory)
@@ -287,14 +286,16 @@ module LibertyBuildpack
       expect($stderr.string).to match(/No components of type jres defined in components configuration/)
     end
 
-    it 'logs information about the git repository of a buildpack' do
+    it 'logs information about the git repository of a buildpack',
+       log_level: 'DEBUG' do
       with_buildpack { |buildpack| buildpack.detect }
       standard_error = $stderr.string
       expect(standard_error).to match(/git remotes/)
       expect(standard_error).to match(/git HEAD commit/)
     end
 
-    it 'realises when buildpack is not stored in a git repository' do
+    it 'realises when buildpack is not stored in a git repository',
+       log_level: 'DEBUG' do
       Dir.mktmpdir do |tmp_dir|
         Buildpack.stub(:git_dir).and_return(tmp_dir)
         with_buildpack { |buildpack| buildpack.detect }

spec/liberty_buildpack/container/liberty_spec.rb

@@ -411,7 +411,7 @@ def check_default_config(server_xml_file, expected_type, expected_context_root,
       end
 
       it 'should not write VCAP_SERVICES credentials as debug info' do
-        old_level = ENV['JBP_LOG_LEVEL']
+        previous_environment = ENV.to_hash
         begin
           Dir.mktmpdir do |root|
             Dir.mkdir File.join(root, 'WEB-INF')
@@ -421,6 +421,7 @@ def check_default_config(server_xml_file, expected_type, expected_context_root,
             secret = 'VERY SECRET PHRASE'
             plaindata = 'PLAIN DATA'
             ENV['JBP_LOG_LEVEL'] = 'debug'
+            ENV['JBP_CONFIG_LOGGING'] = 'enable_log_file: true'
 
             LibertyBuildpack::Diagnostics::LoggerFactory.send :close # suppress warnings
             LibertyBuildpack::Diagnostics::LoggerFactory.create_logger root
@@ -443,7 +444,7 @@ def check_default_config(server_xml_file, expected_type, expected_context_root,
             expect(log_content).to match(plaindata)
           end
         ensure
-          ENV['JBP_LOG_LEVEL'] = old_level
+          ENV.replace previous_environment
         end
       end
 

spec/liberty_buildpack/diagnostics/logger_factory_spec.rb

@@ -21,13 +21,16 @@ module LibertyBuildpack::Diagnostics
 
   describe LoggerFactory do
 
+    previous_environment = ENV.to_hash
+
     LOG_MESSAGE = 'a log message'
 
     before do
       $stderr = StringIO.new
     end
 
     after do
+      ENV.replace previous_environment
       $stderr = STDERR
     end
 
@@ -148,7 +151,7 @@ module LibertyBuildpack::Diagnostics
     end
 
     it 'should take the default log level from a YAML file' do
-      YAML.stub(:load_file).with(File.expand_path('config/logging.yml')).and_return(
+      LibertyBuildpack::Util::ConfigurationUtils.stub(:load).with('logging', false).and_return(
           'default_log_level' => 'DEBUG')
       Dir.mktmpdir do |app_dir|
         logger = new_logger app_dir
@@ -195,6 +198,7 @@ module LibertyBuildpack::Diagnostics
           logger = new_logger app_dir
           logger.debug(LOG_MESSAGE)
           expect($stderr.string).to match(/#{LOG_MESSAGE}/)
+          expect(File.exists?(log_file(app_dir))).to eq(false)
         ensure
           $VERBOSE = previous_value
         end
@@ -209,6 +213,7 @@ module LibertyBuildpack::Diagnostics
           logger = new_logger app_dir
           logger.debug(LOG_MESSAGE)
           expect($stderr.string).to match(/#{LOG_MESSAGE}/)
+          expect(File.exists?(log_file(app_dir))).to eq(false)
         ensure
           $DEBUG = previous_value
         end
@@ -217,10 +222,11 @@ module LibertyBuildpack::Diagnostics
 
     it 'should send info logs to buildpack.log when info is enabled' do
       Dir.mktmpdir do |app_dir|
+        ENV['JBP_CONFIG_LOGGING'] = 'enable_log_file: true'
         with_log_level('info') do
           logger = new_logger app_dir
           logger.info(LOG_MESSAGE)
-          file_contents = File.read File.join(LibertyBuildpack::Diagnostics.get_diagnostic_directory(app_dir), LibertyBuildpack::Diagnostics::LOG_FILE_NAME)
+          file_contents = File.read(log_file(app_dir))
           expect(file_contents).to match(/#{LOG_MESSAGE}/)
         end
       end
@@ -241,6 +247,10 @@ def with_log_level(log_level)
       end
     end
 
+    def log_file(app_dir)
+      File.join(LibertyBuildpack::Diagnostics.get_diagnostic_directory(app_dir), LibertyBuildpack::Diagnostics::LOG_FILE_NAME)
+    end
+
     def info_method_caller(logger)
       logger.info(LOG_MESSAGE)
     end

spec/liberty_buildpack/jre/memory/weight_balancing_memory_heuristic_spec.rb

@@ -195,7 +195,8 @@
 
   it 'should issue a warning when the specified maximum memory sizes imply the total memory size may be too large',
      memory_limit: '4096m',
-     sizes:        { 'heap' => '800m', 'permgen' => '800m' } do
+     sizes:        { 'heap' => '800m', 'permgen' => '800m' },
+     enable_log_file: true do
 
     output = heuristic.resolve
 
@@ -206,7 +207,8 @@
 
   it 'should issue a warning when the specified maximum memory sizes, including native, imply the total memory size may be too large',
      memory_limit: '4096m',
-     sizes:        { 'heap' => '1m', 'permgen' => '1m', 'stack' => '2m', 'native' => '2000m' } do
+     sizes:        { 'heap' => '1m', 'permgen' => '1m', 'stack' => '2m', 'native' => '2000m' },
+     enable_log_file: true do
 
     output = heuristic.resolve
 
@@ -240,7 +242,8 @@
 
   it 'should issue a warning when the specified maximum heap size is close to the default',
      memory_limit: '4096m',
-     sizes:        { 'heap' => '2049m' } do
+     sizes:        { 'heap' => '2049m' },
+     enable_log_file: true do
 
     heuristic.resolve
 
@@ -249,7 +252,8 @@
 
   it 'should issue a warning when the specified maximum permgen size is close to the default',
      memory_limit: '4096m',
-     sizes:        { 'permgen' => '1339m' } do
+     sizes:        { 'permgen' => '1339m' },
+     enable_log_file: true do
 
     heuristic.resolve
 
@@ -258,7 +262,8 @@
 
   it 'should not issue a warning when the specified maximum permgen size is not close to the default',
      memory_limit: '1G',
-     sizes:        { 'permgen' => '128M' } do
+     sizes:        { 'permgen' => '128M' },
+     enable_log_file: true do
 
     heuristic.resolve
 

spec/liberty_buildpack/util/cache/internet_availability_spec.rb

@@ -42,14 +42,16 @@
     end
   end
 
-  it 'records availability' do
+  it 'records availability',
+     :enable_log_file do
     described_class.instance.available false
 
     expect(described_class.instance.available?).not_to be
     expect(log_contents).not_to match(/Internet availability set to false/)
   end
 
-  it 'records availability with message' do
+  it 'records availability with message',
+     :enable_log_file do
     described_class.instance.available false, 'test message'
 
     expect(described_class.instance.available?).not_to be

spec/liberty_buildpack/util/heroku_spec.rb

@@ -112,7 +112,8 @@ def check_debug_output(env)
       end
     end
 
-    it 'generate without service mappings', log_level: 'DEBUG' do
+    it 'generate without service mappings',
+       log_level: 'DEBUG', enable_log_file: true do
       env = {}
       env['DATABASE_URL'] = 'http://u:p@foo:500/bar'
       env['HEROKU_POSTGRESQL_RED_URL'] = 'postgre://doesnotexist.xyz'
@@ -148,7 +149,8 @@ def check_debug_output(env)
       check_debug_output(env)
     end
 
-    it 'generate with service mappings', log_level: 'DEBUG' do
+    it 'generate with service mappings',
+       log_level: 'DEBUG', enable_log_file: true do
       env = {}
       env['DATABASE_URL'] = 'http://u:p@foo:500/bar'
       env['HEROKU_POSTGRESQL_RED_URL'] = 'postgre://doesnotexist.xyz'

spec/logging_helper.rb

@@ -25,6 +25,7 @@
   include_context 'console_helper'
   include_context 'application_helper'
 
+  previous_log_config = ENV['JBP_CONFIG_LOGGING']
   previous_log_level = ENV['JBP_LOG_LEVEL']
   previous_debug_level = $DEBUG
   previous_verbose_level = $VERBOSE
@@ -35,6 +36,9 @@
     log_level = example.metadata[:log_level]
     ENV['JBP_LOG_LEVEL'] = log_level if log_level
 
+    enable_log_file = example.metadata[:enable_log_file]
+    ENV['JBP_CONFIG_LOGGING'] = 'enable_log_file: true' if enable_log_file
+
     $DEBUG = example.metadata[:debug]
     $VERBOSE = example.metadata[:verbose]
 
@@ -45,6 +49,7 @@
   after do
     FileUtils.rm_rf LibertyBuildpack::Diagnostics.get_diagnostic_directory app_dir
 
+    ENV['JBP_CONFIG_LOGGING'] = previous_log_config
     ENV['JBP_LOG_LEVEL'] = previous_log_level
     $VERBOSE = previous_verbose_level
     $DEBUG = previous_debug_level