Code review comment for lp:~rvb/maas-test/power-type-support

Revision history for this message
Raphaël Badin (rvb) wrote :

> Thanks for working on this. The timeout is no longer needed, I suppose, now
> that the API client is fixed.
>
> Is there a race condition between the node booting and maas-test deleting it?
> I suppose it won't matter until we get magical ultra-fast speed-booting nodes.
>
> I have a few syntactical questions about the documentation:
>
> $ sudo maas-test --maas-series trusty --architecture amd64
> --power-type=amt --power-parameters=\"mac_address=aa:bb:cc:dd:ee:ff
> power_pass='my password'"
>
> What does it mean for the opening double-quote after --power-parameters to be
> escaped, but not the closing quote?

An oversight that had no consequence because of the way the man page is rendered. Fixed.

> And, do the single quotes inside the
> quoted dict really work the way you'd expect? Who parses them?

Yes, there is a test for this. All the parsing is done by utils.make_json_power_parameters().

>
> I would like to recommend a few style rules for technical writing, e.g:
> [...]

Done.

>
> The same things go for these messages:
> [...]

Done.

> But back to code. In convert_bmc_details_into_parameters you use an
> OrderedDict to produce consistent ordering — but the ordering isn't
> documented. Wouldn't it be just as easy to make the tests run
> parameters.split() and do an assertItemsEqual on the result? Or use sorted()
> inside convert_bmc_details_into_parameters?

Good idea, done.

« Back to merge proposal