+ 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.
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").
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.
This is bogus. This should be `if origin == "ppa":`, otherwise you're
promising something and delivering something else. Please fix the
branch and documentation.
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.
[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): origin( self):
(...)
+ def get_default_
(...)
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 origin( data) or similar,
be part of a single function, called parse_juju_
and not part of the same function that interacts with the system (calling
"apt-cache policy juju").
[9]
+cat <<EOF us.archive. ubuntu. com/ubuntu/ natty/main amd64 Packages dpkg/status
+juju:
+ Installed: (none)
+ Candidate: good-magic-1.0
+ Version table:
+ *** good-magic-1.0
+ 500 http://
+ 100 /var/lib/
+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): policy( data) als(origin, "whatever") als(source, "whatever else")
origin, source = parse_juju_
self.assertEqu
self.assertEqu
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:") : set_juju_ source( ppa=True)
+ cloud_init.
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 OriginPolicyTes tMixin( object) : policy( self, name, is_branch=False): hTest(EC2TestMi xin, EC2MachineLaunc hMixin, tMixin, TestCase):
+
+ def setup_origin_
(...)
+class EC2MachineLaunc
+ OriginPolicyTes
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.