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

Defaults shouldn't override previously set variables #210

Merged
merged 1 commit into from
Feb 4, 2015

Conversation

kdonovan
Copy link

@kdonovan kdonovan commented Feb 3, 2015

In our application we set the rollbar_token from environmental variables for security, but when rollbar loads itself up it blows away the already-set variables with it's defaults.

@brianr
Copy link
Member

brianr commented Feb 3, 2015

Thanks @kdonovan! This looks good to me. @jondeandres can you review as well?

@jondeandres
Copy link
Contributor

@brianr it's nice for me.

brianr added a commit that referenced this pull request Feb 4, 2015
Defaults shouldn't override previously set variables
@brianr brianr merged commit 8f3fe7e into rollbar:master Feb 4, 2015
@brianr
Copy link
Member

brianr commented Feb 4, 2015

Merge, thanks @kdonovan! Will get this out in the next release.

@brianr
Copy link
Member

brianr commented Feb 4, 2015

Released in 1.4.3 (now on rubygems)

@shaneog
Copy link

shaneog commented Feb 5, 2015

This is breaking deploys for me. The problem is that by the time the Rollbar load:defaults task is run, the deploy.rb file has not been parsed, and thus my set :rollbar_token, ENV['ROLLBAR_ACCESS_TOKEN'] has not been run.
So I'm consistently getting the message Please specify the Rollbar access token, set :rollbar_token, 'your token'

@kdonovan How/where are you loading the Rollbar access token ENV variable?

@jondeandres
Copy link
Contributor

I've detected the problem here.

fetch is not so safe cause it evaluates the default value if it's callable, https://github.com/capistrano/capistrano/blob/master/lib/capistrano/configuration.rb#L41.

We can do something crazy like Proc.new { Proc.new { ... } }, but is really ugly.

@kdonovan could you provide us the loading order you have when setting :rollbar_token, require 'rollbar/capistrano3 and others?

It seems you are setting :rollbar_token before `require 'rollbar/capistrano3' is executed, could be?

My suggestion is change that order, and we'll then revert this PR since it's being a problem for most of the users.

@jondeandres
Copy link
Contributor

@kdonovan I could reproduce your problem with this code

task :foo do
  set :rollbar_token, proc { 'foo' }
end

before 'load:defaults', 'foo'

Are you using something similar? In that case 'load:defaults' will override your values.

We can fix this for everybody with this code:

namespace :load do
  task :defaults do
    set :rollbar_user,  Proc.new { ENV['USER'] || ENV['USERNAME'] } unless fetch(:rollbar_user)
    set :rollbar_env,   Proc.new { fetch :rails_env, 'production' } unless fetch(:rollbar_env)
    set :rollbar_token, Proc.new { abort "Please specify the Rollbar access token, set :rollbar_token, 'your token'" } unless fetch(:rollbar_token)
    set :rollbar_role,  Proc.new { :app } unless fetch(:rollbar_role)
  end
end

But probably the best is just not set anything before 'load:defaults' is executed, https://github.com/capistrano/capistrano/blob/master/lib/capistrano/setup.rb#L13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants