cmd/run: detect simulation errors and exit with appropriate rc #53
Conversation
| printGoodbye(failedSimNames != nil) | ||
| if failedSimNames != nil { | ||
| msg := fmt.Sprintf( | ||
| "The following simulations experienced errors: %v", | ||
| strings.Join(failedSimNames, ", ")) | ||
| return errors.New(msg) | ||
| } |
There was a problem hiding this comment.
printGoodbye is a function which is called only once, so the only reason for its existence is to make the code cleaner. Splitting half of the goodbye message into printGoodbye and leaving another half inlined in the function doesn't make sense. We should either pass the failed simulations into the printGoodbye or remove printGoodbye and inline everything.
| @@ -412,6 +433,7 @@ func run(sims []*Simulation, bind simulator.BindAddr, size int) error { | |||
| // TODO: some module can return custom messages (e.g. hijack) | |||
| // and "ERROR" prefix shouldn't be printed then | |||
| printMsg(sim, fmt.Sprintf("ERROR: %s: %s", host, err.Error())) | |||
| failedSims[sim.Name()] = true | |||
There was a problem hiding this comment.
I wonder if this won't be misleading in some scenarios. What if we run the simulation against 10 hosts and one of them fails? Should we report the whole module as failed?
I don't have an answer here, but we need to define what it means for the module to fail. Maybe simple boolean flag is not enough.
There was a problem hiding this comment.
Also most probably we should extract this for loop body into a separate function/structure so we don't repeat the errors handling (printMsg+failedSims). But we before we do so we should address what we really want to report at the end and how to handle partial failures.
There was a problem hiding this comment.
ACK... I was a little concerned that a single host failure was a bit too coarse. I'll mark this as a draft PR until we discuss further.
Thanks for the review.
Something along these lines: