Code review comment for lp:~jimbaker/pyjuju/expose-provider-ec2

Looks good, +1 with a few extra comments:

[13]

+ raise ProviderInteractionError(
+ "Got EC2 error (id %r: %s) when looking up machine" % (
+ provider_id, ex))

Please reword it like this:

    "EC2 error when looking up instance %s: %s" % (provider_id, e)

[14]

+ raise ProviderInteractionError(
+ "Machine (id: %r) was not found" % provider_id)

Please reword as:

"EC2 instance %s was not found"

[15]

+ except EC2Error:
+ log.debug("Ignoring machine group %s that just disappeared",
+ ensemble_machine_group)
+ pass # Security group for machine is no longer around, ignore

The pass + comment may be dropped as it's redundant with the previous line.

[16]

+# TODO These security group functions do not handle the eventual
+# consistency seen with EC2. This is likely not the right place to
+# address this issue. The provisioning agent is more likely the place
+# to do this.

This would mean we're introducing eventual consistency within our API,
which sounds really bad to handle. The eventual consistency aspect
should be handled as closely to the EC2 API as possible, rather than
across all of the library.

Please adapt the comment to reflect this.

[17]

+ "Got EC2 error (id %r: %s) when attempting to open %s/%s" % (
+ machine.instance_id, ex, port, protocol))

"EC2 error when attempting to open %s/%s on machine %s: %s"

[18]

Similar as the above for the other messages.

[19]

+ for ip_permission in security_groups[0].allowed_ips:
+ if ip_permission.cidr_ip != "0.0.0.0/0":

Is it possible for this list to be empty? In which circumstances?
Do we have to handle it to avoid breaking the agent entirely?

[20]

+ if from_port == to_port:
+ # Ignore ranges of ports, since they are set outside of
+ # Ensemble (at this time at least)

This comment seems reversed. This branch of logic is the one that
does _not_ ignore. It should probably be reworded in the positive
side to be more clear ("Only take ...").

[21]

It would be awesome to take the chance we have Mark and Clint here
and take them through a whole run of that branch before merging,
both to share the knowledge and to help ensuring the latest changes
work fine.