Add restart cmd + auto/start, stop idempotency. #58
Conversation
| } | ||
|
|
||
| args, err := shlex.Split(service.Command) | ||
| commandString := os.Expand(service.Command, func(k string) string { |
There was a problem hiding this comment.
I think this is a good change, but it doesn't look like it's documented (in the README) or tested anywhere. Can we add those?
Nit: what about calling this commandExpanded?
There was a problem hiding this comment.
Oh yeah I forgot I had made this change, I'll make sure to add some tests and documentation.
| @@ -134,6 +150,13 @@ func (m *ServiceManager) doStart(task *state.Task, tomb *tomb.Tomb) error { | |||
| go func() { | |||
| active.err = cmd.Wait() | |||
| _ = active.logBuffer.Close() | |||
| releasePlan, err := m.acquirePlan() | |||
| if err == nil { | |||
| if m.services[req.Name].cmd == cmd { | |||
There was a problem hiding this comment.
What does this if statement check for? (I realize it was there already below.)
There was a problem hiding this comment.
acquirePlan wouldn't actually fail here, since the plan is loaded already. I guess I can make this more future proof by relying on the mutex directly incase acquirePlan implementation changes.
hpidcock
force-pushed
the
restart
branch
2 times, most recently
from
e000d45
to
3fadf14
Compare
| startTasks.WaitAll(stopTasks) | ||
| taskSet = state.NewTaskSet() | ||
| taskSet.AddAll(stopTasks) | ||
| taskSet.AddAll(startTasks) |
There was a problem hiding this comment.
It's nice that both the stop order and the start order is correct here.
| for id := range successors { | ||
| id := string(string(id)) |
There was a problem hiding this comment.
That's a funny left over from an automatic replacement. The id used to be an ObjectId type.
|
Btw, please keep an eye on the commit message. The description of this PR looks out of date, referring to the env change which was split out. |
This PR implements the
restartcommand for (re)starting services.Additionally
Resolves #47