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

artifact_deploy always notifies #57

Open
andrewGarson opened this issue Mar 12, 2013 · 6 comments
Open

artifact_deploy always notifies #57

andrewGarson opened this issue Mar 12, 2013 · 6 comments
Labels
Milestone

Comments

@andrewGarson
Copy link
Contributor

action :deploy always calls

new_resource.updated_by_last_action(true)

This should only be called if something actually gets updated.

Maybe call this only if you need to redploy and leave it up to proc writers to call it when their proc changes something in the system

@KAllan357
Copy link
Contributor

In general, we've advocated the usage of the provided restart Proc to act as a way to execute resources that need to react when a new artifact has been deployed. That being said, due to the architecture of the resource, a bunch of extra Chef contexts are created, so the notifies syntax is probably all out of whack.

Chef 11 has support that they document here under the section labeled "Inline Compile Mode for LWRPs". I'm not sure if supporting this would fix this problem.

I'm still not entirely sure I'm convinced as to whether or not this is something worth fixing, there is so much customizability build into the resource, it's hard to think of a use-case where using the restart Proc doesn't solve it.

@jonathanq
Copy link
Contributor

One issue I've come across is when you have a configuration file written out in one Proc, you aren't able to notify another service defined in another to restart.

This would be a case where a configuration file was updated during Configure but was not related to an actual deploy.

In my after_deploy Proc I already added the if-block from #68 (although I had to do a slight modification, the not_if example doesn't seem to work)

    configure Proc.new {
      template "#{shared_path}/my.conf" do
        source 'template.conf.erb'
        owner 'foo'
        group 'foo'
        mode '644'
      end
    }

    after_deploy Proc.new {
      if current_symlink_changing?
        service 'nginx' do
          action :restart
        end
      else
        Chef::Log.debug 'Not restarting processes no deployment was done'
      end
    }

Ideally you would be able to just notify the service 'nginx' to restart in the configure Proc when a value changes. But I would settle for some sort of a temp value similar to "current_symlink_changing" that you could use to pass state between Procs. Although that could get messy real fast if you need to notify different services based on the file updated.

@gregsymons
Copy link
Contributor

Here's a use case for fixing this issue that the restart proc can't solve:

I have multiple packages that I need to deploy, and if any of them are updated, I need to restart tomcat. I only want to restart tomcat once because there are things that happen during application startup that get very unhappy if they are interrupted, and could potentially put us into a state where we can't start at all. Therefore I have to use notifications so that all those restarts get coalesced. But if the deploy resource always notifies, now I'm restarting tomcat every time chef runs.

@KAllan357
Copy link
Contributor

Thanks, @gregsymons, I can see how that is an issue.

I'd like to come back and take a look at this at some point, though I haven't had the time recently. We are definitely open to pull requests though.

@gregsymons
Copy link
Contributor

I did find a workaround that works:

needs_restart = false

artifacts.each do |artifact|
   artifact_deploy "package-#{artifact}" do
   ...
   ...
     restart Proc.new {
       needs_restart = true
     }
   end
end

ruby_block 'notify-tomcat-restart' do
  block {}
  notifies :restart, 'service[tomcat]'
  only_if { needs_restart }
end

It's a little ugly, but it works. If I get some time I'll see if I can work on a pull request to fix the bug (perhaps after I do the work for #96 😄 )

@fujin
Copy link

fujin commented Aug 29, 2014

ping any updates here, just saw this https://gist.github.com/fujin/935677bb1303d7ad85db

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

No branches or pull requests

5 participants