Code review comment for lp:~xnox/ubiquity/crypto-done

Revision history for this message
Colin Watson (cjwatson) wrote :

This basically looks good, thanks! I confess I haven't gone through all
the state machines in fine detail or anything, but just a few nits and
then I think you should go ahead and merge this so that we can iterate
quickly.

> +Type: Text

Lower-case "text" in all these "Type:" lines.

> + <span foreground="darkred">Warning:</span> If you lose this security key, all data will be lost. if you need to, write down your key and keep it in a safe place elsewhere.

Capital letter at the start of sentences ("If you need to, ..."). Same
in the .ui file.

> - self.assertLessEqual(alloc.width, 640)
> - self.assertLessEqual(alloc.height, 500)
> + self.assertLessEqual(alloc.width, 640, page.module.NAME)
> + self.assertLessEqual(alloc.height, 500, page.module.NAME)

page.module.NAME doesn't seem like a helpful message on test failure.
At least include the expected and observed values.

> + if self.current_page == self.page_crypto and \
> + not self.get_crypto_keys():

Use grouping with parentheses in preference to backslash-continuation
(similarly elsewhere). So:

  if (self.current_page == self.page_crypto and
      not self.get_crypto_keys()):

No doubt we're not entirely consistent about this in existing code, but
let's do this in new code.

> + '^partman-crypto/.*passphrase.*',

> + elif question == 'partman-crypto/weak_passphrase':
> + self.preseed_bool(question, True, seen=False)
> + return True
> +
> + elif question.startswith('partman-crypto/passphrase'):
> + if not self.ui.get_crypto_keys():
> + return False
> + self.preseed(question, self.ui.get_crypto_keys())
> + return True
> +

I'd like to see these two sections match to avoid later problems if the
set of questions asked changes, so I think your entries in the question
list should preferably be:

  '^partman-crypto/weak_passphrase$',
  '^partman-crypto/passphrase.*',

« Back to merge proposal