Code review comment for lp:~justin-fathomdb/nova/check-subprocess-exit-code

Revision history for this message
termie (termie) wrote :

* Please change all variable and parameter names to match Twisted's (camelCase)

* When you write a note about something (you have a comment in an exception below) please use

NOTE(justinsb): comment text

rather than

comment text (justinsb)

* Please make the lines fit in 80 characters.

* Your comment at the beginning of BackRelay has odd spacing.

* Technically the pip change (good catch) should be a different branch, it is fine now that it is already in there but please keep changes as small and specific as possible.

In general I don't think we should change the behavior of how "errortoo" works.

Specifically I don't think the majority of the changes in the twisted code are necessary, it looks like simply making a failure state in processEnded if checkExitCode is true and the exit code is non-zero is enough. I agree that we should also always store the stderr separately in addition to what twisted is doing so that we can give it back when the exit code is non-zero, but that should be an add-on to existing behavior, not a modification.

review: Needs Fixing

« Back to merge proposal