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

Notify deploy only on primary :rollbar_role server #368

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

theist
Copy link
Contributor

@theist theist commented Jan 28, 2016

Hi, I've seen that on every deploy on a multi app node the deploy gets noticed once for each node which get the deploy, so on a tree app node connected to rollbar there's three notifications of the same deploy which makes tree notices on a slack channel if you have slack and rollbar paired.

So perhaps it's a good idea notify the deploy only on primary server.

Important note: I've made this PR using the github editor and my knowledge of capistrano so it is not properly tested. I planed only to open an issue but the change is so small that I've created it as PR.

@jondeandres
Copy link
Contributor

Hi @theist,

I think the PR is ok, but I'll need to check the compatibility with different versions of capistrano.

Have you know the behavior if the user has only one server and didn't flag the server as primary? Maybe the task will not be executed?

Thanks

@theist
Copy link
Contributor Author

theist commented Jan 28, 2016

I'm used to the last version of capistrano. On 3.4 the first server defined for a role is automagically defined as primary so primary :role always return a host. But capistrano changes fast. Perhaps at weekend I be able to do some homework, and check properly this PR

@theist
Copy link
Contributor Author

theist commented Feb 9, 2016

Hi @jondeandres

I've checked capistrano 3.0 and 3.1 behaviour about roles/primary

As in 3.4, in capistrano 3.0 the first server for a defined role is automatically considered primary. So there's always a primary and only one server for each role considered "primary"

This is the current template of "stage" and the text is similar in 3.1 https://github.com/capistrano/capistrano/blob/master/lib/capistrano/templates/stage.rb.erb#L15

I think that there's the code which selects primary :
https://github.com/capistrano/capistrano/blob/master/lib/capistrano/configuration/servers.rb#L50

I also tested capistrano 3.0/3.1 with this config

server '127.1.1.1', user: 'carlos', roles: %w{web app}
server '127.2.2.2', user: 'carlos', roles: %w{web app}
server '127.3.3.3', user: 'carlos', roles: %w{web app}

require "pry"
task :debug do
  binding.pry
end

And did these tests with pry (this is cap 3.0)

[3] pry(main)> on roles(:app) do |host| 
[3] pry(main)*   puts host.hostname 
[3] pry(main)* end  
127.2.2.2
127.3.3.3
127.1.1.1
=> [#<Thread:0x007f25da660e98@/home/carlos/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/sshkit-1.7.1/lib/sshkit/runners/parallel.rb:11 dead>,
 #<Thread:0x007f25da660d58@/home/carlos/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/sshkit-1.7.1/lib/sshkit/runners/parallel.rb:11 dead>,
 #<Thread:0x007f25da660c40@/home/carlos/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/sshkit-1.7.1/lib/sshkit/runners/parallel.rb:11 dead>]

[4] pry(main)> primary(:app)
=> #<Capistrano::Configuration::Server:0x007fb6610615c0
 @hostname="127.1.1.1",
 @port=nil,
 @properties=
  #<Capistrano::Configuration::Server::Properties:0x007fb661057ca0
   @properties={},
   @roles=#<Set: {:web, :app}>>,
 @user="carlos">

[5] pry(main)> primary(:app).hostname
=> "127.1.1.1"

[6] pry(main)> on primary(:app) do |host|
[6] pry(main)*   puts host.hostname
[6] pry(main)* end  
=> [#<Thread:0x007fb66341f250@/home/carlos/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/sshkit-1.7.1/lib/sshkit/runners/parallel.rb:11 dead>]

[7] pry(main)> 

@jondeandres
Copy link
Contributor

Awesome @theist! Thanks for debugging this and resolve the doubts. I'll merge this today, thanks for this!

@jondeandres
Copy link
Contributor

@theist could you sync the branch with master? We fixed a problem with the sucker punch version and your tests are failing cause that.

Thanks!

Hi, I've seen that on every deploy on a multi app node the deploy gets noticed once for each node which get the deploy, so on a tree app node connected to rollbar there's three notifications of the same deploy which makes tree notices on a slack channel if you have slack and rollbar paired.

So perhaps it's a good idea notify the deploy only on primary server.

*Important note*: I've made this PR using the github editor and my knowledge of capistrano so it is not properly tested. I planed only to open an issue but the change is so small that I've created it as PR.
@theist
Copy link
Contributor Author

theist commented Feb 10, 2016

Took two tries but seems ok now. Sorry about the double CI cycle. Now its pushed only my change against the current master.

@jondeandres
Copy link
Contributor

awesome @theist, thanks!

jondeandres added a commit that referenced this pull request Feb 11, 2016
Notify deploy only on primary :rollbar_role server
@jondeandres jondeandres merged commit e69a3e9 into rollbar:master Feb 11, 2016
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.

2 participants