Merge ~hloeung/jenkins-slave-charm:switch-to-layer-status into jenkins-slave-charm:master

Proposed by Haw Loeung on 2019-03-13
Status: Merged
Approved by: Alexandre Gomes on 2019-03-13
Approved revision: 901a909d302fb4e3eeaa00be8cb2401a25240495
Merged at revision: e010d61685b05c5a20614b23fd97a94fb9bac2e2
Proposed branch: ~hloeung/jenkins-slave-charm:switch-to-layer-status
Merge into: jenkins-slave-charm:master
Diff against target: 175 lines (+26/-22)
3 files modified
layer.yaml (+1/-0)
reactive/jenkins_slave.py (+24/-22)
tests/unit/test_jenkins_slave.py (+1/-0)
Reviewer Review Type Date Requested Status
Joel Sing +1 2019-03-13 Approve on 2019-03-14
Canonical IS Reviewers 2019-03-14 Pending
Review via email: mp+364411@code.launchpad.net

Commit message

Switch from hookenv.status_set() to layer-status

Description of the change

As advised by Stuart, using layer-status has many benefits. One major one being that we don't have to keep track of 'blocked' status and that it is set to be the final state (avoids bouncing between non-blocked and blocked).

To post a comment you must log in.

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

Change successfully merged at revision e010d61685b05c5a20614b23fd97a94fb9bac2e2

Joel Sing (jsing) wrote :

LGTM

review: Approve (+1)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/layer.yaml b/layer.yaml
index 70c79a6..52ccf61 100644
--- a/layer.yaml
+++ b/layer.yaml
@@ -2,4 +2,5 @@ includes:
2 - layer:basic2 - layer:basic
3 - layer:apt3 - layer:apt
4 - layer:nagios4 - layer:nagios
5 - layer:status
5repo: lp:jenkins-slave-charm6repo: lp:jenkins-slave-charm
diff --git a/reactive/jenkins_slave.py b/reactive/jenkins_slave.py
index beecf87..1d5c545 100755
--- a/reactive/jenkins_slave.py
+++ b/reactive/jenkins_slave.py
@@ -4,29 +4,30 @@ from charmhelpers.core import hookenv, host, templating, unitdata
4from charmhelpers.contrib.charmsupport import nrpe4from charmhelpers.contrib.charmsupport import nrpe
5from charmhelpers.fetch import apt_purge5from charmhelpers.fetch import apt_purge
6from charms import apt, reactive6from charms import apt, reactive
7from charms.layer import status
78
89
9@reactive.hook('upgrade-charm')10@reactive.hook('upgrade-charm')
10def upgrade_charm():11def upgrade_charm():
11 hookenv.status_set('maintenance', 'forcing reconfiguration on upgrade-charm')12 status.maintenance('forcing reconfiguration on upgrade-charm')
12 reactive.clear_flag('jenkins-slave.active')13 reactive.clear_flag('jenkins-slave.active')
13 reactive.clear_flag('jenkins-slave.installed')14 reactive.clear_flag('jenkins-slave.installed')
1415
1516
16@reactive.when_not('jenkins-slave.installed')17@reactive.when_not('jenkins-slave.installed')
17def install():18def install():
18 hookenv.status_set('maintenance', 'installing jenkins-slave')19 status.maintenance('installing jenkins-slave')
19 reactive.clear_flag('jenkins-slave.active')20 reactive.clear_flag('jenkins-slave.active')
2021
21 config = hookenv.config()22 config = hookenv.config()
2223
23 hookenv.log('Adding jenkins-slave dependencies to be installed')24 status.maintenance('Adding jenkins-slave dependencies to be installed')
24 packages = ['wget', 'default-jre-headless']25 packages = ['wget', 'default-jre-headless']
2526
26 # Install extra packages needed by the slave.27 # Install extra packages needed by the slave.
27 tools = config.get('tools')28 tools = config.get('tools')
28 if tools:29 if tools:
29 hookenv.log('Adding jenkins-slave additional tools to be installed: {}'.format(tools))30 status.maintenance('Adding jenkins-slave additional tools to be installed: {}'.format(tools))
30 for package in tools.split():31 for package in tools.split():
31 packages.append(package)32 packages.append(package)
32 apt.queue_install(packages)33 apt.queue_install(packages)
@@ -34,35 +35,35 @@ def install():
34 return # apt layer already set blocked state.35 return # apt layer already set blocked state.
3536
36 # Get rid of the legacy jenkins-slave package, including config files.37 # Get rid of the legacy jenkins-slave package, including config files.
37 hookenv.log('Removing the old jenkins-slave package... (obsoleted by this charm)')38 status.maintenance('Removing the old jenkins-slave package... (obsoleted by this charm)')
38 apt_purge(['jenkins-slave'])39 apt_purge(['jenkins-slave'])
3940
40 # Create jenkins user if it doesn't exist.41 # Create jenkins user if it doesn't exist.
41 if host.user_exists('jenkins'):42 if host.user_exists('jenkins'):
42 hookenv.log('Installing jenkins-slave (user account already exists)...')43 status.maintenance('Installing jenkins-slave (user account already exists)...')
43 else:44 else:
44 hookenv.log('Installing jenkins-slave (user account)...')45 status.maintenance('Installing jenkins-slave (user account)...')
45 host.adduser(username='jenkins', system_user=True, home_dir='/var/lib/jenkins')46 host.adduser(username='jenkins', system_user=True, home_dir='/var/lib/jenkins')
4647
47 # And ensure required directories exist and are set up.48 # And ensure required directories exist and are set up.
48 hookenv.log('Installing jenkins-slave (directories)...')49 status.maintenance('Installing jenkins-slave (directories)...')
49 host.mkdir('/var/lib/jenkins', owner='jenkins', group='jenkins')50 host.mkdir('/var/lib/jenkins', owner='jenkins', group='jenkins')
50 host.mkdir('/var/log/jenkins', owner='jenkins', group='jenkins')51 host.mkdir('/var/log/jenkins', owner='jenkins', group='jenkins')
5152
52 hookenv.log('Installing jenkins-slave (common files)...')53 status.maintenance('Installing jenkins-slave (common files)...')
53 write_default_conf()54 write_default_conf()
54 file_to_units('files/download-slave.sh', '/usr/local/sbin/download-slave.sh')55 file_to_units('files/download-slave.sh', '/usr/local/sbin/download-slave.sh')
55 file_to_units('files/jenkins-slave-logrotate-config', '/etc/logrotate.d/jenkins-slave')56 file_to_units('files/jenkins-slave-logrotate-config', '/etc/logrotate.d/jenkins-slave')
5657
57 if host.lsb_release()['DISTRIB_CODENAME'] == 'trusty':58 if host.lsb_release()['DISTRIB_CODENAME'] == 'trusty':
58 hookenv.log('Installing jenkins-slave (upstart job)...')59 status.maintenance('Installing jenkins-slave (upstart job)...')
59 file_to_units('files/jenkins-slave-upstart-config', '/etc/init/jenkins-slave.conf')60 file_to_units('files/jenkins-slave-upstart-config', '/etc/init/jenkins-slave.conf')
60 else:61 else:
61 hookenv.log('Installing jenkins-slave (system unit)...')62 status.maintenance('Installing jenkins-slave (system unit)...')
62 file_to_units('files/jenkins-slave-systemd-config', '/lib/systemd/system/jenkins-slave.service')63 file_to_units('files/jenkins-slave-systemd-config', '/lib/systemd/system/jenkins-slave.service')
63 host.service('enable', 'jenkins-slave')64 host.service('enable', 'jenkins-slave')
6465
65 hookenv.log('Installing jenkins-slave... done.')66 status.maintenance('Installing jenkins-slave... done.')
66 reactive.clear_flag('jenkins-slave.blocked')67 reactive.clear_flag('jenkins-slave.blocked')
67 reactive.clear_flag('jenkins-slave.configured')68 reactive.clear_flag('jenkins-slave.configured')
68 reactive.set_flag('jenkins-slave.installed')69 reactive.set_flag('jenkins-slave.installed')
@@ -70,6 +71,7 @@ def install():
7071
71@reactive.when('config.changed')72@reactive.when('config.changed')
72def config_changed():73def config_changed():
74 status.maintenance('Configuration changed')
73 reactive.clear_flag('jenkins-slave.blocked')75 reactive.clear_flag('jenkins-slave.blocked')
74 reactive.clear_flag('jenkins-slave.configured')76 reactive.clear_flag('jenkins-slave.configured')
75 reactive.clear_flag('nagios-nrpe.configured')77 reactive.clear_flag('nagios-nrpe.configured')
@@ -79,21 +81,21 @@ def config_changed():
79@reactive.when_not('jenkins-slave.configured')81@reactive.when_not('jenkins-slave.configured')
80@reactive.when_not('jenkins-slave.blocked')82@reactive.when_not('jenkins-slave.blocked')
81def configure_jenkins_slave():83def configure_jenkins_slave():
82 hookenv.status_set('maintenance', 'configuring jenkins-slave')84 status.maintenance('configuring jenkins-slave')
83 reactive.clear_flag('jenkins-slave.active')85 reactive.clear_flag('jenkins-slave.active')
8486
85 config = hookenv.config()87 config = hookenv.config()
86 kv = unitdata.kv()88 kv = unitdata.kv()
8789
88 if config.get('master_url'):90 if config.get('master_url'):
89 hookenv.log("Using 'master_url' to configure the slave.")91 status.maintenance("Using 'master_url' to configure the slave.")
90 write_default_conf(config.get('master_url'))92 write_default_conf(config.get('master_url'))
91 elif kv.get('url'):93 elif kv.get('url'):
92 hookenv.log("Using url from relation as 'master_url'")94 status.maintenance("Using url from relation as 'master_url'")
93 write_default_conf(kv.get('url'))95 write_default_conf(kv.get('url'))
94 else:96 else:
95 hookenv.log("No 'master_url' set; not configuring slave at this time.")97 status.maintenance("No 'master_url' set; not configuring slave at this time.")
96 hookenv.status_set('blocked', "requires either slave relation or 'master_url'")98 status.blocked("requires either slave relation or 'master_url'")
97 reactive.set_flag('jenkins-slave.blocked')99 reactive.set_flag('jenkins-slave.blocked')
98 return100 return
99101
@@ -113,7 +115,7 @@ def blocked_on_jenkins_url():
113@reactive.when('jenkins-slave.active')115@reactive.when('jenkins-slave.active')
114@reactive.when_not('nagios-nrpe.configured')116@reactive.when_not('nagios-nrpe.configured')
115def configure_nagios(nagios):117def configure_nagios(nagios):
116 hookenv.status_set('maintenance', 'setting up NRPE checks')118 status.maintenance('setting up NRPE checks')
117119
118 # Use charmhelpers.contrib.charmsupport's nrpe to determine hostname120 # Use charmhelpers.contrib.charmsupport's nrpe to determine hostname
119 hostname = nrpe.get_nagios_hostname()121 hostname = nrpe.get_nagios_hostname()
@@ -133,13 +135,13 @@ def set_active():
133 # install, config-changed and upgrade-charm paths. It needs to be135 # install, config-changed and upgrade-charm paths. It needs to be
134 # tolerant of a running or stopped jenkins-slave.136 # tolerant of a running or stopped jenkins-slave.
135 if host.service_running('jenkins-slave'):137 if host.service_running('jenkins-slave'):
136 hookenv.log('Restarting jenkins-slave...')138 status.maintenance('Restarting jenkins-slave...')
137 host.service_restart('jenkins-slave')139 host.service_restart('jenkins-slave')
138 else:140 else:
139 hookenv.log('Starting jenkins-slave...')141 status.maintenance('Starting jenkins-slave...')
140 host.service_start('jenkins-slave')142 host.service_start('jenkins-slave')
141143
142 hookenv.status_set('active', 'ready')144 status.active('ready')
143 reactive.set_flag('jenkins-slave.active')145 reactive.set_flag('jenkins-slave.active')
144146
145147
@@ -162,7 +164,7 @@ def slave_relation_removed():
162@reactive.when('slave-relation.available')164@reactive.when('slave-relation.available')
163@reactive.when_not('slave-relation.configured')165@reactive.when_not('slave-relation.configured')
164def slave_relation():166def slave_relation():
165 hookenv.status_set('maintenance', 'setting up jenkins via slave relation')167 status.maintenance('setting up jenkins via slave relation')
166 config = hookenv.config()168 config = hookenv.config()
167 kv = unitdata.kv()169 kv = unitdata.kv()
168170
diff --git a/tests/unit/test_jenkins_slave.py b/tests/unit/test_jenkins_slave.py
index 06879e5..7bd3ead 100644
--- a/tests/unit/test_jenkins_slave.py
+++ b/tests/unit/test_jenkins_slave.py
@@ -9,6 +9,7 @@ from unittest import mock
99
10sys.modules['charms.apt'] = mock.MagicMock()10sys.modules['charms.apt'] = mock.MagicMock()
11from charms import apt # NOQA: E40211from charms import apt # NOQA: E402
12sys.modules['charms.layer'] = mock.MagicMock()
1213
13# Add path to where our reactive layer lives and import.14# Add path to where our reactive layer lives and import.
14sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))))15sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))))

Subscribers

People subscribed via source and target branches