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

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? And, do the single quotes inside the quoted dict really work the way you'd expect? Who parses them?

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

                    "You need to provide all the details required to power "
                    "control the node under test. "
                    "You need to provide the --bmc-* parameters if the node "
                    "supports IPMI or use --power-type and --power-parameters "
                    "if you want to use any of the other power types "
                    "supported by MAAS (see %s for a list of the supported "
                    "power types)."

Try to keep elements of a sentence in an easy-to-understand order, so readers don't have to remember or re-read text that may only make sense later. Limit the nesting of phrases. That second sentence is structured like “A if B or C if D (E)” — that's quite hard to read if you're confused, annoyed, under pressure, or not very good at English.

Only use "[statement] if [condition]" if [statement] is very brief and very simple. Even then, keep an eye out for ambiguity around ‘and’/‘or’: is it part of the condition? Or does it start a whole new statement? These are parsing ambiguities. If the reader picks the wrong interpretation at first, they will hit a “parse error” later and have to re-read, which is annoying.

So: don't try to cram all relevant information into your sentence. See how _little_ information you can put in a sentence while still making a clear chain of steps towards full understanding. Good technical writing follows Antoine de Saint Exupéry's rule: “perfection is achieved, not when there is nothing more to add, but when there is nothing more to take away.”

The same things go for these messages:

                self.error(
                    "All the BMC details (MAC address or IP address, "
                    "username and password) must be provided."
                )

...

                self.error(
                    "Value for both --power-type and --power-parameters must "
                    "be provided."

Can you find a way to make these easier to read? Changing from passive voice to active voice is usually a good step.

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?