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

[COOK-2353] split out config from enable action #20

Merged
merged 5 commits into from
Feb 28, 2013

Conversation

dwradcliffe
Copy link
Contributor

This will ensure templates are changed when service is already running. Seems to be working for me but definitely needs to be looked over. There might be a better way to get this done. :)

@@ -250,6 +270,7 @@ def run_script
@run_script.source("sv-#{new_resource.run_template_name}-run.erb")
@run_script.cookbook(template_cookbook)
@run_script.mode(00755)
@run_script.notifies(:restart, "service[#{new_resource.service_name}]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't seem to be working for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

From Seth, how about something like this at the end of the action_enable method?

scripts_updated = [ run_script, log_run_script, finish_script, control_signal_files ].flatten.any? do |r| 
  r.updated_by_last_action?
end

restart_service if scripts_updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The finish and control scripts shouldn't need a restart, since they are run from disk at the time they are called.
I'm still on the fence about the log script, and the more I think about it I'd really like to be able to NOT restart the service in either case.

Example: We use unicorn, and do zero-downtime deploys using USR2 to restart. I really want to control when a restart or a full restart happens, and it's almost never going to be at the time of a chef run.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, untested code and all that :-). So really:

restart_service if run-script.updated_by_last_action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this:

restart_service if new_resource.restart_on_update and run_script.updated_by_last_action?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is restart_on_update? Is that a new attribute you're proposing? Would it be true by default so existing recipes wouldn't need to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 - make sure we have test coverage. :)

@dwradcliffe
Copy link
Contributor Author

Tests added for restart_on_update attribute. All tests passing. Might be good to add a test for the new restart action.

resource.restart_on_update.should be_true
end

it 'hash a restart_on_update parameter that controls whether a the service is restarted when the run script is updated' do

Choose a reason for hiding this comment

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

s/hash/has/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

jtimberman pushed a commit that referenced this pull request Feb 28, 2013
[COOK-2353] split out config from enable action
@jtimberman jtimberman merged commit cfa5cfc into chef-cookbooks:master Feb 28, 2013
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.

3 participants