Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rails): Log missing key warning after Rails initialization completes #444

Merged
merged 3 commits into from
Apr 6, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions lib/bugsnag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,10 @@ class << self
# Configure the Bugsnag notifier application-wide settings.
#
# Yields a configuration object to use to set application settings.
def configure
def configure(validate_api_key=true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a named parameter as calling configure(false|true) is not obvious what it will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting Ruby 1.9.3 makes this difficult as 2.0.0 introduced named parameters. It could potentially be something for the next major release.

yield(configuration) if block_given?

@key_warning = false unless defined?(@key_warning)
if !configuration.valid_api_key? && !@key_warning
configuration.warn("No valid API key has been set, notifications will not be sent")
@key_warning = true
end
check_key_valid if validate_api_key
end

##
Expand Down Expand Up @@ -179,6 +175,15 @@ def load_integration(integration)
configuration.debug("Integration #{integration} is not currently supported")
end
end

# Check if the API key is valid and warn (once) if it is not
def check_key_valid
@key_warning = false unless defined?(@key_warning)
if !configuration.valid_api_key? && !@key_warning
configuration.warn("No valid API key has been set, notifications will not be sent")
@key_warning = true
end
end
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/integrations/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class Railtie < Rails::Railtie

config.before_initialize do
# Configure bugsnag rails defaults
Bugsnag.configure do |config|
Bugsnag.configure(false) do |config|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a comment might be good to explain why we are not validating the key here

config.logger = ::Rails.logger
config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] || ::Rails.env.to_s
config.project_root = ::Rails.root.to_s
Expand Down
26 changes: 26 additions & 0 deletions spec/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,32 @@ def debug(name, &block)
end
end

it "logs a warning when configuring without an API key" do
Bugsnag.configuration.api_key = nil
Bugsnag.instance_variable_set("@key_warning", nil)
ENV['BUGSNAG_API_KEY'] = nil
expect(@logger.logs.size).to eq(0)
Bugsnag.configure
expect(@logger.logs.size).to eq(1)
log = @logger.logs.first
expect(log).to eq({
:level => "warning",
:name => "[Bugsnag]",
:message => "No valid API key has been set, notifications will not be sent"
})
end

it "skips logging a warning when configuring with an API key" do
Bugsnag.configuration.api_key = nil
Bugsnag.instance_variable_set("@key_warning", nil)
ENV['BUGSNAG_API_KEY'] = nil
expect(@logger.logs.size).to eq(0)
Bugsnag.configure do |config|
config.api_key = 'd57a2472bd130ac0ab0f52715bbdc600'
end
expect(@logger.logs.size).to eq(0)
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth adding similar test cases for when we pass in the new method parameter in these scenarios?

it "should log info messages to the set logger" do
expect(@logger.logs.size).to eq(0)
Bugsnag.configuration.info("Info message")
Expand Down
4 changes: 4 additions & 0 deletions spec/fixtures/apps/rails-initializer-config/Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
source 'https://rubygems.org'

gem 'railties', '4.2.10', require: %w(action_controller rails)
gem 'bugsnag', path: '../../../..'
16 changes: 16 additions & 0 deletions spec/fixtures/apps/rails-initializer-config/config.ru
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
Bundler.require

run InitializerConfigApp ||= Class.new(Rails::Application) {
config.secret_key_base = routes.append {
root to: proc {
[200, {"Content-Type" => "text/plain"}, ["Hello!"]]
}
}.to_s

config.cache_classes = true
config.eager_load = true
config.logger = Logger.new(STDOUT)
config.log_level = :warn

initialize!
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Bugsnag.configure do |config|
config.api_key = 'c34a2472bd240ac0ab0f52715bbdc05d'
end
4 changes: 4 additions & 0 deletions spec/fixtures/apps/rails-no-config/Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
source 'https://rubygems.org'

gem 'railties', '4.2.10', require: %w(action_controller rails)
gem 'bugsnag', path: '../../../..'
16 changes: 16 additions & 0 deletions spec/fixtures/apps/rails-no-config/config.ru
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
Bundler.require

run NoConfigApp ||= Class.new(Rails::Application) {
config.secret_key_base = routes.append {
root to: proc {
[200, {"Content-Type" => "text/plain"}, ["Hello!"]]
}
}.to_s

config.cache_classes = true
config.eager_load = true
config.logger = Logger.new(STDOUT)
config.log_level = :warn

initialize!
}
3 changes: 3 additions & 0 deletions spec/fixtures/apps/scripts/Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
source 'https://rubygems.org'

gem 'bugsnag', path: '../../../..'
5 changes: 5 additions & 0 deletions spec/fixtures/apps/scripts/configure_key.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
require 'bugsnag'

Bugsnag.configure do |config|
config.api_key = 'f25a2472bd230ac0ab0fa2715bbdc654'
end
3 changes: 3 additions & 0 deletions spec/fixtures/apps/scripts/no_config.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
require 'bugsnag'

Bugsnag.configure
106 changes: 106 additions & 0 deletions spec/integrations/logger_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
require 'spec_helper'

describe 'Configuration.logger' do

before do
@env = {}
end

context 'in a Rails app' do
key_warning = '[Bugsnag]: No valid API key has been set, notifications will not be sent'
is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby'
incompatible = (RUBY_VERSION < '2.0.0') || is_jruby

before do
skip "Incompatible with Ruby <2.0 and JRuby" if incompatible
@env['RACK_ENV'] = 'production'
end

def run_app(name)
out_reader, out_writer = IO.pipe
Dir.chdir(File.join(File.dirname(__FILE__), "../fixtures/apps/#{name}")) do
Bundler.with_clean_env do
pid = Process.spawn('bundle install',
out: out_writer.fileno,
err: out_writer.fileno)
Process.waitpid(pid, 0)
pid = Process.spawn(@env, 'bundle exec rackup config.ru',
out: out_writer.fileno,
err: out_writer.fileno)
sleep(2)
Process.kill(1, pid)
end
end
out_writer.close
output = ""
output << out_reader.gets until out_reader.eof?
output
end
context 'sets an API key using the BUGSNAG_API_KEY env var' do
it 'does not log a warning' do
skip "Incompatible with Ruby <2.0 and JRuby" if incompatible
@env['BUGSNAG_API_KEY'] = 'c34a2472bd240ac0ab0f52715bbdc05d'
output = run_app('rails-no-config')
expect(output).not_to include(key_warning)
end
end

context 'sets an API key using the bugsnag initializer' do
it 'does not log a warning' do
skip "Incompatible with Ruby <2.0 and JRuby" if incompatible
output = run_app('rails-initializer-config')
expect(output).not_to include(key_warning)
end
end

context 'skips setting an API key' do
it 'logs a warning' do
skip "Incompatible with Ruby <2.0 and JRuby" if incompatible
output = run_app('rails-no-config')
expect(output).to include(key_warning)
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test in here where the env variable is provided but is not a valid api key and configure is not called might be a good addition


context 'in a script' do
key_warning = /\[Bugsnag\] .* No valid API key has been set, notifications will not be sent/

def run_app(name)
out_reader, out_writer = IO.pipe
Dir.chdir(File.join(File.dirname(__FILE__), "../fixtures/apps/scripts")) do
Bundler.with_clean_env do
pid = Process.spawn(@env, "bundle exec ruby #{name}.rb",
out: out_writer.fileno,
err: out_writer.fileno)
Process.waitpid(pid, 0)
end
end
out_writer.close
output = ""
output << out_reader.gets until out_reader.eof?
output
end

context 'sets an API key using the BUGSNAG_API_KEY env var' do
it 'does not log a warning' do
@env['BUGSNAG_API_KEY'] = 'c34a2472bd240ac0ab0f52715bbdc05d'
output = run_app('no_config')
expect(output).not_to match(key_warning)
end
end

context 'sets an API key using Bugsnag.configure' do
it 'does not log a warning' do
output = run_app('configure_key')
expect(output).not_to match(key_warning)
end
end

context 'skips setting an API key' do
it 'logs a warning' do
output = run_app('no_config')
expect(output).to match(key_warning)
end
end
end
end