Merge lp:~jason-hobbs/maas/lp-1210393 into lp:~maas-committers/maas/trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Jason Hobbs | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 1991 | ||||
Proposed branch: | lp:~jason-hobbs/maas/lp-1210393 | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
456 lines (+329/-44) 2 files modified
etc/maas/templates/commissioning-user-data/snippets/maas_ipmi_autodetect.py (+91/-26) etc/maas/templates/commissioning-user-data/snippets/tests/test_maas_ipmi_autodetect.py (+238/-18) |
||||
To merge this branch: | bzr merge lp:~jason-hobbs/maas/lp-1210393 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+207288@code.launchpad.net |
Commit message
Refactor user selection.
Don't assume a fixed number of users, or rely on any given user existing. Instead, ask the BMC what users exist, and then inspect users from that list. This eliminates the dependency on a 'User10' existing.
Pick the first user with username 'maas'. If no user with username 'maas' exists, pick the first user that has no username set. Otherwise, fail IPMI setup.
Description of the change
Refactor user selection.
Don't assume a fixed number of users, or rely on any given user existing. Instead, ask the BMC what users exist, and then inspect users from that list. This eliminates the dependency on a 'User10' existing.
Pick the first user with username 'maas'. If no user with username 'maas' exists, pick the first user has no username set. Otherwise, fail IPMI setup.
Looks good, thanks again!
[1]
+ def test_runs_ bmc_config( self): maas_ipmi_ autodetect, 'run_command') section( section) call_args[ 0][0] l('bmc- config' , args[0])
+ """Ensure bmc-config is called with the correct args."""
+ recorder = self.patch(
+
+ section = 'foo'
+ bmc_get_
+ args = recorder.
+
+ self.assertEqua
+
+ for arg in ['--checkout', '--section', section]:
+ self.assertIn(arg, args)
The ordering is static so you can save some pain by using:
It'll also give a more useful error message if it fails.
I think there's also an assert_ called_ with() that matches any of
multiple calls, but if you need it you ought to check.
[2]
+ bgs_mock = Mock(return_ value=user_ text) maas_ipmi_ autodetect, 'bmc_get_section', bgs_mock) value=( user_number, user_record)) maas_ipmi_ autodetect, 'parse_section', ps_mock)
+ self.patch(
+ ps_mock = Mock(return_
+ self.patch(
self.patch() does some of the above for you. You could also write it as:
bgs_mock = self.patch( maas_ipmi_ autodetect, 'bmc_get_section')
bgs_mock. return_ value = user_text maas_ipmi_ autodetect, 'parse_section')
ps_mock. return_ value = (user_number, user_record)
ps_mock = self.patch(
It only saves the import of Mock, but you might like it.
[3]
+ self.assertEqua l(user_ number, bgs_mock. call_args[ 0][0]) l(user_ text, ps_mock. call_args[ 0][0])
+ self.assertEqua
Again, assert_ called_ once_with( ) is your friend.
If you need to match something with multiple arguments, but you can only
about one or two, there's ANY:
from mock import ANY
thing. assert_ called_ once_with( "important" , ANY, keyword=ANY)
There's also call(), which also works with ANY:
from mock import call
self.assertEqu al(
[call( 1, ANY), call(2, ANY)],
thing. call_args_ list)
[4]
+def disabled_ user(update= None):
+ """Record for a disabled user."""
+ base = {'Enable_User': 'No'}
+
+ if update:
+ base.update(update)
+
+ return base
Can you name this make_disabled_ user()? All test factories have the
prefix "make", except where they don't, er, they have the prefix "get".
[5]
+def base_attributes (attributes_ update= None): deepcopy( attributes_ template) )
+ """Base user records with updates in an OrderedDict."""
+
+ attributes_template = {
+ """Starting point for user records."""
+ 'User1': {
+ 'Enable_User': 'Yes'
+ },
+ 'User2': {
+ 'Username': 'admin',
+ 'Enable_User': 'Yes'
+ }
+ }
+
+ base = OrderedDict(
I don't think the deepcopy() is necessary.
[6]
+ if attributes_update: attributes_ update)
+ base.update(
Another habit we have is never relying on the implicit boolean-ness of
things in Python. Explicitly test for None instead:
if attributes_update is not None:
base.update( attributes_ update)
[7]
+ ('one_disabled_ not_blank_ user', dict(
Fwiw, scenario names don't have to be valid identifiers; they're
free-form text I ...