Code review comment for lp://staging/~rvb/maas/dhcp-stop-bug-1328656

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

> > Also I'd at least normalise whitespace:
> > ' '.join(output.strip().split()). It may even be worth a dedicated test.
>
> Why do you think this is worth the trouble? I mean, this error string is
> present in upstart's source code.

Because nobody will think twice about changing the handling of whitespace, not just in Upstart but also in how we process error output. For example we might reduce stripping, and suddenly get a newline at the end. I don't care much about the internal whitespace.

> > You may also want to say that (apparently) the error ends in a colon, with
> > nothing to follow. It looks weird, and reading it I sort of expected a
> > "message begins with" check.
>
> I know it looks weird, but that's what the error looks like. I could relax
> the condition a bit (by just checking that the error contains "Unknown
> instance") but I think it's much more important not to hide legitimate errors
> so I'd rather keep a very precise check. The worse that can happen is that
> the user will see misleading stacktraces: painful but not as misleading as
> missing valid errors.

I'm not asking you to change the code, only to document that the error is weird.

« Back to merge proposal