Merge lp:~benji/charms/precise/juju-gui/fix-cache-headers into lp:~charmers/charms/precise/juju-gui/trunk

Proposed by Benji York
Status: Merged
Merge reported by: Marco Ceppi
Merged at revision: not available
Proposed branch: lp:~benji/charms/precise/juju-gui/fix-cache-headers
Merge into: lp:~charmers/charms/precise/juju-gui/trunk
Prerequisite: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 199 lines (+64/-23) (has conflicts)
6 files modified
HACKING.md (+5/-2)
Makefile (+8/-2)
config/apache-site.template (+3/-0)
hooks/utils.py (+1/-0)
revision (+4/-0)
tests/20-functional.test (+43/-19)
Text conflict in revision
To merge this branch: bzr merge lp:~benji/charms/precise/juju-gui/fix-cache-headers
Reviewer Review Type Date Requested Status
Francesco Banconi (community) Approve
Gary Poster (community) Approve
charmers Pending
Review via email: mp+177958@code.launchpad.net

Commit message

Add good cache headers to the GUI when served by Apache.

Description of the change

Add good cache headers to the GUI when served by Apache.

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

Great, thanks Benji! Thanks also for writing the test: that is very cool.

Nice flyby change to Makefile. Nice approach to juju_deploy.

LGTM

Gary

review: Approve
Revision history for this message
Gary Poster (gary) wrote :

Please land this change in ~juju-gui-charmers *and* ~juju-gui. I would suggest landing it in ~juju-gui-charmers and merging that branch back to ~juju-gui. Your MP is against ~juju-gui: please don't merge other ~juju-gui stuff into ~juju-gui-charmers! Only your fix.

Beware that when you land this against ~juju-gui-charmers *you are making a release of the official charm*. Please qa your changes with that branch before landing.

Revision history for this message
Francesco Banconi (frankban) wrote :

This branch looks good Benji, thank you!
I am doing QA now.
Please take a look at the comment below.

113 + if is_legacy_juju:
114 + hostname = unit_info['public-address']
115 + self.addCleanup(
116 + self.stop_services,
117 + hostname, ['haproxy', 'apache2', 'juju-api-agent'])

It's nice to have this logic abstracted in its own method.
However, while this is correct for test_api_agent, test_cache_headers
and test_branch_source, for test_staging we need to replace
'juju-api-agent' with 'juju-api-improv' in the list of services:

146 - hostname, ['haproxy', 'apache2', 'juju-api-improv'])

Revision history for this message
Francesco Banconi (frankban) wrote :

QA: everything seems to work well.
Started with 0.8.0, played with the GUI and charm browser,
switched to 0.8.1, the GUI looks good.
Thank you!

review: Approve
Revision history for this message
Benji York (benji) wrote :

> It's nice to have this logic abstracted in its own method.
> However, while this is correct for test_api_agent, test_cache_headers
> and test_branch_source, for test_staging we need to replace
> 'juju-api-agent' with 'juju-api-improv' in the list of services:

Good catch. I guess this is one reason I like to remove repetition like
this, it can trick you by not really being repetitious. :)

I've tweaked the juju_deploy method to fix this problem.

Thanks for the review.

72. By Benji York

fix service cleanup

Revision history for this message
Gary Poster (gary) wrote :

Sorry, this branch is against the wrong target!

Benji, the official branch is

lp:~juju-gui-charmers/charms/precise/juju-gui/trunk

Please land on that one, and then merge to lp:~juju-gui/charms/precise/juju-gui/trunk

The target of this MP (lp:~charmers/charms/precise/juju-gui/trunk) is not used for anything much. It is a decoy.

Revision history for this message
Marco Ceppi (marcoceppi) wrote :

This appears to have been merged in to the proper charmstore version of the charm

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'HACKING.md'
--- HACKING.md 2013-07-17 21:30:28 +0000
+++ HACKING.md 2013-08-01 13:01:28 +0000
@@ -39,9 +39,12 @@
39it locally, e.g.:39it locally, e.g.:
4040
41 bzr checkout --lightweight lp:juju-plugins41 bzr checkout --lightweight lp:juju-plugins
42 ln -s juju-plugins/plugins/juju_test.py juju-test
43 export PATH="$PATH":`pwd`
4244
43At this point, link "juju-plugins/plugins/juju_test.py" as "juju-test"45Alternatively you may check out lp:juju-plugins and link
44somewhere in your PATH, so that it is possible to execute "juju-test".46"juju-plugins/plugins/juju_test.py" as "juju-test" somewhere in your PATH, so
47that it is possible to execute "juju-test".
4548
46Before being able to run the suite, test requirements need to be installed49Before being able to run the suite, test requirements need to be installed
47running the command:50running the command:
4851
=== modified file 'Makefile'
--- Makefile 2013-08-01 13:01:27 +0000
+++ Makefile 2013-08-01 13:01:28 +0000
@@ -26,7 +26,12 @@
26 ./tests/10-unit.test26 ./tests/10-unit.test
27 ./tests/11-server.test27 ./tests/11-server.test
2828
29ftest: setup29ensure-juju-test:
30 @which juju-test > /dev/null \
31 || (echo 'The "juju-test" command is missing. See HACKING.md.' \
32 ; false)
33
34ftest: setup ensure-juju-test
30 $(JUJUTEST) 20-functional.test35 $(JUJUTEST) 20-functional.test
3136
32# This will be eventually removed when we have juju-test --clean-state.37# This will be eventually removed when we have juju-test --clean-state.
@@ -62,4 +67,5 @@
62 @echo ' service to be started. If JUJU_ENV is not passed, the charm will'67 @echo ' service to be started. If JUJU_ENV is not passed, the charm will'
63 @echo ' be deployed in the default Juju environment.'68 @echo ' be deployed in the default Juju environment.'
6469
65.PHONY: all clean deploy ftest help jujutest lint setup test unittest70.PHONY: all clean deploy ensure-juju-test ftest help jujutest lint setup test \
71 unittest
6672
=== modified file 'config/apache-site.template'
--- config/apache-site.template 2013-04-23 20:15:28 +0000
+++ config/apache-site.template 2013-08-01 13:01:28 +0000
@@ -27,4 +27,7 @@
2727
28 FallbackResource /index.html28 FallbackResource /index.html
2929
30 Header unset Cache-Control
31 Header set Cache-Control "max-age=0, public, must-revalidate"
32
30</VirtualHost>33</VirtualHost>
3134
=== modified file 'hooks/utils.py'
--- hooks/utils.py 2013-08-01 13:01:27 +0000
+++ hooks/utils.py 2013-08-01 13:01:28 +0000
@@ -561,6 +561,7 @@
561 with su('root'):561 with su('root'):
562 run('a2dissite', 'default')562 run('a2dissite', 'default')
563 run('a2ensite', 'juju-gui')563 run('a2ensite', 'juju-gui')
564 run('a2enmod', 'headers')
564565
565566
566def save_or_create_certificates(567def save_or_create_certificates(
567568
=== modified file 'revision'
--- revision 2013-07-31 17:30:56 +0000
+++ revision 2013-08-01 13:01:28 +0000
@@ -1,1 +1,5 @@
1<<<<<<< TREE
162262
3=======
463
5>>>>>>> MERGE-SOURCE
26
=== modified file 'tests/20-functional.test'
--- tests/20-functional.test 2013-06-12 09:07:29 +0000
+++ tests/20-functional.test 2013-08-01 13:01:28 +0000
@@ -16,6 +16,7 @@
16# You should have received a copy of the GNU Affero General Public License16# You should have received a copy of the GNU Affero General Public License
17# along with this program. If not, see <http://www.gnu.org/licenses/>.17# along with this program. If not, see <http://www.gnu.org/licenses/>.
1818
19import httplib
19import unittest20import unittest
20import urlparse21import urlparse
2122
@@ -23,6 +24,8 @@
23from selenium.webdriver.support import ui24from selenium.webdriver.support import ui
24from xvfbwrapper import Xvfb25from xvfbwrapper import Xvfb
2526
27# XXX 2013-07-30 benji bug=872264: Don't use juju_deploy directly, use
28# DeployTestMixin.juju_deploy instead. See comment in stop_services.
26from deploy import juju_deploy29from deploy import juju_deploy
27from helpers import (30from helpers import (
28 juju_destroy_service,31 juju_destroy_service,
@@ -129,24 +132,36 @@
129 # Just invoking ``juju destroy-service juju-gui`` in tearDown132 # Just invoking ``juju destroy-service juju-gui`` in tearDown
130 # should execute the ``stop`` hook, stopping all the services133 # should execute the ``stop`` hook, stopping all the services
131 # started by the charm in the machine. Right now this does not134 # started by the charm in the machine. Right now this does not
132 # work in pyJuju, so the same behavior is accomplished keeping135 # work in pyJuju, so the desired effect is achieved by keeping
133 # track of started services and manually stopping them here.136 # track of started services and manually stopping them here.
134 target = 'ubuntu@{0}'.format(hostname)137 target = 'ubuntu@{0}'.format(hostname)
135 for service in services:138 for service in services:
136 ssh(target, 'sudo', 'service', service, 'stop')139 ssh(target, 'sudo', 'service', service, 'stop')
137140
141 def juju_deploy(self, *args, **kws):
142 """Shim in our additional cleanup for pyJuju."""
143 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
144 # Once pyJuju works correctly or we drop support for it altogether, we
145 # can remove this shim.
146 unit_info = juju_deploy(*args, **kws)
147 if is_legacy_juju:
148 hostname = unit_info['public-address']
149 services = ['haproxy', 'apache2']
150 # Staging uses improv, otherwise the API agent is used.
151 if kws.get('options').get('staging') == 'true':
152 services.append('juju-api-improv')
153 else:
154 services.append('juju-api-agent')
155 self.addCleanup(self.stop_services, hostname, services)
156 return unit_info
157
138158
139class DeployTest(DeployTestMixin, unittest.TestCase):159class DeployTest(DeployTestMixin, unittest.TestCase):
140160
141 def test_api_agent(self):161 def test_api_agent(self):
142 # Ensure the Juju GUI and API agent services are correctly set up.162 # Ensure the Juju GUI and API agent services are correctly set up.
143 unit_info = juju_deploy(self.charm)163 unit_info = self.juju_deploy(self.charm)
144 hostname = unit_info['public-address']164 hostname = unit_info['public-address']
145 if is_legacy_juju:
146 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
147 self.addCleanup(
148 self.stop_services,
149 hostname, ['haproxy', 'apache2', 'juju-api-agent'])
150 self.navigate_to(hostname)165 self.navigate_to(hostname)
151 self.handle_browser_warning()166 self.handle_browser_warning()
152 self.assertEnvironmentIsConnected()167 self.assertEnvironmentIsConnected()
@@ -154,12 +169,8 @@
154 @unittest.skipUnless(is_legacy_juju, 'staging only works in pyJuju')169 @unittest.skipUnless(is_legacy_juju, 'staging only works in pyJuju')
155 def test_staging(self):170 def test_staging(self):
156 # Ensure the Juju GUI and improv services are correctly set up.171 # Ensure the Juju GUI and improv services are correctly set up.
157 unit_info = juju_deploy(self.charm, options={'staging': 'true'})172 unit_info = self.juju_deploy(self.charm, options={'staging': 'true'})
158 hostname = unit_info['public-address']173 hostname = unit_info['public-address']
159 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
160 self.addCleanup(
161 self.stop_services,
162 hostname, ['haproxy', 'apache2', 'juju-api-improv'])
163 self.navigate_to(hostname)174 self.navigate_to(hostname)
164 self.handle_browser_warning()175 self.handle_browser_warning()
165 self.assertEnvironmentIsConnected()176 self.assertEnvironmentIsConnected()
@@ -168,22 +179,35 @@
168179
169 def test_branch_source(self):180 def test_branch_source(self):
170 # Ensure the Juju GUI is correctly deployed from a Bazaar branch.181 # Ensure the Juju GUI is correctly deployed from a Bazaar branch.
171 unit_info = juju_deploy(182 unit_info = self.juju_deploy(
172 self.charm, options={'juju-gui-source': JUJU_GUI_TEST_BRANCH})183 self.charm, options={'juju-gui-source': JUJU_GUI_TEST_BRANCH})
173 hostname = unit_info['public-address']184 hostname = unit_info['public-address']
174 if is_legacy_juju:
175 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
176 self.addCleanup(
177 self.stop_services,
178 hostname, ['haproxy', 'apache2', 'juju-api-agent'])
179 self.navigate_to(hostname)185 self.navigate_to(hostname)
180 self.handle_browser_warning()186 self.handle_browser_warning()
181 self.assertEnvironmentIsConnected()187 self.assertEnvironmentIsConnected()
182188
189 def test_cache_headers(self):
190 # Make sure the correct cache headers are sent.
191 unit_info = self.juju_deploy(
192 self.charm, options={'juju-gui-source': JUJU_GUI_TEST_BRANCH})
193 hostname = unit_info['public-address']
194 conn = httplib.HTTPSConnection(hostname)
195 conn.request('GET', '/')
196 headers = conn.getresponse().getheaders()
197 # There is only one Cache-Control header.
198 self.assertEqual(zip(*headers)[0].count('cache-control'), 1)
199 # The right cache directives are in Cache-Control.
200 cache_control = dict(headers)['cache-control']
201 cache_directives = [s.strip() for s in cache_control.split(',')]
202 self.assertIn('max-age=0', cache_directives)
203 self.assertIn('public', cache_directives)
204 self.assertIn('must-revalidate', cache_directives)
205
206
183 @unittest.skipIf(is_legacy_juju, 'force-machine only works in juju-core')207 @unittest.skipIf(is_legacy_juju, 'force-machine only works in juju-core')
184 def test_force_machine(self):208 def test_force_machine(self):
185 # Ensure the Juju GUI is correctly set up in the Juju bootstrap node.209 # Ensure the Juju GUI is correctly set up in the Juju bootstrap node.
186 unit_info = juju_deploy(self.charm, force_machine=0)210 unit_info = self.juju_deploy(self.charm, force_machine=0)
187 self.assertEqual('0', unit_info['machine'])211 self.assertEqual('0', unit_info['machine'])
188 self.navigate_to(unit_info['public-address'])212 self.navigate_to(unit_info['public-address'])
189 self.handle_browser_warning()213 self.handle_browser_warning()

Subscribers

People subscribed via source and target branches

to all changes: