Merge lp://staging/~fwereade/pyjuju/cobbler-connect-production into lp://staging/pyjuju

Looks nice. Just a few minors, and +1.

[1]

+ @property
+ def _acquired_class(self):
+ return self._config["acquired-mgmt-class"]
+
+ @property
+ def _available_class(self):
+ return self._config["available-mgmt-class"]
+
+ @property
+ def _credentials(self):
+ return (self._config["orchestra-user"],
+ self._config["orchestra-pass"])

Not a big deal, but just as some feedback, this feels like
adding more complexity than necessary for the problem at
hand. Compare with this version:

 self._acquired_class = self._config["acquired-mgmt-class"]
 self._available_class = self._config["available-mgmt-class"]
 self._user = self._config["orchestra-user"]
 self._pass = self._config["orchestra-pass"]

[2]

+ if "invalid token" not in failure.getErrorMessage():
+ return failure
+ login = self._login()
+ login.addCallback(lambda unused: call())

Looks nice.

[3]

+ failure.trap(Fault)
+ if "invalid token" not in failure.getErrorMessage():
+ return failure

None of that logic is covered by tests, though.

[4]

+ def check(actual):
+ if actual != expect:
+ raise ProviderError(
+ "Bad result from call to %s with %s (got %r, expected %r)"
+ % (name, args, actual, expect))
+ return actual

Also untouched by tests.

[5]

+ self.mock_proxy()
+ self.mock_get_systems()
+ self.mock_acquire_system()
+ self.mock_set_ks_meta()
+ self.mock_start_system()

I guess there's no better way to test that interaction with the external system,
but I have to say I'm a bit sad about the amount of mocking here.

Quick responses:

[1] I guess something in my past has caused me to become fanatical about having a single point of truth for every possible piece of data in the system. What if someone changes config underneath me!? (Yeah, I'll change it.)

[2] Cheers :-).

[3] Yes it is: test_bootstrap.py, lines 43-48. I'm open to the argument that testing a single call that happens to follow this path is a little weak, but I'm reluctant to add further complexity to the already excessive mocking here. See below...

[4] Thanks, nice catch. I have a comment I'll come back to in the light of...

[5] It'll only get worse in future branches :-(. On the bright side, I think this is a good opportunity for a philosophical ramble :-p.

I'd categorise your favoured testing style as "integration" rather than "unit", which I've tended to neglect in the past, but I'm reevaluating that: factoring the generic code out of providers.ec2 was *much* easier than I expected, largely thanks to this testing style.

My own approach has historically tended to favour ludicrously tight unit testing, in which *everything* that isn't part of the SUT is mocked out; the obvious drawback is that the burden of significant refactorings can be much heavier (and it's very possible to miss tests which expect another object to have an outdated interface), but I've generally had good experiences when my unit tests are backed up by relatively few high-level functional tests: 9 times out of 10, I've found, an end-to-end test of the happy path will flush out the inconsistencies.

In the end, I think, it's a choice between testing (1) the ultimate external effects of an interaction with the SUT or (2) the precise interactions with the SUT's dependencies; which path is better in the long term depends on the interactions' relative susceptibility to change. I do feel that only (2) really qualifies as "unit" testing, but that's frankly irrelevant: reliable coverage matters a lot more than the names we happen to use for the techniques we employ.

I'm not advocating a change of approach here, but I would say that we're we're pretty close to the line: the current strategy means that a tweak to CobblerConnect's interaction with the server will lead to failures in an arbitrary number of tests, none of which are related to the cause of the failure (except coincidentally). We'll have to see how it shakes out here; I think the fact that the cobbler API is much chattier than EC2's may eventually justify a different approach locally, but it hasn't hurt me enough to seriously complain. Yet :-).

[3/4 redux] In [3], I'm comfortable testing only one interaction with the "invalid token" code, because I "know" everything goes through ._call(). In [4], there are 3 distinct operations that can hit ._check_call(), and under the current approach I can't see any option but to test both happy and sad paths each one individually... but this makes me uncomfortable, because a minor tweak to that code would entail fixing at least 6 tests. I'll fix it anyway; I just had an urge to register my misgivings ;-).

Er, I really did intend these to be quick responses. Thanks for the thorough review!

[4] Further thought: since all uses of _check_call() are actually in _set_on_system, I intend to write a relatively small number of tests that exercise that code, giving us a similar imperfect-but-wieldy level of testing to that in [3]. Tolerable?

[3]

Try taking the commented lines out and run test_bootstrap.

[5]

Not entirely. It is possible to do good unit testing without mocking, but I agree
that in some cases good unit testing moves toward the integration side of things,
and that sounds quite alright in most cases.

The test should be as unity as possible before it starts to get fake and invasive.
Mocking is silly in the sense that it forces you to repeat the same logic twice
(e.g. call foo() in the test, and call foo() in the code), giving a false sense
of confidence in the logic. The outcome is something that does not guarantee
correctness, because you were worried about "how-to" when you wrote the test rather
than the outcome, and that very fact binds the implementation to the test in a
tight way that makes it hard to refactor, as you noticed by counter example.

[3] Bah. Thank you, nice catch; on it.

[5] I think we agree on the problem, at least ;).

I can't speak at all to what this means in terms of cobbler/orchestra interaction but code style/syntax looks good.

[0] This bit appears unused and extraneous.

ensemble/lib/testing.py

74 + client_got = False
75 +

[1] Just a naming thing, but i'd change CobblerConnect to CobblerConnection

[2] random thoughts.

For launching machines is there anyway to distinguish size/hardware capabilities? I'm just thinking that in the future, we'll want to have some abstracted notion of machine size so we can utilize them for deploying services that need additional hardware capabilities available to a machine provider.

[3] Some pep8/syntax items that came up captured in diff to fix.
https://pastebin.canonical.com/50619/

[4] I'm curious what the ensemble late command stuff is.. it might be nice to have some documentation of the orchestration integration when all of this said and done.

> [1] Just a naming thing, but i'd change CobblerConnect to CobblerConnection

Hm, it's not really a *connection* as such, but I agree it's not a very helpful name; I think CobblerClient fits better for now.

> [2] random thoughts.
>
> For launching machines is there anyway to distinguish size/hardware
> capabilities? I'm just thinking that in the future, we'll want to have some
> abstracted notion of machine size so we can utilize them for deploying
> services that need additional hardware capabilities available to a machine
> provider.

Not yet; agree we'll need it at some stage.

> [3] Some pep8/syntax items that came up captured in diff to fix.
> https://pastebin.canonical.com/50619/

Thanks :).

> [4] I'm curious what the ensemble late command stuff is.. it might be nice to
> have some documentation of the orchestration integration when all of this said
> and done.

It's basically just writing the user-data/meta-data files for cloud-init. I agree that we'll want documentation for the Orchestra provider, but there are still a fair number of unanswered questions, so I'm not sure it's quite the right time yet.