Merge lp:~psivaa/charms/trusty/jenkaas/install-jenkins into lp:~canonical-ci-engineering/charms/trusty/jenkaas/trunk

Proposed by Para Siva
Status: Merged
Approved by: Para Siva
Approved revision: 18
Merged at revision: 3
Proposed branch: lp:~psivaa/charms/trusty/jenkaas/install-jenkins
Merge into: lp:~canonical-ci-engineering/charms/trusty/jenkaas/trunk
Diff against target: 104 lines (+19/-17)
4 files modified
README (+4/-1)
config.yaml (+2/-0)
hooks/actions.py (+11/-14)
hooks/services.py (+2/-2)
To merge this branch: bzr merge lp:~psivaa/charms/trusty/jenkaas/install-jenkins
Reviewer Review Type Date Requested Status
Joe Talbott (community) Approve
Francis Ginther Approve
Review via email: mp+261820@code.launchpad.net

Commit message

Install jenkins package from http://archive.admin.canonical.com/

Description of the change

Install jenkins package from files/ directory which should be downloaded from http://archive.admin.canonical.com/ by the mojo spec. As part of incremental MP's. This one is to install jenkins.

To post a comment you must log in.
Revision history for this message
Para Siva (psivaa) wrote :

It looks like the 'deb' file needs to be downloaded and then installed here. Need to have a chat with IS about it.

7. By Para Siva

Typo in config yaml

8. By Para Siva

Fix typo properly

Revision history for this message
Francis Ginther (fginther) wrote :

I think the dpkg call will fail as is (and from IRC, looks like you already ran into this). But I think this is the right approach overall.

review: Needs Fixing
9. By Para Siva

Install dependency packages from jenkins ci

10. By Para Siva

jenkins ci deb line is not needed

11. By Para Siva

force install

12. By Para Siva

Charm name typo

13. By Para Siva

Install deps package with --fix-broken

14. By Para Siva

No need to setup admin user

Revision history for this message
Para Siva (psivaa) wrote :

Thanks Francis for the review and the information about missing symlinks. Rev 14 now installs jenkins and I was able to deploy this in wendigo. A re-review will be really helpful.

Revision history for this message
Joe Talbott (joetalbott) wrote :

One minor typo.

And a question. Is this charm going to always install the local deb or do I need to pass something in via the mojo spec to tell it to use the local deb? The charmstore version of jenkins takes the 'release' option, which when set to 'local' uses the local deb.

review: Needs Fixing
Revision history for this message
Francis Ginther (fginther) wrote :

> And a question. Is this charm going to always install the local deb or do I
> need to pass something in via the mojo spec to tell it to use the local deb?
> The charmstore version of jenkins takes the 'release' option, which when set
> to 'local' uses the local deb.

I vote for the answer being "This charm will only ever install a local .deb." and so there is no to specify any parameters of special logic for handling anything else. I think we can rely on IS always wanting to use a specific jenkins.deb package.

Revision history for this message
Francis Ginther (fginther) wrote :

I'd leave the charm name as 'jenkaas' (the one specified in the metafile) and also matches the trunk name. This charm is specific to our deployment and not a general purpose jenkins charm, so it makes sense to me to use a unique name.

Otherwise, looks good.

review: Needs Fixing
15. By Para Siva

Review comment fixes, install as jenkaas, specify only local .deb are supported to install jenkins

Revision history for this message
Para Siva (psivaa) wrote :

Thanks again. I've fixed them in rev 15 and added a line in README for stating that it will be local deb that will be supported.

Revision history for this message
Francis Ginther (fginther) wrote :

Cool, looks good now.

review: Approve
Revision history for this message
Francis Ginther (fginther) wrote :

Oops, found a reference to create_admin_user() that was left in while trying to deploy a mojo spec.

review: Needs Fixing
16. By Para Siva

Remove the removed function from services

Revision history for this message
Para Siva (psivaa) wrote :

So sorry about that. Forgot to removed it. Pushed it to rev 16

17. By Para Siva

Change service name to jenkaas

18. By Para Siva

pyflakes and pep8

Revision history for this message
Francis Ginther (fginther) wrote :

Approve.

review: Approve
Revision history for this message
Joe Talbott (joetalbott) wrote :

Looks good. You found the other things that I discovered last night as well. Good job!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'README'
2--- README 2015-06-11 17:19:34 +0000
3+++ README 2015-06-16 08:35:02 +0000
4@@ -13,4 +13,7 @@
5
6 The charm expects the payload code to be provided as
7 files/jenkaas.tgz (a snapshot of lp:jenkaas) along
8-with its corresponding files/pip-cache directory (python dependencies).
9+with its corresponding files/pip-cache directory (python dependencies).
10+
11+This charm will only ever install a local .deb provided under the files/
12+directory.
13
14=== modified file 'config.yaml'
15--- config.yaml 2015-06-11 18:26:32 +0000
16+++ config.yaml 2015-06-16 08:35:02 +0000
17@@ -1,3 +1,5 @@
18+options:
19+ environment:
20 default: "production"
21 type: string
22 description: |
23
24=== modified file 'hooks/actions.py'
25--- hooks/actions.py 2015-06-11 18:26:32 +0000
26+++ hooks/actions.py 2015-06-16 08:35:02 +0000
27@@ -4,18 +4,17 @@
28
29 from charmhelpers import fetch
30 from charmhelpers.core import hookenv
31-from charmhelpers.core.host import adduser
32-from charmhelpers.payload import (archive, execd)
33+from charmhelpers.payload import execd
34
35 SERVICE_NAME = 'jenkaas'
36 SERVICE_CONFIGNAME = 'jenkaas.conf'
37+DEPS_PKGES = ["daemon", "adduser", "psmisc", "default-jre"]
38
39 config = hookenv.config()
40
41
42 def _service_dir():
43- template = '/srv/{}/{}'
44- return template.format(config['environment'], SERVICE_NAME)
45+ return '/var/lib/jenkins'
46
47
48 def basenode(service_name):
49@@ -26,12 +25,10 @@
50 def log_start(service_name):
51 hookenv.log('%s starting', SERVICE_NAME)
52
53-def install_service_tarball(service_name):
54- files_dir = os.path.join(hookenv.charm_dir(), 'files')
55- tarball = os.path.join(files_dir, '{}.tgz'.format(SERVICE_NAME))
56- if not os.path.exists(_service_dir()):
57- hookenv.log('Installing the code for the first time from tarball')
58- archive.extract_tarfile(tarball, os.path.dirname(_service_dir()))
59+
60+def install_jenkins(service_name):
61+ jenkins_deb = os.path.join(hookenv.charm_dir(), 'files/jenkins.deb')
62+ subprocess.check_call(['dpkg', '-i', jenkins_deb])
63
64
65 def update_config_file(service_name):
66@@ -43,7 +40,7 @@
67 f.write(config_content)
68
69
70-def create_admin_user(service_name):
71- username = 'admin'
72- hookenv.log('Creating service user: %s', username)
73- adduser(username)
74+def install_jenkins_dep_pkges(service_name):
75+ hookenv.log('Installing dependency packages')
76+ fetch.apt_update()
77+ fetch.apt_install(DEPS_PKGES, options=['--fix-broken', ], fatal=True)
78
79=== added symlink 'hooks/config-changed'
80=== target is u'hooks.py'
81=== added symlink 'hooks/install'
82=== target is u'hooks.py'
83=== modified file 'hooks/services.py'
84--- hooks/services.py 2015-06-11 18:26:32 +0000
85+++ hooks/services.py 2015-06-16 08:35:02 +0000
86@@ -13,9 +13,9 @@
87 'required_data': [config],
88 'data_ready': [
89 actions.basenode,
90- actions.install_service_tarball,
91+ actions.install_jenkins_dep_pkges,
92+ actions.install_jenkins,
93 actions.update_config_file,
94- actions.create_admin_user,
95 helpers.render_template(
96 source='upstart.conf',
97 target='/etc/init/jenkaas.conf'),
98
99=== added symlink 'hooks/start'
100=== target is u'hooks.py'
101=== added symlink 'hooks/stop'
102=== target is u'hooks.py'
103=== added symlink 'hooks/upgrade-charm'
104=== target is u'hooks.py'

Subscribers

People subscribed via source and target branches