Merge ~charlie4284/jenkins-agent-charm:fix/relation into jenkins-agent-charm:master

[Yanks]

fix: set relation data regardless of other side relation data

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision efc192f2a4ae84114e88ce2f3e50d5739c02424c

diff --git a/reactive/jenkins_slave.py b/reactive/jenkins_slave.py
index 91f281c..90bba9c 100644
--- a/reactive/jenkins_slave.py
+++ b/reactive/jenkins_slave.py
@@ -1,7 +1,7 @@
 import os
 
-from charmhelpers.core import hookenv, host, templating, unitdata
 from charmhelpers.contrib.charmsupport import nrpe
+from charmhelpers.core import hookenv, host, templating, unitdata
 from charmhelpers.fetch import apt_purge
 from charms import apt, reactive
 from charms.layer import status
@@ -185,19 +185,6 @@ def slave_relation():
         hookenv.log("Config option 'master_url' is set. Can't use slave relation.")
         reactive.set_flag('slave-relation.configured')
         return
-
-    url = hookenv.relation_get('url')
-    if url:
-        secret = hookenv.relation_get('secret')
-        kv.set('url', url)
-        kv.set('secret', secret)
-        write_default_conf(url, secret=secret, download_jnlp_file=config.get('download_jnlp_file'))
-    else:
-        hookenv.log("Master hasn't exported its url yet, exiting...")
-        return
-
-    reactive.clear_flag('jenkins-slave.active')
-
     # Set the slave hostname to match the juju unit
     # in the jenkins master instance
     slave_host = hookenv.local_unit().replace('/', '-')
@@ -210,11 +197,26 @@ def slave_relation():
     else:
         labels = os.uname()[4]
 
-    # Set all relations
-    hookenv.relation_set(executors=noexecutors)
-    hookenv.relation_set(labels=labels)
-    hookenv.relation_set(slavehost=slave_host)
-    hookenv.relation_set(slaveaddress=slave_address)
+    # Set required relation data regardless of counterpart's relation data to
+    # reduce number of calls required to set up relation.
+    hookenv.relation_set(
+        executors=noexecutors,
+        labels=labels,
+        slavehost=slave_host,
+        slaveaddress=slave_address,
+    )
+
+    url = hookenv.relation_get('url')
+    if url:
+        secret = hookenv.relation_get('secret')
+        kv.set('url', url)
+        kv.set('secret', secret)
+        write_default_conf(url, secret=secret, download_jnlp_file=config.get('download_jnlp_file'))
+    else:
+        hookenv.log("Master hasn't exported its url yet, exiting...")
+        return
+
+    reactive.clear_flag('jenkins-slave.active')
     reactive.set_flag('slave-relation.configured')
 
 
diff --git a/tests/functional/bundles/overlays/local-charm-overlay.yaml.j2 b/tests/functional/bundles/overlays/local-charm-overlay.yaml.j2
index 1adcb54..eee9b6c 100644
--- a/tests/functional/bundles/overlays/local-charm-overlay.yaml.j2
+++ b/tests/functional/bundles/overlays/local-charm-overlay.yaml.j2
@@ -1,3 +1,3 @@
 applications:
   jenkins-slave:
-    charm: /tmp/charm-builds/jenkins-slave
+    charm: /tmp/charm-builds/jenkins-agent
diff --git a/tests/unit/test_jenkins_slave.py b/tests/unit/test_jenkins_slave.py
index 6d32481..ef1d584 100644
--- a/tests/unit/test_jenkins_slave.py
+++ b/tests/unit/test_jenkins_slave.py
@@ -8,7 +8,6 @@ import tempfile
 import unittest
 from unittest import mock
 
-
 # We also need to mock up charms.apt and charms.layer so we can run
 # unit tests without having to build the charm and pull in layers such
 # as layer-status.
@@ -21,7 +20,6 @@ from charms.layer import status  # NOQA: E402
 sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))))
 from reactive import jenkins_slave  # NOQA: E402
 
-
 INITIAL_CONF = 'tests/unit/files/jenkins-slave-default'
 
 
@@ -313,10 +311,7 @@ class TestSetDefaultConf(unittest.TestCase):
         self.assertFalse(clear_flag.assert_called_once_with('jenkins-slave.active'))
         self.assertFalse(set_flag.assert_called_once_with('slave-relation.configured'))
         expected = [
-            mock.call(executors=3),
-            mock.call(labels='x86_64'),
-            mock.call(slavehost='mock-jenkins-slave-0'),
-            mock.call(slaveaddress='10.1.2.3'),
+            mock.call(executors=3, labels='x86_64', slavehost='mock-jenkins-slave-0', slaveaddress='10.1.2.3'),
         ]
         self.assertFalse(relation_set.assert_has_calls(expected, any_order=True))
 
@@ -325,10 +320,7 @@ class TestSetDefaultConf(unittest.TestCase):
         relation_set.reset_mock()
         jenkins_slave.slave_relation()
         expected = [
-            mock.call(executors=3),
-            mock.call(labels='label1 label2'),
-            mock.call(slavehost='mock-jenkins-slave-0'),
-            mock.call(slaveaddress='10.1.2.3'),
+            mock.call(executors=3, labels='label1 label2', slavehost='mock-jenkins-slave-0', slaveaddress='10.1.2.3'),
         ]
         self.assertFalse(relation_set.assert_has_calls(expected, any_order=True))
 
@@ -346,10 +338,17 @@ class TestSetDefaultConf(unittest.TestCase):
     @mock.patch('charms.reactive.set_flag')
     @mock.patch('charmhelpers.core.hookenv.config')
     @mock.patch('charmhelpers.core.hookenv.relation_get')
-    def test_hook_slave_relation_relation_url_not_set(self, relation_get, config, set_flag, clear_flag):
+    @mock.patch('charmhelpers.core.hookenv.relation_set')
+    def test_hook_slave_relation_relation_url_not_set(self, relation_set, relation_get, config, set_flag, clear_flag):
         config.return_value = {'master_url': ''}
         relation_get.return_value = ''
         jenkins_slave.slave_relation()
+        relation_set.assert_called_once_with(
+            executors=3,
+            labels='x86_64',
+            slavehost='mock-jenkins-slave-0',
+            slaveaddress='10.1.2.3',
+        )
         self.assertFalse(clear_flag.assert_not_called())
 
     @mock.patch('charms.reactive.set_flag')