Code review comment for lp:~fcorrea/landscape-client/add-otp-field

Nice working, marking needs fixing mainly because of [1].

Thanks!

[1]

+ elif self._config.provisioning_otp:
+ return True

I think this will make the client try to register again every time it's started. It should be probably

return (not id.secure_id) and self._message_store.accepts("register-provisioned-machine")

the (not id.secure_id) means that the computer has not a secure ID yet (which afair is something that the client get and stores after a successful registration) and self._message_store.accepts("register-provisioned-machine") ensures that the server actually supports that kind of message (in principle it might be an older server that does not support it yet).

[2]

+ logging.info(u"Queueing message to register with OTP as a"
+ u" provisiong machine.")

I'd suggest s/provisioning/newly provisioned/

[3]

+ {"otp": Any(String(), Constant(None))})

This should be just

    {"otp": String()})

as the client must never send an otp with a None value.

[4]
+ """
+ If the provisioning OTP is specified, there is no need to pass the
+ account name and the computer title.
+ """

This is fine, but I just wanted to remind that in practice what we will do is probably to include the account name and computer title in the configuration file (via debconf), for sake of reference. However I think this code doesn't need to be changed in that case.