Merge ~alejdg/jenkins-agent-charm:fix_master_relation_setup into jenkins-agent-charm:master

[Alexandre Gomes]

Make sure secret is not overwritten outside of the relation context

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Haw Loeung (hloeung)]

LGTM

review: Approve (+1)

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

Change successfully merged at revision ac9b1d1220c1670af0dbb92bde13f1883d66b497

diff --git a/files/download-slave.sh b/files/download-slave.sh
index 831f066..dfe1462 100755
--- a/files/download-slave.sh
+++ b/files/download-slave.sh
@@ -29,7 +29,7 @@ if [ -s ${SLAVE_JAR}.new ] && [ -s ${JENKINS_JNLP}.new ]
 then
     mv ${SLAVE_JAR}.new ${SLAVE_JAR}
     mv ${JENKINS_JNLP}.new ${JENKINS_JNLP}
-	exit 0
+    exit 0
 else
-	exit 1
+    exit 1
 fi
diff --git a/reactive/jenkins_slave.py b/reactive/jenkins_slave.py
index 615084e..be3b256 100644
--- a/reactive/jenkins_slave.py
+++ b/reactive/jenkins_slave.py
@@ -96,7 +96,7 @@ def configure_jenkins_slave():
         write_default_conf(config.get('master_url'))
     elif kv.get('url'):
         status.maintenance("Using url from relation as 'master_url'")
-        write_default_conf(kv.get('url'))
+        write_default_conf(kv.get('url'), kv.get('secret'))
     else:
         status.maintenance("No 'master_url' set; not configuring slave at this time.")
         status.blocked("requires either slave relation or 'master_url'")
@@ -168,6 +168,7 @@ def slave_relation_changed():
 def slave_relation_removed():
     kv = unitdata.kv()
     kv.set('url', None)
+    kv.set('secret', None)
     reactive.clear_flag('slave-relation.available')
 
 
@@ -186,8 +187,9 @@ def slave_relation():
 
     url = hookenv.relation_get('url')
     if url:
-        kv.set('url', url)
         secret = hookenv.relation_get('secret')
+        kv.set('url', url)
+        kv.set('secret', secret)
         write_default_conf(url, secret=secret)
     else:
         hookenv.log("Master hasn't exported its url yet, exiting...")
@@ -229,7 +231,9 @@ def file_to_units(local_path, unit_path, perms=None, owner='root', group='root')
         host.write_file(path=unit_path, content=fh.read().encode(), owner=owner, group=group, perms=file_perms)
 
 
-def write_default_conf(master_url=None, owner='root', group='root', conf_path='/etc/default/jenkins-slave', secret=None):
+def write_default_conf(
+    master_url=None, owner='root', group='root', conf_path='/etc/default/jenkins-slave', secret=None
+):
     templates_dir = os.path.join(hookenv.charm_dir(), 'templates')
     slave_host = hookenv.local_unit().replace('/', '-')
     templating.render(
diff --git a/templates/jenkins-slave-default b/templates/jenkins-slave-default
index fcf5d35..b3b2148 100644
--- a/templates/jenkins-slave-default
+++ b/templates/jenkins-slave-default
@@ -65,7 +65,7 @@ JENKINS_JNLP="${JENKINS_HOME}/agent.jnlp"
 
 # Arguments to pass to jenkins slave on startup
 {% if secret %}
-JENKINS_SLAVE_SECRET="{{ secret 
+JENKINS_SLAVE_SECRET="{{ secret }}"
 
 JENKINS_ARGS="-jnlpUrl file:${JENKINS_JNLP} -secret ${JENKINS_SLAVE_SECRET}"
 {% else %}
diff --git a/tests/unit/test_jenkins_slave.py b/tests/unit/test_jenkins_slave.py
index d10d787..a0c1677 100644
--- a/tests/unit/test_jenkins_slave.py
+++ b/tests/unit/test_jenkins_slave.py
@@ -290,7 +290,8 @@ class TestSetDefaultConf(unittest.TestCase):
         config.return_value = {'master_url': ''}
         relation_get.return_value = 'http://10.1.1.1:8080'
         jenkins_slave.slave_relation()
-        self.assertFalse(relation_get.assert_called_once_with('url'))
+        expected = [mock.call('url'), mock.call('secret')]
+        self.assertFalse(relation_get.assert_has_calls(expected))
         self.assertFalse(clear_flag.assert_called_once_with('jenkins-slave.active'))
         self.assertFalse(set_flag.assert_called_once_with('slave-relation.configured'))
         expected = [