Code review comment for lp:~gary/charms/precise/juju-gui/update-for-jujucharms
Revision history for this message
| Gary Poster (gary) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
*** Submitted:
Fix charm for jujucharms.com use
- Merge work from mews to support exec.d directory and relative file
paths. Add tests.
- Merge work from benji to support charmworld option needed by upcoming
gui changes. Make a small fix.
- Fix deploy tests.
Tests are still fragile, but I got them all to pass with the current
branch at one time, and verified manually that the various steps still
work.
R=rharding, benji
CC=
https:/
https:/
File hooks/install (right):
https:/
hooks/install:46: for module in os.listdir(
On 2013/04/27 20:29:30, benji wrote:
> Usually ".d" files are to be run in lexicographic order, so throwing a
sort()
> around the listdir would be good.
Good idea in the abstract, and I changed it.
FWIW, this code is provided by IS for their use, and I don't like the
pattern and at least mew has agreed with me, so I'm not super excited
about improving it. Your ideas were good, though, so I grudgingly did
them anyway. :-)
https:/
hooks/install:49: except OSError:
On 2013/04/27 20:29:30, benji wrote:
> It would be safer if we knew which OSErrors in particular we should
ignore. It
> would also be nice to know why we are ignoring them (i.e., a comment).
OK, good point...I did it, because I agree with you, though I had to
guess at their intent. Hopefully I got it right. :-/
https:/
File hooks/utils.py (right):
https:/
hooks/utils.py:195: """Log when a hook starts and stops its execution.
On 2013/04/27 20:29:30, benji wrote:
> Yay! I hate the "an" before non-silent "h" thing.
> I know, I know; I'm weird.
heh
https:/
hooks/utils.py:228: if source[0] != '/':
On 2013/04/27 20:29:30, benji wrote:
> I don't really prefer it, but just in case you do:
> if not source.
Yeah, I do, thanks. Changed.
https:/
File hooks/web-
https:/
hooks/web-
On 2013/04/27 20:29:30, benji wrote:
> Unnecessary newline.
I was doing as the Romans do, but eh, I don't think the Romans will
notice. Deleted.
https:/
hooks/web-
On 2013/04/27 20:29:30, benji wrote:
> You can use a one-liner here.
Again, I was following the local patterns in the charm, but eh, I agree.
Changed.
https:/
hooks/web-
On 2013/04/27 20:29:30, benji wrote:
> I'm pretty sure lines 16-18 are equivalent to
> hostname = socket.getfqdn()
I stared at this for a bit. I think you are probably right. What I had
here is what the haproxy or apache2 charms are doing, I forget. Under
the time pressure, it is arguably better to go with the proven code.
However, decided to go with the simpler version you proposed. If it
doesn't work, we'll find out why and add a comment about it, unlike the
other charm I saw.
https:/
File tests/test_utils.py (right):
https:/
tests/test_
parse_source('url:http://
On 2013/04/27 20:29:30, benji wrote:
> I think this line is over 79 characters.
Done.
https:/
tests/test_
parse_source(
On 2013/04/27 20:29:30, benji wrote:
> Recent versions of unittest automatically pick the assert function
that matches
> the datatype so you can just do assertEqual and it will call
assertTupleEqual
> for you.
That sounds cool. For now, I'm sticking with these here local Romans
and their assertTupleEqual calls, as found in all proceeding tests in
this case. We can switch out later in another branch, as desired.
https:/