Merge lp:~jcsackett/charmworld/trigger-test-builds into lp:charmworld

Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: 480
Merged at revision: 475
Proposed branch: lp:~jcsackett/charmworld/trigger-test-builds
Merge into: lp:charmworld
Diff against target: 146 lines (+64/-2)
4 files modified
charmworld/jobs/ingest.py (+17/-0)
charmworld/jobs/tests/test_ingest.py (+36/-2)
default.ini (+6/-0)
test-setup.ini (+5/-0)
To merge this branch: bzr merge lp:~jcsackett/charmworld/trigger-test-builds
Reviewer Review Type Date Requested Status
Juju Gui Bot continuous-integration Approve
Curtis Hovey (community) code Approve
Review via email: mp+201466@code.launchpad.net

Commit message

Automatically trigger builds for new charms and new revisions of existing charms.

Description of the change

This branch introduces a feature to automatically trigger test builds for new
charms and new revisions of existing charms as they are ingested.

charmworld/jobs/ingest.py
-------------------------
* `trigger_tests` triggers a test build for a branch and revision on launchpad.
  The associated jenkins instance is configured to start a build whenever a GET
  is made on the created URL in this function.

* `trigger_tests` is added to the block corresponding to updating a charm, as we
  only want to run it under the circumstances when the charm needs updating on
  charmworld.

Misc
----
* Tests have been added.
* Settings specific to the jenkins instance for testing have been added to the
  ini.

To post a comment you must log in.
477. By j.c.sackett

Add option to disable testing builds on ingest.

478. By j.c.sackett

Remove token from ini. It should probably not be public.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Line 18
    if settings['run_test_builds'] != 'false':
could be expressed as a positive
    if settings['run_test_builds'] == 'true':
which also ensures tests only run under a narrow condition that we control

The test and format string appear to be missing 'token'
    http://%s/job/charm-delegator/buildWithParameters?=%s&branch=lp:foo&revno=1
I expected
    http://%s/job/charm-delegator/buildWithParameters?token=%s&branch=lp:foo&revno=1

The jenkins_url should not be configured by default. Developers will run this. Maybe someone will run it themselves. Only manage.jujucharms.com will be configured to send data to jenkins, only it will have all the information to trigger a jenkins job. The tests or develop can configure the jenkins url as needed.

It's an error to run_test_builds to true, not but not set jenkins_url. I imagine we would see ingest slow,
waiting for each call to timeout. Maybe we don't need run_test_builds, or we want a sanity check to verify charmworld has enough information. I am not sure raising an error will help though. We will only see it if we look in the log.
It might be nice to print to stdout and maybe log that the url is not configured.

jenkins_url and run_test_builds don't appear related by name. As charm test data might come from several
places, maybe we want names that describe the intent clearly. Maybe charm_test_url and run_charm_tests?

The jenkins URL is somewhat brittle. The protocol might change. I don't think we need to separate the token from the URL. Can we place a formate template in a single value?
    charm_test_url: http://1.2.3.4:8080/job/charm-delegator/buildWithParameters?token=DING-DONG&branch={branch}&revno={revno}'
and trigger_tests() might do
    charm_test_url = settings['charm_test_url']
    url = charm_test_url.format(dict(branch=charm_branch, revno=revision))
charmworld only cares about a template that defines {branch} and {revno}; Everything is Jenkin's concern.

review: Needs Information (code)
479. By j.c.sackett

Addresses comments in review.

Revision history for this message
j.c.sackett (jcsackett) wrote :

So, it occurred to me as I tried to resolve the issues with "run_test_builds" that we don't actually need it. Whether or not tests run can simply be based off whether jenkins_url has been set. If it's not set, there's either an error and we shouldn't run, or we don't want to run so we didn't provide it.

I think that addresses the bulk of the comments.

As to the jenkins_url/jenkins_token problem, I'm not sure I agree with the proposed url pattern change. I think providing the http://... and the token separately allows someone working with just the deployment to easily figure out what should be changed without knowing what's going on in the code. If the token on the jenkins instance was altered, change the token, and if the url has changed, change that. Since both of those are things that I think *can* change independently, they should be set indepenently. Does that make sense to you?

Revision history for this message
Curtis Hovey (sinzui) wrote :

The jenkins_url/jenkins_token problem is not two points of change. The IP will change, the port may change. The job name can change. The token will change if we think it is compromised. Charmworld doesn't care about any of those details. We may not use Jenkins to test charms. We are certain of is that charmworld may need to tell some URI that a charm at a specific revision will be tested.

480. By j.c.sackett

Changes to address commentar in review.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you.

review: Approve (code)
Revision history for this message
Juju Gui Bot (juju-gui-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmworld/jobs/ingest.py'
2--- charmworld/jobs/ingest.py 2014-01-02 17:34:07 +0000
3+++ charmworld/jobs/ingest.py 2014-01-14 14:38:16 +0000
4@@ -51,6 +51,7 @@
5 SearchServiceNotAvailable,
6 )
7 from charmworld.utils import (
8+ get_ini,
9 LAST_INGEST_JOB_FINISHED,
10 quote_key,
11 quote_yaml,
12@@ -251,6 +252,21 @@
13 charm_data['hash'] = h.hexdigest()
14
15
16+def trigger_tests(charm_branch, revision):
17+ settings = get_ini()
18+ charm_test_url = settings.get('charm_test_url', None)
19+ if charm_test_url:
20+ # charm_test URL hasn't been set; don't trigger tests.
21+ token = settings['charm_test_token']
22+ job = settings['charm_test_job']
23+ url = ('http://%s/job/%s/buildWithParameters?'
24+ 'token=%s&branch=%s&revno=%s') % (
25+ charm_test_url, token, job, charm_branch, revision)
26+ # Having assembled the url, all we need to do to trigger the build is
27+ # call get on the url.
28+ requests.get(url)
29+
30+
31 def should_update_charm(this_revision, newest_revision, charm_in_db):
32 return this_revision == newest_revision or not charm_in_db
33
34@@ -302,6 +318,7 @@
35 index_client = ElasticSearchClient.from_settings(settings)
36 self.log.info('Saving %s' % charm_data['_id'])
37 CharmSource(self.db, index_client).save(charm_data)
38+ trigger_tests(charm_data['branch_spec'], newest_revision)
39 else:
40 self.log.info('Skipping %s' % charm_data['_id'])
41 self.update_ingest_status()
42
43=== modified file 'charmworld/jobs/tests/test_ingest.py'
44--- charmworld/jobs/tests/test_ingest.py 2014-01-02 17:34:07 +0000
45+++ charmworld/jobs/tests/test_ingest.py 2014-01-14 14:38:16 +0000
46@@ -34,6 +34,7 @@
47 run_job,
48 should_update_charm,
49 slurp_files,
50+ trigger_tests,
51 update_branch,
52 update_charm,
53 update_hash,
54@@ -54,6 +55,7 @@
55 TestCase,
56 )
57 from charmworld.utils import (
58+ get_ini,
59 LAST_INGEST_JOB_FINISHED,
60 quote_key,
61 )
62@@ -183,10 +185,41 @@
63 charm_data = factory.get_charm_json(revision=1)
64 update_handler = self.get_handler('charm.update')
65 with charm_update_environment(charm_data, self.use_index_client()):
66- run_job(UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
67+ with patch('charmworld.jobs.ingest.trigger_tests'):
68+ run_job(
69+ UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
70 logs = ''.join(r.getMessage() for r in update_handler.buffer)
71 self.assertEqual('Saving %s' % charm_data['_id'], logs)
72
73+ def test_trigger_tests_builds_correct_url(self):
74+ with patch('requests.get') as patched_get:
75+ trigger_tests('lp:foo', 1)
76+ settings = get_ini()
77+ charm_test_url = settings['charm_test_url']
78+ charm_test_token = settings['charm_test_token']
79+ charm_test_job = settings['charm_test_job']
80+ expected_url = ('http://%s/job/%s/buildWithParameters?'
81+ 'token=%s&branch=lp:foo&revno=1') % (
82+ charm_test_url, charm_test_token, charm_test_job)
83+ patched_get.assert_called_once_with(expected_url)
84+
85+ def test_run_does_not_trigger_tests_without_new_revision(self):
86+ charm_data = factory.get_charm_json(revision=2)
87+ self.db.charms.save(charm_data)
88+ with charm_update_environment(charm_data, self.use_index_client()):
89+ with patch('charmworld.jobs.ingest.trigger_tests') as trigger:
90+ run_job(
91+ UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
92+ self.assertEqual(0, trigger.call_count)
93+
94+ def test_run_triggers_tests_with_new_revision(self):
95+ charm_data = factory.get_charm_json(revision=1)
96+ with charm_update_environment(charm_data, self.use_index_client()):
97+ with patch('charmworld.jobs.ingest.trigger_tests') as trigger:
98+ run_job(
99+ UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
100+ trigger.assert_called_once_with(charm_data['branch_spec'], 2)
101+
102 def test_saves_early_error(self):
103 charm_data = factory.get_charm_json()
104 # Induce improbable error -- bname must be a string.
105@@ -194,7 +227,8 @@
106 update_handler = self.get_handler('charm.update')
107 update_charm_handler = self.get_handler('charm.update_charm')
108 with patch('charmworld.jobs.ingest.update_from_store'):
109- run_job(UpdateCharmJob(), self.payload(charm_data), db=self.db)
110+ with patch('charmworld.jobs.ingest.trigger_tests'):
111+ run_job(UpdateCharmJob(), self.payload(charm_data), db=self.db)
112 self.assertIsNotNone(self.db.charms.find_one(charm_data['_id']))
113 logs = ''.join(r.getMessage() for r in update_handler.buffer)
114 update_charm_logs = ''.join(r.getMessage()
115
116=== modified file 'default.ini'
117--- default.ini 2014-01-07 16:38:00 +0000
118+++ default.ini 2014-01-14 14:38:16 +0000
119@@ -62,6 +62,12 @@
120 # below.
121 proof.port = 2464
122
123+# Info for triggering new test builds for new charm revisions
124+run_test_builds = false
125+charm_test_url =
126+charm_test_token =
127+charm_test_job =
128+
129 [server:main]
130 use = egg:Paste#http
131 host = 0.0.0.0
132
133=== modified file 'test-setup.ini'
134--- test-setup.ini 2013-07-17 21:40:06 +0000
135+++ test-setup.ini 2014-01-14 14:38:16 +0000
136@@ -20,5 +20,10 @@
137 # Datastores
138 mongo.database = juju_test
139
140+# Set nonsense to check charm test trigger function.
141+charm_test_url = http://example.com/
142+charm_test_token = 'this-is-not-the-real-token'
143+charm_test_job = "aCharmTestingJob"
144+
145 # This signals that we are running under test.
146 is_testing = true

Subscribers

People subscribed via source and target branches

to all changes: