DarthSim / overmind

Process manager for Procfile-based applications and tmux
MIT License
2.86k stars 81 forks source link

Allow specifying 'all' for process auto restarts #154

Closed dunkmann00 closed 1 year ago

dunkmann00 commented 1 year ago

This closes #153

I ended up going with the suggestion for specifying that all processes should auto restart by passing in all, i.e. overmind start -r all rather than adding another flag that is specifically for auto restarting all, i.e. overmind start --auto-restart-all. I thought this was slightly more intuitive, especially since this is how setting formation is already handled.

Let me know what you think!

dunkmann00 commented 1 year ago

Hey @DarthSim! If you have a chance could you look this over? Thanks!

catpreneur commented 1 year ago

@dunkmann00 Thanks for this pr!

Look at line 96 https://github.com/DarthSim/overmind/blob/7d970570e9208812eaf3782d695bc13700bee48e/start/command.go#L96

They use a boolean to indicate all can die, not all string. Maybe you should make it a boolean so it's consistent.

dunkmann00 commented 1 year ago

Thanks @oofdog! Like I said in my initial comment of this PR, after seeing the suggestion in #153, I thought using all made more sense.

Just to add some more detail - There is precedent for this in Overmind. Setting the number of instances of a process (formation) uses all as well.

From the Readme:

There is a special name all that you can use to scale all processes at once:

$ overmind start -m all=2,worker=5
$ OVERMIND_FORMATION=all=2,worker=5 overmind start

IMHO, I think changing can-die to support all (or any if preferable) would be better for consistency and simplicity.

P.S. Here is where formation is handled in the code:

https://github.com/DarthSim/overmind/blob/7d970570e9208812eaf3782d695bc13700bee48e/start/procfile.go#L43-L48

dunkmann00 commented 1 year ago

Hey @DarthSim! Just wanted to reach out again and see if this PR looks good to you. Thanks!

dunkmann00 commented 1 year ago

Thanks!