Merge lp:~canonical-ci-engineering/charms/precise/gunicorn/fix-config-changed-departed into lp:~charmers/charms/precise/gunicorn/trunk

[Andy Doan]

fix config-changed and wsgi_file_relation_broken hooks

These were depending on "remote unit" for the service name that's
only available in those contexts. This saves the service name so
that config-changed and wsgi_file_relation_broken can properly deal
with the upstart file.

[Evan (ev)]

Can we have a comment in the code for why __unit__ is needed? I want to make sure that when we try to merge back to gunicorn trunk, they understand the rationale behind all the changes without having to endlessly dig through bzr logs.

Otherwise, +1.

review: Approve

[Simon Davy (bloodearnest)]

Heya, I have a branch pending that fixes this issue, but differently. It also provides support for an upgrade path for those using the older env_extra relation paramter format.

https://code.launchpad.net/~bloodearnest/charms/precise/gunicorn/upgrade-path/+merge/213236

My solution was like yours to just use the gunicorn unit name rather than the remote unit name (which was the original authors design). I did not bother saving it, as it is always available to all gunicorn hooks, but maybe I missed some detail?

Be good to coordinate these landing.

[Evan (ev)]

Wow, well spotted Simon. Andy, any objections to picking up Simon's change?

[Andy Doan (doanac)]

> Wow, well spotted Simon. Andy, any objections to picking up Simon's change?

No, I'll take either. My change is slightly different. It still uses the remote-unit for the service name, so the upstart file/service name will continue to have the same name as the current version of the charm. This change uses the local-unit so things will be like "myservice-gunicorn" rather than the prior "myservice-django".

[Simon Davy (bloodearnest)]

Ah, so it does.

I'm not too bothered on the upstart name either way, tbh. Given that a user shouldn't really need to manually restart gunicorn often, I don't think it matters that much. Slight preference for just using local_unit to reduce the complexity of the charm slightly, but it's not a big deal.

[Andy Doan (doanac)]

On 04/02/2014 10:54 AM, Simon Davy wrote:
> Ah, so it does.
>
> I'm not too bothered on the upstart name either way, tbh. Given that a user shouldn't really need to manually restart gunicorn often, I don't think it matters that much. Slight preference for just using local_unit to reduce the complexity of the charm slightly, but it's not a big deal.

+1 for me then. I just really want to get a fix upstreamed so my team
doesn't have to carry this patch along.

[Matt Bruzek (mbruzek)]

This change was superceded by https://code.launchpad.net/~bloodearnest/charms/precise/gunicorn/upgrade-path/+merge/213236

review: Needs Fixing

[Matt Bruzek (mbruzek)]

Hello Andy,

Thanks for your submission this problem was addressed by Simon Davy (bloodearnest) in a different merge propsoal that was merged today. I rejected this merge proposal because you indicated that the fix that bloodearnest came up with would work for your team. Please contact me or reopen the merge proposal if you have anything else to add.

Thanks again,

  - Matt Bruzek

Unmerged revisions

30. By Andy Doan on 2014-03-28

fix config-changed and wsgi_file_relation_broken hooks

These were depending on "remote unit" for the service name that's
only available in those contexts. This saves the service name so
that config-changed and wsgi_file_relation_broken can properly deal
with the upstart file.

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-02-27 14:54:18 +0000
+++ hooks/hooks.py	2014-03-28 21:08:05 +0000
@@ -29,8 +29,8 @@
     return s
 
 
-def sanitized_service_name():
-    return sanitize(hookenv.remote_unit().split('/')[0])
+def sanitized_service_name(unit):
+    return sanitize(unit.split('/')[0])
 
 
 def upstart_conf_path(name):
@@ -76,8 +76,10 @@
 
     relation_data = relations[0]
 
-    service_name = sanitized_service_name()
+    service_name = sanitized_service_name(relation_data['__unit__'])
     wsgi_config['unit_name'] = service_name
+    if os.path.basename(sys.argv[0]) == 'wsgi-file-relation-joined':
+        hookenv.relation_set(None, {'service_name': service_name})
 
     project_conf = upstart_conf_path(service_name)
 
@@ -117,7 +119,7 @@
 
 @hooks.hook("wsgi_file_relation_broken")
 def wsgi_file_relation_broken():
-    service_name = sanitized_service_name()
+    service_name = hookenv.relation_get('service_name', hookenv.local_unit())
     host.service_stop(service_name)
     try:
         os.remove(upstart_conf_path(service_name))
 
=== modified file 'hooks/tests/test_hooks.py'
--- hooks/tests/test_hooks.py	2014-02-27 14:54:18 +0000
+++ hooks/tests/test_hooks.py	2014-03-28 21:08:05 +0000
@@ -47,7 +47,10 @@
         # environment context to juju hooks
 
         self.config = DEFAULTS.copy()
-        self.relation_data = {'working_dir': self.WORKING_DIR}
+        self.relation_data = {
+            'working_dir': self.WORKING_DIR,
+            '__unit__': 'foo/0'
+        }
 
         # intercept all charmsupport usage
         self.hookenv = self.apply_patch('hooks.hookenv')
@@ -147,10 +150,10 @@
     def test_configure_worker_class_gevent(self):
         self.do_worker_class('gevent')
 
-
-
     @patch('hooks.os.remove')
-    def test_wsgi_file_relation_broken(self, remove):
+    @patch('hooks.hookenv.relation_get')
+    def test_wsgi_file_relation_broken(self, relation_get, remove):
+        relation_get.return_value = self.SERVICE_NAME
         hooks.wsgi_file_relation_broken()
         self.host.service_stop.assert_called_once_with(self.SERVICE_NAME)
         remove.assert_called_once_with(