yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

I was just reviewing where we were and had two comments.  No change to
my previous review/blessing. :-)

Gary


https://codereview.appspot.com/6977043/diff/8001/hooks/config-changed
File hooks/config-changed (right):

https://codereview.appspot.com/6977043/diff/8001/hooks/config-changed#newcode50
hooks/config-changed:50: # Restarting of the gui and api services is
handled below.
On 2012/12/20 13:09:14, teknico wrote:
> The two lines above could be replaced by:
> # Fetch new sources if needed, and restart GUI and API services
I think the point of the comment is to reassure readers that these
things will be restarted, just not in this code block. "below" means
"not in this code block."

The comment worked for me.  Would this have been clearer for you? "# The
juju_gui_source_changed and juju_api_branch_changed variables control
whether we restart the GUI and the API, respectively, at the end of the
function."  Or something.  Other ideas?

https://codereview.appspot.com/6977043/diff/8001/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/6977043/diff/8001/hooks/utils.py#newcode310
hooks/utils.py:310: """Set up nginx."""
On 2012/12/20 13:09:14, teknico wrote:
> Well, nginx has nothing to do with the API environment, has it? It
looks like
> its setup code should be part of setup_gui, and setup_api should be
empty, for
> now. Shortly we will copy the nginx SSL certificate and private key to
the API
> environment, that's one thing that should go in setup_api.

Good point.

https://codereview.appspot.com/6977043/

-- 
https://code.launchpad.net/~frankban/charms/precise/juju-gui/bug-1088618-serve-releases/+merge/140745
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/charms/precise/juju-gui/bug-1088618-serve-releases into lp:~juju-gui/charms/precise/juju-gui/trunk.