Code review comment for lp:~fwereade/pyjuju/cobbler-connect-production

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.