Code review comment for lp:~fwereade/pyjuju/cloud-init-class-used

This looks _very_ nice. Thanks William!

+1, with the following sorted:

[1]

+ self._zookeeper_names = [m.private_dns_name for m in machines]
(...)
+ def _zookeeper_hosts(self):
+ all_names = self._zookeeper_names[:]

The branch has two different names for the same data in different
formats. The better approach is the opposite: the same name with
different actions that give a hint about the undelying operation.

I suggest using "zookeeper_hosts" for both, and naming the last
function as join_zookeeper_hosts or something similar.

[2]

+ def _validate(self):
+ missing = []

Very nice logic there.

[3]

+ def _create_cloud_init(self, machine_id, zookeepers):
+ config = self._provider.config
+ cloud_init = CloudInit()
+ cloud_init.add_ssh_key(get_user_authorized_keys(config))
+ cloud_init.set_machine_id(machine_id)
+ cloud_init.set_zookeeper_machines(zookeepers)
+ if config.get("ensemble-branch"):
+ cloud_init.set_ensemble_source(config["ensemble-branch"])
+ if self._master:
+ cloud_init.enable_bootstrap()
+ cloud_init.set_zookeeper_secret(config["admin-secret"])
+ return cloud_init

Beautiful! :-)

[4]

+ instance_type = self._provider.config.get(
+ "default-instance-type", "m1.small")
+ image_id = yield get_image_id(self._provider.config, self._constraints)
+ security_groups = yield self._ensure_groups(machine_id)
+
+ instances = yield self._provider.ec2.run_instances(
+ image_id=image_id,
+ instance_type=instance_type,
+ min_count=1,
+ max_count=1,
+ security_groups=security_groups,
+ user_data=user_data)

Very clean as well, thanks!

[5]

+def get_image_id(config, constraints):
(...)
+_KWARG_NAMES = {
+ "ubuntu_release": "ubuntu_release_name",
+ "architecture": "arch",
+ "daily": "daily",
+ "persistent_storage": "ebs"}
(...)
+def _convert_constraints(constraints):
+ kwargs = {}
+ for (constraint_name, kwarg_name) in _KWARG_NAMES.items():
+ if constraint_name in constraints:
+ kwargs[kwarg_name] = constraints[constraint_name]
+ return kwargs
(...)
+ kwargs = _convert_constraints(constraints)

Why are we doing this? These names in the right-hand-side of the
_KWARG_NAMES are keyword arguments of get_current_ami, which is a function
on the same file (!). The left-hand-side ones are not even used in
any sensible way in our current implementation.

Let's just unify the naming and avoid any conversions entirely. Feel
free to pick sensible ones.

[6]

Again, this looks very nice. Please just make sure you merge trunk into
this, and do one final real-world test on EC2 before committing back
into trunk.