Merge lp:~jtv/maas/bug-987629 into lp:~maas-committers/maas/trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Jeroen T. Vermeulen | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 515 | ||||
Proposed branch: | lp:~jtv/maas/bug-987629 | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
71 lines (+26/-2) 4 files modified
src/maasserver/fixtures/dev_fixture.yaml (+1/-1) src/maasserver/forms.py (+9/-1) src/maasserver/models/__init__.py (+5/-0) src/maasserver/tests/test_views_nodes.py (+11/-0) |
||||
To merge this branch: | bzr merge lp:~jtv/maas/bug-987629 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+103416@code.launchpad.net |
Commit message
Allow admin to retry failed commissioning of a node.
Description of the change
There was nobody around to pre-imp this with, so I just went ahead and did the absolute minimum that bug 987629 required: if a node is in Failed Tests state, allow an admin to retry the commissioning process (e.g. after swapping out bad hardware or remembering to plug the machine into the network).
In order of diff:
In the sampledata, node “moon” was in state Failed Tests but had an owner. This is unrealistic. A node in this state does not have an owner. (And thus, it can be deleted which makes a lot of sense!)
In NODE_ACTIONS, I added the retry button (as well as a missing comma at the end of the last entry in the preceding dict). All it does is send the node straight back into commissioning. There's no need to send it back into Declared state first, which would only have added unnecessary code paths.
Why is there no need? As you see in NODE_TRANSITIONS, all I had to do was allow the node transition from Failed Tests back to Commissioning. In fact we didn't have any transitions from that state defined, so I defined what seemed sensible — a superset of what we actually use.
I did *not* add a “Retire” button for failed nodes, although I did allow the transition. The reason is that we don't seem to have that option anywhere else in the UI, and we might as well add that, wherever appropriate, in one fell swoop. Including it in this branch would water down its purpose, and doing it only for failed nodes would give a false impression that the job was done.
Oh, and finally of course there's a test. There's no negative test to prove that non-admins can't do this; that's purely data-driven, so essentially a configuration matter. There's little point in a test duplicating that information.
I toyed with making the Retry button require only View permission on the node, since after all at this stage there's no longer any objection to wiping its disks. The only scenario I could think of where it mattered was if an admin owned a node that failed commissioning, and then lost admin privileges but remained a valid user of the MAAS; they could then retain responsibility for dealing with the broken node. —And then I realized that a Failed node does not have an owner. Never mind; Admin is nice and simple.
Jeroen
> In the sampledata, node “moon” was in state Failed Tests but had an
> owner. This is unrealistic. A node in this state does not have an
> owner. (And thus, it can be deleted which makes a lot of sense!)
I thought that the owner is set when going into commissioning... is it
unset when test results are posted?