Code review comment for lp:~jimbaker/pyjuju/env-origin

[6]

+ PPA (use a `ppa:` prefix, eg `ppa:juju/pkgs`). Otherwise, the
+ utilized source will be based on the machine running the
+ bootstrap command (distribution, PPA, or branch).

s/eg/e.g./

In the following sentence, please use this wording:

  Otherwise, juju will attempt to detect the correct origin based on
  its run location and the installed juju package.

Also this should be fixed according to the rest of this review.

[7]

+ def _parse_policy(self, output):
(...)
+ def get_default_origin(self):
(...)

Neither of these methods seem to need any information from "self",
which means they are actually independent functions rather than
methods.

[8]

+ if k != "juju":
+ # Sanity check: no record at all, the only possibility is that
+ # this being run as a branch
+ return "lp:juju"
+ for k, v in parse:
+ if k == "Installed" and v == "(none)":
+ return _PPA
+ if k == "Version table":
+ break
+ for _, version_info in parse:
+ if "ppa.launchpad.net" in version_info:
+ return _PPA

As we talked yesterday online, all of this logic is idempotent, and should
be part of a single function, called parse_juju_origin(data) or similar,
and not part of the same function that interacts with the system (calling
"apt-cache policy juju").

[9]

+cat <<EOF
+juju:
+ Installed: (none)
+ Candidate: good-magic-1.0
+ Version table:
+ *** good-magic-1.0
+ 500 http://us.archive.ubuntu.com/ubuntu/ natty/main amd64 Packages
+ 100 /var/lib/dpkg/status
+EOF

This is testing the idempotent parse_juju_origin(data) function. Put the
data inside the test file, and test the function trivially:

    def test_foo(self):
        origin, source = parse_juju_policy(data)
 self.assertEquals(origin, "whatever")
 self.assertEquals(source, "whatever else")

It's really that simple! :-)

Kill most of these apt-cache dummies, please. You only need one test
of these, to ensure that the system interaction is really working as
it should. All the variations should be tested in the above fashion.

[10]

+ elif origin.startswith("ppa:"):
+ cloud_init.set_juju_source(ppa=True)

This is bogus. This should be `if origin == "ppa":`, otherwise you're
promising something and delivering something else. Please fix the
branch and documentation.

[11]

+class OriginPolicyTestMixin(object):
+
+ def setup_origin_policy(self, name, is_branch=False):
(...)
+class EC2MachineLaunchTest(EC2TestMixin, EC2MachineLaunchMixin,
+ OriginPolicyTestMixin, TestCase):

Ouch!

Please drop the mixin.. this is a single simple function,
so let's handle it as such.

[12]

--- juju/providers/ec2/tests/data/launch_cloud_init_branch 1970-01-01 00:00:00 +0000

The number of these files and the way we copy them blindly on every
minor detail we want to test is pretty sad. That's fine for this
branch, but it'd be awesome to figure a better way to test this in
the future.