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

[j.c.sackett]

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

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.

[lp:~jcsackett/charmworld/trigger-test-builds]

477. By j.c.sackett on 2014-01-13

Add option to disable testing builds on ingest.

478. By j.c.sackett on 2014-01-13

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

[Curtis Hovey (sinzui)]

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)

[lp:~jcsackett/charmworld/trigger-test-builds]

479. By j.c.sackett on 2014-01-13

Addresses comments in review.

[j.c.sackett (jcsackett)]

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?

[Curtis Hovey (sinzui)]

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.

[lp:~jcsackett/charmworld/trigger-test-builds]

480. By j.c.sackett on 2014-01-14

Changes to address commentar in review.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2014-01-02 17:34:07 +0000
+++ charmworld/jobs/ingest.py	2014-01-14 14:38:16 +0000
@@ -51,6 +51,7 @@
     SearchServiceNotAvailable,
 )
 from charmworld.utils import (
+    get_ini,
     LAST_INGEST_JOB_FINISHED,
     quote_key,
     quote_yaml,
@@ -251,6 +252,21 @@
     charm_data['hash'] = h.hexdigest()
 
 
+def trigger_tests(charm_branch, revision):
+    settings = get_ini()
+    charm_test_url = settings.get('charm_test_url', None)
+    if charm_test_url:
+        # charm_test URL hasn't been set; don't trigger tests.
+        token = settings['charm_test_token']
+        job = settings['charm_test_job']
+        url = ('http://%s/job/%s/buildWithParameters?'
+               'token=%s&branch=%s&revno=%s') % (
+                   charm_test_url, token, job, charm_branch, revision)
+        # Having assembled the url, all we need to do to trigger the build is
+        # call get on the url.
+        requests.get(url)
+
+
 def should_update_charm(this_revision, newest_revision, charm_in_db):
     return this_revision == newest_revision or not charm_in_db
 
@@ -302,6 +318,7 @@
             index_client = ElasticSearchClient.from_settings(settings)
             self.log.info('Saving %s' % charm_data['_id'])
             CharmSource(self.db, index_client).save(charm_data)
+            trigger_tests(charm_data['branch_spec'], newest_revision)
         else:
             self.log.info('Skipping %s' % charm_data['_id'])
         self.update_ingest_status()
 
=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2014-01-02 17:34:07 +0000
+++ charmworld/jobs/tests/test_ingest.py	2014-01-14 14:38:16 +0000
@@ -34,6 +34,7 @@
     run_job,
     should_update_charm,
     slurp_files,
+    trigger_tests,
     update_branch,
     update_charm,
     update_hash,
@@ -54,6 +55,7 @@
     TestCase,
 )
 from charmworld.utils import (
+    get_ini,
     LAST_INGEST_JOB_FINISHED,
     quote_key,
 )
@@ -183,10 +185,41 @@
         charm_data = factory.get_charm_json(revision=1)
         update_handler = self.get_handler('charm.update')
         with charm_update_environment(charm_data, self.use_index_client()):
-            run_job(UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
+            with patch('charmworld.jobs.ingest.trigger_tests'):
+                run_job(
+                    UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
         logs = ''.join(r.getMessage() for r in update_handler.buffer)
         self.assertEqual('Saving %s' % charm_data['_id'], logs)
 
+    def test_trigger_tests_builds_correct_url(self):
+        with patch('requests.get') as patched_get:
+            trigger_tests('lp:foo', 1)
+        settings = get_ini()
+        charm_test_url = settings['charm_test_url']
+        charm_test_token = settings['charm_test_token']
+        charm_test_job = settings['charm_test_job']
+        expected_url = ('http://%s/job/%s/buildWithParameters?'
+                        'token=%s&branch=lp:foo&revno=1') % (
+                            charm_test_url, charm_test_token, charm_test_job)
+        patched_get.assert_called_once_with(expected_url)
+
+    def test_run_does_not_trigger_tests_without_new_revision(self):
+        charm_data = factory.get_charm_json(revision=2)
+        self.db.charms.save(charm_data)
+        with charm_update_environment(charm_data, self.use_index_client()):
+            with patch('charmworld.jobs.ingest.trigger_tests') as trigger:
+                run_job(
+                    UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
+        self.assertEqual(0, trigger.call_count)
+
+    def test_run_triggers_tests_with_new_revision(self):
+        charm_data = factory.get_charm_json(revision=1)
+        with charm_update_environment(charm_data, self.use_index_client()):
+            with patch('charmworld.jobs.ingest.trigger_tests') as trigger:
+                run_job(
+                    UpdateCharmJob(), self.payload(charm_data, 2), db=self.db)
+        trigger.assert_called_once_with(charm_data['branch_spec'], 2)
+
     def test_saves_early_error(self):
         charm_data = factory.get_charm_json()
         # Induce improbable error -- bname must be a string.
@@ -194,7 +227,8 @@
         update_handler = self.get_handler('charm.update')
         update_charm_handler = self.get_handler('charm.update_charm')
         with patch('charmworld.jobs.ingest.update_from_store'):
-            run_job(UpdateCharmJob(), self.payload(charm_data), db=self.db)
+            with patch('charmworld.jobs.ingest.trigger_tests'):
+                run_job(UpdateCharmJob(), self.payload(charm_data), db=self.db)
         self.assertIsNotNone(self.db.charms.find_one(charm_data['_id']))
         logs = ''.join(r.getMessage() for r in update_handler.buffer)
         update_charm_logs = ''.join(r.getMessage()
 
=== modified file 'default.ini'
--- default.ini	2014-01-07 16:38:00 +0000
+++ default.ini	2014-01-14 14:38:16 +0000
@@ -62,6 +62,12 @@
 # below.
 proof.port = 2464
 
+# Info for triggering new test builds for new charm revisions
+run_test_builds = false
+charm_test_url = 
+charm_test_token = 
+charm_test_job = 
+
 [server:main]
 use = egg:Paste#http
 host = 0.0.0.0
 
=== modified file 'test-setup.ini'
--- test-setup.ini	2013-07-17 21:40:06 +0000
+++ test-setup.ini	2014-01-14 14:38:16 +0000
@@ -20,5 +20,10 @@
 # Datastores
 mongo.database = juju_test
 
+# Set nonsense to check charm test trigger function.
+charm_test_url = http://example.com/
+charm_test_token = 'this-is-not-the-real-token'
+charm_test_job = "aCharmTestingJob"
+
 # This signals that we are running under test.
 is_testing = true