Merge lp:~racb/maas/arch-detect into lp:~maas-committers/maas/trunk
Proposed by
Robie Basak
Status: | Merged |
---|---|
Approved by: | Robie Basak |
Approved revision: | no longer in the source branch. |
Merged at revision: | 1142 |
Proposed branch: | lp:~racb/maas/arch-detect |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
371 lines (+175/-45) 4 files modified
src/maasserver/api.py (+66/-12) src/maasserver/tests/test_api.py (+36/-25) src/provisioningserver/tests/test_tftp.py (+30/-3) src/provisioningserver/tftp.py (+43/-5) |
To merge this branch: | bzr merge lp:~racb/maas/arch-detect |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email:
|
Commit message
Add architecture detection
Rather than always responding to pxelinux.
This allows multiple architectures to be served the correct pxelinux.cfg configuration for enlistment without any needed intervention.
See bug 1041092 for details on pxelinux.
To post a comment you must log in.
Several minor comments, but they're mostly about style; the approach
is good, afaict.
[1]
+ if macaddress:
+ return macaddress.node
+ else:
+ return None
Don't know how you feel about conditional expressions, but this might
be as clear but more concise as:
return macaddress.node if macaddress else None
[2]
+ :param arch: Architecture name (in the pxelinux namespace, eg. 'arm' not
+ 'armhf').
Typically we align follow-on lines 4-spaces in from the first
line. Yay, another trivial nitpick :)
[3]
+ # Request was pxelinux. cfg/01- <mac>, so attempt fall back cfg/default- <arch> for arch detection.
+ # to pxelinux.
Is it default-<arch> or default.<arch>?
Update: the regex says either.
[4]
+ if pxelinux_arch == 'arm':
+ arch = 'armhf'
+ else:
+ arch = pxelinux_arch
One step in the direction of a lookup-table, and more concise:
arch = {"arm": "armhf" }.get(pxelinux_ arch, pxelinux_arch)
Er, but it's not very pretty. Never mind. Why is "arm" being sent
instead of "armhf"? Should we normalise on just "arm"?
[5]
+ response = self.client. get(reverse( 'pxeconfig' ), default_ params( ))
+ self.get_
We have followed the style of wrapping 4 spaces in most places:
response = self.client.get(
reverse( 'pxeconfig' ), self.get_ default_ params( ))
Also, when we need to break onto a newline, we break at the opening
brace, so not like this:
response = self.client. get(reverse( 'pxeconfig' ),
self. get_default_ params( ))
Too much code on Launchpad ended up in the rightmost 20 columns
(there, like here, we chose to use a right margin) so we changed the
convention.
Hardly needs saying, but Emacs' python-mode does this all perfectly,
right out of the box ;)
[6]
+ ( # either a MAC
Make this a non-capturing group, i.e. (?:...)
+ {htype:02x} # ARP HTYPE. {re_mac_ address. pattern} ) # Capture MAC.
+ -
+ (?P<mac>
+ | # or "default"
+ default
+ ( # perhaps with specified arch
Here too?
+ [.-](?P<arch>\w+) # arch \w+))? # optional subarch
+ (-(?P<subarch>
And here?
+ )?
[7]
+ params = {k: v file_match. groupdict( ).iteritems( )
+ for k, v in config_
+ if v is not None}
We have tended to avoid single-letter variable names (sorry, again,
this is Launchpad engineering heritage showing its face, along with my
pedantry). Also, we decided to use the non-iter methods on dicts, I
think because they're spelled the same as in Python 3. I preferred to
use them, because they behave differently, and 2to3 translates them
anyway, but we, Red Squad, in days of old, agreed this. So, we'd write
this as:
params = {
key: value file_match. groupdict( ).items( )
for key, value in config_
if value is not None
}