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

[Benji York]

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

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

[Gary Poster (gary)]

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

[Gary Poster (gary)]

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.

[Francesco Banconi (frankban)]

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'])

[Francesco Banconi (frankban)]

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

[Benji York (benji)]

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

[lp:~benji/charms/precise/juju-gui/fix-cache-headers]

72. By Benji York on 2013-08-01

fix service cleanup

[Gary Poster (gary)]

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.

[Marco Ceppi (marcoceppi)]

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

=== 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 @@
 it locally, e.g.:
 
     bzr checkout --lightweight lp:juju-plugins
+    ln -s juju-plugins/plugins/juju_test.py juju-test
+    export PATH="$PATH":`pwd`
 
-At this point, link "juju-plugins/plugins/juju_test.py" as "juju-test"
-somewhere in your PATH, so that it is possible to execute "juju-test".
+Alternatively you may check out lp:juju-plugins and link
+"juju-plugins/plugins/juju_test.py" as "juju-test" somewhere in your PATH, so
+that it is possible to execute "juju-test".
 
 Before being able to run the suite, test requirements need to be installed
 running the command:

=== modified file 'Makefile'
--- Makefile	2013-08-01 13:01:27 +0000
+++ Makefile	2013-08-01 13:01:28 +0000
@@ -26,7 +26,12 @@
 	./tests/10-unit.test
 	./tests/11-server.test
 
-ftest: setup
+ensure-juju-test:
+	@which juju-test > /dev/null \
+	    || (echo 'The "juju-test" command is missing.  See HACKING.md.' \
+		; false)
+
+ftest: setup ensure-juju-test
 	$(JUJUTEST) 20-functional.test
 
 # This will be eventually removed when we have juju-test --clean-state.
@@ -62,4 +67,5 @@
 	@echo '  service to be started.  If JUJU_ENV is not passed, the charm will'
 	@echo '  be deployed in the default Juju environment.'
 
-.PHONY: all clean deploy ftest help jujutest lint setup test unittest
+.PHONY: all clean deploy ensure-juju-test ftest help jujutest lint setup test \
+    unittest

=== 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 @@
 
     FallbackResource /index.html
 
+    Header unset Cache-Control
+    Header set Cache-Control "max-age=0, public, must-revalidate"
+
 </VirtualHost>

=== 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 @@
     with su('root'):
         run('a2dissite', 'default')
         run('a2ensite', 'juju-gui')
+        run('a2enmod', 'headers')
 
 
 def save_or_create_certificates(

=== modified file 'revision'
--- revision	2013-07-31 17:30:56 +0000
+++ revision	2013-08-01 13:01:28 +0000
@@ -1,1 +1,5 @@
+<<<<<<< TREE
 62
+=======
+63
+>>>>>>> MERGE-SOURCE

=== 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 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import httplib
 import unittest
 import urlparse
 
@@ -23,6 +24,8 @@
 from selenium.webdriver.support import ui
 from xvfbwrapper import Xvfb
 
+# XXX 2013-07-30 benji bug=872264: Don't use juju_deploy directly, use
+# DeployTestMixin.juju_deploy instead.  See comment in stop_services.
 from deploy import juju_deploy
 from helpers import (
     juju_destroy_service,
@@ -129,24 +132,36 @@
             # Just invoking ``juju destroy-service juju-gui`` in tearDown
             # should execute the ``stop`` hook, stopping all the services
             # started by the charm in the machine. Right now this does not
-            # work in pyJuju, so the same behavior is accomplished keeping
+            # work in pyJuju, so the desired effect is achieved by keeping
             # track of started services and manually stopping them here.
         target = 'ubuntu@{0}'.format(hostname)
         for service in services:
             ssh(target, 'sudo', 'service', service, 'stop')
 
+    def juju_deploy(self, *args, **kws):
+        """Shim in our additional cleanup for pyJuju."""
+        # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
+        # Once pyJuju works correctly or we drop support for it altogether, we
+        # can remove this shim.
+        unit_info = juju_deploy(*args, **kws)
+        if is_legacy_juju:
+            hostname = unit_info['public-address']
+            services = ['haproxy', 'apache2']
+            # Staging uses improv, otherwise the API agent is used.
+            if kws.get('options').get('staging') == 'true':
+                services.append('juju-api-improv')
+            else:
+                services.append('juju-api-agent')
+            self.addCleanup(self.stop_services, hostname, services)
+        return unit_info
+
 
 class DeployTest(DeployTestMixin, unittest.TestCase):
 
     def test_api_agent(self):
         # Ensure the Juju GUI and API agent services are correctly set up.
-        unit_info = juju_deploy(self.charm)
+        unit_info = self.juju_deploy(self.charm)
         hostname = unit_info['public-address']
-        if is_legacy_juju:
-            # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
-            self.addCleanup(
-                self.stop_services,
-                hostname, ['haproxy', 'apache2', 'juju-api-agent'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
@@ -154,12 +169,8 @@
     @unittest.skipUnless(is_legacy_juju, 'staging only works in pyJuju')
     def test_staging(self):
         # Ensure the Juju GUI and improv services are correctly set up.
-        unit_info = juju_deploy(self.charm, options={'staging': 'true'})
+        unit_info = self.juju_deploy(self.charm, options={'staging': 'true'})
         hostname = unit_info['public-address']
-        # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
-        self.addCleanup(
-            self.stop_services,
-            hostname, ['haproxy', 'apache2', 'juju-api-improv'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
@@ -168,22 +179,35 @@
 
     def test_branch_source(self):
         # Ensure the Juju GUI is correctly deployed from a Bazaar branch.
-        unit_info = juju_deploy(
+        unit_info = self.juju_deploy(
             self.charm, options={'juju-gui-source': JUJU_GUI_TEST_BRANCH})
         hostname = unit_info['public-address']
-        if is_legacy_juju:
-            # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
-            self.addCleanup(
-                self.stop_services,
-                hostname, ['haproxy', 'apache2', 'juju-api-agent'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
 
+    def test_cache_headers(self):
+        # Make sure the correct cache headers are sent.
+        unit_info = self.juju_deploy(
+            self.charm, options={'juju-gui-source': JUJU_GUI_TEST_BRANCH})
+        hostname = unit_info['public-address']
+        conn = httplib.HTTPSConnection(hostname)
+        conn.request('GET', '/')
+        headers = conn.getresponse().getheaders()
+        # There is only one Cache-Control header.
+        self.assertEqual(zip(*headers)[0].count('cache-control'), 1)
+        # The right cache directives are in Cache-Control.
+        cache_control = dict(headers)['cache-control']
+        cache_directives = [s.strip() for s in cache_control.split(',')]
+        self.assertIn('max-age=0', cache_directives)
+        self.assertIn('public', cache_directives)
+        self.assertIn('must-revalidate', cache_directives)
+        
+
     @unittest.skipIf(is_legacy_juju, 'force-machine only works in juju-core')
     def test_force_machine(self):
         # Ensure the Juju GUI is correctly set up in the Juju bootstrap node.
-        unit_info = juju_deploy(self.charm, force_machine=0)
+        unit_info = self.juju_deploy(self.charm, force_machine=0)
         self.assertEqual('0', unit_info['machine'])
         self.navigate_to(unit_info['public-address'])
         self.handle_browser_warning()