juju/juju — pull request #1218 by fwereade (2014-11-24 22:42:53+00:00)
Title: Uniter: extract operations

So, this is an appalling branch, but it's not *quite* as awful as it looks, because:

* more than half of the added lines of code are *just* new tests -- and I've only written half of them
* more than half what remains is just moved code, although it may be tricky to directly compare them
* it's otherwise all sufficiently related that the components are annoyingly hard to judge in isolation -- it took a depressingly long time to come up with separations of concerns that made things arguably better overall, rather than wallpaper-bubbling the mess back and forth
* the pre-existing tests have changed in exactly 3 places; 2 of those are checking for better error messages, and one of them is handling a notification callback that should always have been triggering but wasn't
* and it's got a bunch of trivial drivebys too, which bump up the line count without affecting complexity

The best thing is that lots of Uniter code was extracted into the operation package, and split up into reasonably independent types:

* operation.Operation expresses the common execution paths of install/upgrade/run-hook/run-action/run-commands
* operation.Factory creates those various operations
* operation.Executor is responsible for recording state changes as the operations progress
* operation.Callbacks is, uh, all the yucky bits that didn't quite fit

So, most of the code removed from uniter.go moved to either op_callbacks.go or the operation package. The changes to modes.go were comparatively trivial:

* the beginning of ModeContinue is now covered by operation.NewExecutor, which loads initial local state
* u.operationState is now accessed via u.operationState()
* u.currentHookName was used only in ModeHookError, and was inlined there
* thanks to reasonably liberal error tracing/annotation, we now pay a bit more attention to error causes than errors

As for the rest:

* the context package got some trivial interface changes to assist in testing operation
* operation.StateFile has a cleaner interface
* action executions are no longer recorded as hook executions -- they act really quite differently, and should be treated as such

...and there are many things I did *not* do in this branch, despite the temptation, including:

* recovering properly from agent bounces mid-action
* handling juju-reboot in actions
* allowing actions during hook error states

...but I think I've laid the groundwork for fixing those, and more. Anyway, it's not *quite* done, because I have a whole pile of tests to finish, but it's definitely ready for review -- and I'm more than a little tempted to say that with the integration tests still running, and 1350-odd lines of new tests for the refactored operation functionality, I should go ahead and land this ASAP and defer detailed testing of actions and commands to a followup.