Code review comment for lp:~newell-jensen/maas/fix-1553841

Revision history for this message
Newell Jensen (newell-jensen) wrote :

> Code looks fine to me. Just a couple things I'm wondering:
>
> * Can you update the commit message so that it is more accurate? (it makes it
> sound like error propagation in general, but the branch seems to just fix up
> the AMT exceptions so that they are thrown correctly.)
>
> * Are we confident the AMT utilities will always print these particular
> strings? (I assume we're using a consistent locale, so that the strings don't
> show up translated into another language and surprise us.

The subprocesses are executed using select_c_utf8_locale() to set the environment.

 And I was wondering
> if using just the HTTP status code for matching might be more future-proof...
> assuming the output isn't too large, and we could match the number in multiple
> places in the string...?)

I am on the fence about changing what we are matching on. Here are the two errors that amttool gives back:

401 Unauthorized at /usr/bin/amttool line 126.

and

500 Can't connect to 172.24.124.257:16992 at /usr/bin/amttool line 126.

If you think we should match just on the error status code and don't think it will cause any future issues, that is fine with me. These messages are coming back from amttool, so how I have it now seems fine to me.

« Back to merge proposal