Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(687)

Issue 8640044: Apache support

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by bcsaller
Modified:
10 years, 11 months ago
Reviewers:
benjamin.saller, teknico, mp+160413, benji
Visibility:
Public.

Description

Apache support Change charm to use apache2 rather than nginx. Some additional cleanups around testing as well. https://code.launchpad.net/~bcsaller/charms/precise/juju-gui/apache-version/+merge/160413 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+362 lines, -130 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
A config/apache-ports.template View 1 chunk +2 lines, -0 lines 0 comments Download
A config/apache-site.template View 1 chunk +27 lines, -0 lines 6 comments Download
D config/nginx.conf View 1 chunk +0 lines, -24 lines 0 comments Download
D config/nginx-site.template View 1 chunk +0 lines, -35 lines 0 comments Download
M hooks/backend.py View 7 chunks +9 lines, -9 lines 2 comments Download
M hooks/install View 2 chunks +3 lines, -3 lines 4 comments Download
M hooks/utils.py View 10 chunks +33 lines, -26 lines 0 comments Download
A icon.svg View 1 chunk +257 lines, -0 lines 0 comments Download
M metadata.yaml View 1 chunk +1 line, -0 lines 0 comments Download
M revision View 1 chunk +1 line, -1 line 0 comments Download
M tests/test_backends.py View 2 chunks +3 lines, -3 lines 0 comments Download
M tests/test_utils.py View 6 chunks +24 lines, -29 lines 5 comments Download

Messages

Total messages: 4
bcsaller
Please take a look.
10 years, 11 months ago (2013-04-23 15:37:43 UTC) #1
benji
This branch looks great. I had a couple of small suggestions on the apache config ...
10 years, 11 months ago (2013-04-23 16:41:09 UTC) #2
teknico
LGTM, QA'd deployment, see comments below. https://codereview.appspot.com/8640044/diff/1/hooks/backend.py File hooks/backend.py (right): https://codereview.appspot.com/8640044/diff/1/hooks/backend.py#newcode184 hooks/backend.py:184: backends.append(UpstartMixin) So UpstartMixin ...
10 years, 11 months ago (2013-04-23 16:41:50 UTC) #3
benjamin.saller
10 years, 11 months ago (2013-04-23 16:55:41 UTC) #4
Thanks for the review, I'll submit with the changes.

https://codereview.appspot.com/8640044/diff/1/config/apache-site.template
File config/apache-site.template (right):

https://codereview.appspot.com/8640044/diff/1/config/apache-site.template#new...
config/apache-site.template:3: 
On 2013/04/23 16:41:09, benji wrote:
> We could reduce the version and module information exposed with
"ServerSignature
> Off" and "ServerTokens Prod".

Done.

https://codereview.appspot.com/8640044/diff/1/config/apache-site.template#new...
config/apache-site.template:7: AllowOverride None
On 2013/04/23 16:41:09, benji wrote:
> Since we're not using old-school CGI scripts we could disable them: Options
> -ExecCGI.  I don't honestly know if that buys us anything, so I'm just
throwing
> it our there.

Done.

https://codereview.appspot.com/8640044/diff/1/config/apache-site.template#new...
config/apache-site.template:10: Options Indexes FollowSymLinks MultiViews
On 2013/04/23 16:41:09, benji wrote:
> Do we want "Indexes"?  I don't think there is a use case for being able to
view
> directory listings.

This is pretty much taken from the default site template. I'll take out
multiviews and added in the -execcgi flag

https://codereview.appspot.com/8640044/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/8640044/diff/1/hooks/backend.py#newcode184
hooks/backend.py:184: backends.append(UpstartMixin)
On 2013/04/23 16:41:50, teknico wrote:
> So UpstartMixin needs to be last? Why?
The order things start in. The last composed backend should (re)start the
services with the current config. Because start() is used in both hooks/start
and hooks/config-changed (where we rewrite config files) this must come last.

In the future utils like start_gui should be split into the various mixins but I
can't force another round of testing under this timeframe.

https://codereview.appspot.com/8640044/diff/1/hooks/install
File hooks/install (right):

https://codereview.appspot.com/8640044/diff/1/hooks/install#newcode5
hooks/install:5: import json
On 2013/04/23 16:41:50, teknico wrote:
> Maybe move this line before the previous one.

Done.

https://codereview.appspot.com/8640044/diff/1/hooks/install#newcode16
hooks/install:16: return json.loads(output)
On 2013/04/23 16:41:50, teknico wrote:
> Thanks for fixing this.

Sorry for the overlap on that one. Came up in testing as a blocker. Not sure why
it worked before.

https://codereview.appspot.com/8640044/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/8640044/diff/1/tests/test_utils.py#newcode495
tests/test_utils.py:495: self.files[os.path.basename(dest)] = open(target.name,
'r').read()
I like that style as well. I was following the existing module convention but
I've changed it now.

On 2013/04/23 16:41:09, benji wrote:
> If this were
> 
> with open(target.name, 'r') as f:
>     self.files[os.path.basename(dest)] = f.read()
> 
> Then the file would be closed a bit earlier.  It's not really bad as-is, but I
> support the habit of closing files as quickly as possible.

https://codereview.appspot.com/8640044/diff/1/tests/test_utils.py#newcode510
tests/test_utils.py:510: self.destination_file = tempfile.NamedTemporaryFile()
This was also dead code with the self.files change so removed.

https://codereview.appspot.com/8640044/diff/1/tests/test_utils.py#newcode519
tests/test_utils.py:519: #self.files = {}
On 2013/04/23 16:41:09, benji wrote:
> This commented-out code can be removed.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b