Merge lp:~andreserl/maas/fix_ipmi_lp1234880_1.4 into lp:maas/1.4
Proposed by
Andres Rodriguez
| Status: | Merged |
|---|---|
| Approved by: | Raphaël Badin |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 1716 |
| Proposed branch: | lp:~andreserl/maas/fix_ipmi_lp1234880_1.4 |
| Merge into: | lp:maas/1.4 |
| Diff against target: |
69 lines (+21/-9) 2 files modified
etc/maas/templates/commissioning-user-data/snippets/maas_ipmi_autodetect.py (+18/-7) etc/maas/templates/commissioning-user-data/snippets/maas_signal.py (+3/-2) |
| To merge this branch: | bzr merge lp:~andreserl/maas/fix_ipmi_lp1234880_1.4 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jeroen T. Vermeulen (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Fix ipmi version autodetection
To post a comment you must log in.
Revision history for this message
| Julian Edwards (julian-edwards) wrote | # |
Revision history for this message
| Jeroen T. Vermeulen (jtv) wrote | # |
Revision history for this message
| Julian Edwards (julian-edwards) wrote | # |
Revision history for this message
| Jeroen T. Vermeulen (jtv) wrote | # |
Revision history for this message
| Julian Edwards (julian-edwards) wrote | # |
Revision history for this message
| Julian Edwards (julian-edwards) wrote | # |
Revision history for this message
| MAAS Lander (maas-lander) wrote | # |
Download full text (87.5 KiB)
Revision history for this message
| Julian Edwards (julian-edwards) wrote | # |
Revision history for this message
| MAAS Lander (maas-lander) wrote | # |
Download full text (25.0 KiB)
Revision history for this message
| MAAS Lander (maas-lander) wrote | # |
Download full text (20.6 KiB)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Looks good, thanks! Since the snippets are increasingly "serious" code, I'll give some notes about coding conventions below. No actual complaints, just small tweaks that'll make it fit in more smoothly with the server-side codebase.
.
Make it a habit to run "make lint" before submitting. It would have told you that this line in etc/maas/
power_params = {"power_address": ipaddress, "power_pass": password, "power_user": user, "power_driver": version}
It's line 17 in the diff. By MAAS standards we would format that as:
power_params = {
}
.
By the way, it's always good to explain what a function is for. Stating a function's purpose up front can make maintenance a lot easier in the future, when people who are less familiar with the code need to work on it. Even if it all feels obvious to you, chances are that when you stop and ask, you can think of things that a new maintainer will want to know right from the start.
Have a look at the source files anywhere in src/ for examples of docstrings.
.
A few small notes about get_ipmi_version, ll. 25—31 in the diff:
«
def get_ipmi_version():
(status, output) = commands.
show_re = re.compile('(IPMI\ Version:) (\d\.\d)')
res = show_re.
if res is None:
return
return res.group(2)
»
It's not important, but just in case it's useful to you: that first assignment (status, output) doesn't need the parentheses. We normally leave them out. Of course if you like them for clarity, I'm not complaining!
Then, at the end of the function, "return" without a value will return None — but making use of that makes it look as if maybe at some point there was confusion about whether the function returns a value. If the function returns a value, better "return None" explicitly!
Also nice to know is that Python has a specific pattern for cases where you return different things based on a condition, but there's no real work to do depending on the condition. Instead of doing the "if foo: return x; return y" that's normal in most languages, we like to do:
if foo:
return x
else:
return y
Putting both returns at the same indentation level makes it instantly clear that these are two alternative exits to the function. Of course if you have lots of "early return" clauses dotted around the function anyway, you may still want to keep the style you used here.
.
The same thing goes for setting IPMI_VERSION. Instead of making a choice and then changing it, it's clearer to put the choice up front:
if get_ipmi_version() == "2.0":
else:
This also makes it clear what to do if there's ever a LAN_3_0 or whatever.
.
I'm not entirely without selfish interest here... At some point we're going to move to Python 3. The more consistent our coding habits are, the easier that will be for us. :-)
Jeroen