Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Add additional supervisorctl commands as actions #2

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

devsibwarra
Copy link

I'm working to replace the poise supervisor cookbook with yours. Needed a couple tweaks to make that happen, but it's looking great so far!

Changes

  • supervisor_process
    • added restart action
  • supervisor_service
    • renamed reload action to reread, added alias for reload
    • added update action
    • added shutdown action

Not sure what spec or test updates would be needed for these changes, so I'm happy for guidance on that

@RulerOf
Copy link
Contributor

RulerOf commented Apr 2, 2019

The addition of the :restart action to the Process is great, and looks good to me. It can't really be tested with an InSpec integration test, but a unit test would work fine. I can write both and don't mind updating this PR with them if you need any help, just LMK.

I like the additional functionality to the service, but I'm working with this cookbook under the impression that the supervisor_service resource's actions are modeled after traditional chef service resources. Getting those typical effects requires some translation between the supervisorctl commands and the traditional chef service resource verbs.

Also, now that I'm both reading the supervisorctl docs and referencing another github issue about how unclear these verbs are, I can see that even my patch wasn't quite right.

I suggest the following for supervisor_service:

  • :reload should perform supervisorctl update
  • :restart should perform supervisorctl reload
  • :stop should send SIGTERM to the supervisord process.
    • Add a stop_signal 'SIGTERM' to the service definition and then your :shutdown action can be renamed

Other actions can be aliased to these or can be more specific (such as an actual reread action that performs supervisorctl reread), but the above is what I would suggest in order to have the resource behave the way that most Chef users would expect it to.

@devsibwarra
Copy link
Author

devsibwarra commented Apr 2, 2019

Thanks for taking a look at this. I chose supervisor command names just to make it easier to understand from a supervisorctl perspective, but having some of the standard Chef actions would be ideal. I'll work on morphing those changes into the branch tomorrow. Feel free to add unit tests to this branch.

@devsibwarra
Copy link
Author

@RulerOf I updated the supervisor_service with your suggestions, included the help output line just to help understand why those actions call those supervisorctl commands, and tried my hand at cleaning up the duplication.

What are your thoughts on the notifies action sent by the other resources?

@RulerOf
Copy link
Contributor

RulerOf commented Apr 10, 2019

@RulerOf I updated the supervisor_service with your suggestions, included the help output line just to help understand why those actions call those supervisorctl commands, and tried my hand at cleaning up the duplication.

That makes sense.

What are your thoughts on the notifies action sent by the other resources?

I started preparing a PR over the weekend which you'll be able to merge into this one that contains some minimal unit testing and a few minor changes to the notifications to get the integration tests passing again. I started refactoring portions of the code base to make the unit tests possible (chefspec has some limits) and realized I should probably save those for a different PR altogether.

I'll take some time out of my evening tomorrow to get the changes submitted here.

@devsibwarra
Copy link
Author

Having a working supervisor cookbook that supports newer Chef releases was my motivation for this work. Feels closer than ever now! Thanks for your help / suggestions so far

@8la
Copy link

8la commented Jul 11, 2019

Any update on this guys? we'll love to have a supervisor cookbook replacement also.

@RulerOf
Copy link
Contributor

RulerOf commented Jul 11, 2019

This is still on my list of things I'm perpetually procrastinating on. I did some of the work back in April but still haven't finished it.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants