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

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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

review: Needs Fixing

« Back to merge proposal