Merge lp://staging/~jimbaker/pyjuju/env-origin into lp://staging/pyjuju

[1]

The documentation must be changed accordingly.

[2]

Please drop the version support. Without pinning, this doesn't guarantee that
the version will stay as suggested, and we shouldn't be looking into pinning
right now.

[3]

238 + # XXX needed for python-txzookeeper dependency
239 return ["ppa:juju/pkgs"]

That's a *major* one. Why? Please synchronize with SpamapS and ensure that's not
needed anymore.

[4]

213 + # Classify origins so the proper install script is later selected
214 + if origin is None:
215 + self._origin_type = _DISTRO
216 + elif (origin.startswith("deb ") or \
217 + origin.startswith("ppa:") or \
218 + origin.startswith("http:") or \
219 + origin.startswith("https:")):
220 + self._origin_type = _DEBIAN
221 + elif origin.startswith("lp:"):
222 + self._origin_type = _BRANCH

Why is this changing? The original version of this logic was significantly
simpler and cleaner, and seems to support all the cases we're trying to
support right now.

[5]

I'm _very_ concerned about your summary. The support for other package sources
besides branches is precisely what this branch is introducing, and you seem to
claim this wasn't tested at all.

Please do make sure this stuff is tested.

[0]

+ :type branch: str or None

two of these, should be "origin" and "version" instead of "branch"

[1]

+ version=str(config.get("juju-version")))

Won't that lead to version being "None" rather than None? If so, maybe add a test for that path.

Otherwise looks very nice, +1.

[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.

[13]

+ juju-origin:
+ The origin of juju deployed to machines within an environment
+ is set by this option. This option can be set to a bzr branch
+ pushed to Launchpad (use a `lp:` prefix, e.g., `lp:juju`); to
+ the PPA by using `ppa`; or to the juju distribution by using
+ `distro`. If this option cannot be parsed, the distribution is
+ used.

This sentence is not reading correctly in a few ways (e.g. the
juju _origin_ is not really deployed, and there's no such thing
as a juju distribution.. it's the Ubuntu distribution).

Please use this:

    juju-origin:
        Defines where juju should be obtained for installing in
 machines. Can be set to a "lp..." branch url, to "ppa" for
 getting packages from the official juju PPA, or to "distro"
 for using packages from the official Ubuntu repositories.

[14]

Jim, please read this excerpt from our conversation again, *carefully*:

<niemeyer> 1) Grab the installed version from the "Installed:" line
<niemeyer> 2) If this is "(none)" return "branch"
<niemeyer> 3) Otherwise, store the version in a variable, and keep parsing
<niemeyer> 4) Find the version table, and find the installed version
<niemeyer> 5) For each line in the given version, if it contains "ppa.launchpad.net/juju/pkgs", return "ppa"
<niemeyer> 6) If you reach the end of the installed version, return "distro"

Compare to what is in the branch right now.

[15]

+ if k == "Installed" and v == "(none)":
+ return "ppa", None

Compare with the summary above.

If there's no juju package installed in the system, it means the
user is not using the ppa.

[16]

+ for _, version_info in parse:
+ if "ppa.launchpad.net" in version_info:
+ return "ppa", None

Compare with the summary above.

Which version is this? The existence of _any_ version that comes from
a PPA in the apt cache is not an indication that the _installed_
version comes from a PPA. The indication that it comes from _a_ ppa
is not an indication that it comes from _our_ ppa either.

[17]

+ def test_distro_not_installed(self):
+ data = (
+ "juju:\n"
+ " Installed: (none)\n"
+ " Candidate: good-magic-1.0\n"

Much better, thank you.

I do not see my reply that I emailed to the lp gateway last night, so I'm reposting:

I have made the desired changes, merged trunk, and retested against EC2 for
the various combinations (other than distro, of course).

Please note that some of the branches that were recently merged into trunk
have broken it, as indicated here: http://wtf.labix.org/. I will certainly
remerge trunk as soon as that's fixed.

- Jim

On Wed, Sep 28, 2011 at 7:18 PM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
>
> [13]
>
> + juju-origin:
> + The origin of juju deployed to machines within an environment
> + is set by this option. This option can be set to a bzr branch
> + pushed to Launchpad (use a `lp:` prefix, e.g., `lp:juju`); to
> + the PPA by using `ppa`; or to the juju distribution by using
> + `distro`. If this option cannot be parsed, the distribution is
> + used.
>
> This sentence is not reading correctly in a few ways (e.g. the
> juju _origin_ is not really deployed, and there's no such thing
> as a juju distribution.. it's the Ubuntu distribution).
>
> Please use this:
>
> juju-origin:
> Defines where juju should be obtained for installing in
> machines. Can be set to a "lp..." branch url, to "ppa" for
> getting packages from the official juju PPA, or to "distro"
> for using packages from the official Ubuntu repositories.
>
>
Used this text as suggested.

>
> [14]
>
> Jim, please read this excerpt from our conversation again, *carefully*:
>
> <niemeyer> 1) Grab the installed version from the "Installed:" line
> <niemeyer> 2) If this is "(none)" return "branch"
> <niemeyer> 3) Otherwise, store the version in a variable, and keep parsing
> <niemeyer> 4) Find the version table, and find the installed version
> <niemeyer> 5) For each line in the given version, if it contains "
> ppa.launchpad.net/juju/pkgs", return "ppa"
> <niemeyer> 6) If you reach the end of the installed version, return
> "distro"
>
> Compare to what is in the branch right now.
>
>
> [15]
>
> + if k == "Installed" and v == "(none)":
> + return "ppa", None
>
> Compare with the summary above.
>
> If there's no juju package installed in the system, it means the
> user is not using the ppa.
>

Changed the logic above and the impacted test

>
>
> [16]
>
> + for _, version_info in parse:
> + if "ppa.launchpad.net" in version_info:
> + return "ppa", None
>
> Compare with the summary above.
>
> Which version is this? The existence of _any_ version that comes from
> a PPA in the apt cache is not an indication that the _installed_
> version comes from a PPA. The indication that it comes from _a_ ppa
> is not an indication that it comes from _our_ ppa either.
>
>
Got it. It's now using the precise test as you specified earlier. Sorry
about that.

>
> [17]
>
> + def test_distro_not_installed(self):
> + data = (
> + "juju:\n"
> + " Installed: (none)\n"
> + " Candidate: good-magic-1.0\n"
>
> Much better, thank you.
>
>
Thanks!

<jimbaker> niemeyer, hi
<niemeyer> jimbaker: Hi
<niemeyer> jimbaker: Please re-read points 3, 4 and 5
<niemeyer> jimbaker: and compare to the implementation
<jimbaker> niemeyer, ok
<niemeyer> jimbaker: Is there anything missing there?
<niemeyer> jimbaker: Maybe I'm just missing.. where's that: 3) Otherwise, store the version in a variable, and keep parsing
<jimbaker> niemeyer, #3 - python-txzookeeper dependency is no longer relevant; #4, it uses the original implementation; #5, there is no attempt to test deb repositories, since they are no longer supported
<niemeyer> jimbaker: 4) Find the version table, and find the installed version
<niemeyer> ?
* ejat has quit (Remote host closed the connection)
<jimbaker> niemeyer, ok, i'm referring to the review points. you are referring to a list from irc
<niemeyer> jimbaker: yeah
<jimbaker> looking at that now
<jimbaker> niemeyer, ok, i believe what you are saying here is that 6) refers to the installed version, which was stored earlier in 3), is that correct?
<niemeyer> 1) Grab the installed version from the "Installed:" line
<niemeyer> 2) If this is "(none)" return "branch"
<niemeyer> 3) Otherwise, store the version in a variable, and keep parsing
<niemeyer> 4) Find the version table, and find the installed version
<niemeyer> jimbaker: Is there any ambiguity here?
<jimbaker> niemeyer, correct, there is no ambiguity. the version table has more information than i have assumed, and this parse needs to take it in account

> <niemeyer> 1) Grab the installed version from the "Installed:" line
> <niemeyer> 2) If this is "(none)" return "branch"
> <niemeyer> 3) Otherwise, store the version in a variable, and keep parsing
> <niemeyer> 4) Find the version table, and find the installed version
> <niemeyer> jimbaker: Is there any ambiguity here?
> <jimbaker> niemeyer, correct, there is no ambiguity. the version table has
> more information than i have assumed, and this parse needs to take it in
> account

The algorithm in parse_juju_origin, and its helper function, has been reworked as described.

[18]

345 + " *** 0.5+bzr366-1juju1~natty1 0\n"
346 + " 500 http://ppa.launchpad.net/juju/pkgs/ubuntu/ "
347 + "natty/main amd64 Packages\n"
348 + " *** 0.5+bzr356-1juju1~natty1 0\n"
349 + " 500 http://ppa.launchpad.net/juju/pkgs/ubuntu/ "

Your multiple versions test has the PPA both in the installed and in the
non-installed version, so it's not really verifying the bug that you have
just fixed.

Change the URL in the non-installed version in this test so it doesn't come
from the PPA so that we ensure that it's really testing the installed version.
Also add a non-installed version that comes from the ppa in the distro test,
to ensure it's not being considered.

Please ensure tests pass with this and that a real interaction passes as well
before merging.

I'm taking this over after a conversation on IRC.

Pushed the following changes onto lp:~niemeyer/juju/env-origin, reviewed by Kapil:

- Implementation redone completely.
- Do not crash on missing apt-cache.
- Exported and tested get_default_origin.
- Tests tweaked to explore edge cases.