Merge lp:~ursinha/ubuntu-ci-services-itself/ppa-add-private into lp:ubuntu-ci-services-itself

[Ursula Junque]

This branch adds 'private' to PPA, adds ability to get private and public information from launchpad and adds a variable to define we're using private or public ppas.

This branch adds 'private' to PPA. It was supposed to have signing_key_fingerprint, but now it seems all components are allowed to access launchpad so this is not required.

lp_collection now GETs public and private information, and reserve() only gets ppas according to settings.PRIVATE_ONLY.

[Ursula Junque (ursinha)]

I don't like the check on lp_collection because that's lying. It's not fetching private information only, but *also*.

[lp:~ursinha/ubuntu-ci-services-itself/ppa-add-private]

384. By Ursula Junque on 2014-03-13

Merging trunk

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:384
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/405/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/405/rebuild

review: Approve (continuous-integration)

[Andy Doan (doanac)]

45 +def _lp_get(url):

might be better name _lp_get_authenticated

21 - resp = urllib2.urlopen(url)
22 + if settings.PRIVATE_ONLY:
23 + resp = _lp_get(url)
24 + else:
25 + resp = urllib2.urlopen(url)

Is there really any need to distinguish the 2 code paths? Maybe just always call _lp_get there since you already have the logic in the model to differentiate them?

[lp:~ursinha/ubuntu-ci-services-itself/ppa-add-private]

385. By Ursula Junque on 2014-03-13

s/_lp_get/_lp_get_authenticated/

386. By Ursula Junque on 2014-03-13

all ppa-assigner/launchpad GETs are now authenticated, bringing also private information

[Ursula Junque (ursinha)]

> 45 +def _lp_get(url):
>
> might be better name _lp_get_authenticated

r385.

>
> 21 - resp = urllib2.urlopen(url)
> 22 + if settings.PRIVATE_ONLY:
> 23 + resp = _lp_get(url)
> 24 + else:
> 25 + resp = urllib2.urlopen(url)
>
> Is there really any need to distinguish the 2 code paths? Maybe just always
> call _lp_get there since you already have the logic in the model to
> differentiate them?

I think I was only being overcautious about always getting private information (as other methods also use lp_collection), but at this point I think it shouldn't matter really. As I said before I don't even like it, hehe. Fixed on r386.

Do you think these tests are enough? I deployment similar code yesterday and it seemed to me reserve() wasn't respecting the settings.PRIVATE_ONLY variable, and I couldn't find any logs (gunicorn issue, I think). Is there anything else I need to do besides setting that on unit_config? Tests are passing now but maybe I'm not testing this as I should.

[Ursula Junque (ursinha)]

> Do you think these tests are enough? I deployment similar code yesterday and
> it seemed to me reserve() wasn't respecting the settings.PRIVATE_ONLY
> variable, and I couldn't find any logs (gunicorn issue, I think). Is there
> anything else I need to do besides setting that on unit_config? Tests are
> passing now but maybe I'm not testing this as I should.

s/deployment/deployed/

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:386
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/406/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/406/rebuild

review: Approve (continuous-integration)

[Andy Doan (doanac)]

> Do you think these tests are enough? I deployment similar code yesterday and
> it seemed to me reserve() wasn't respecting the settings.PRIVATE_ONLY
> variable, and I couldn't find any logs (gunicorn issue, I think). Is there
> anything else I need to do besides setting that on unit_config? Tests are
> passing now but maybe I'm not testing this as I should.

is it possible you failed to restart django after changing your unit-config and/or settings.py?

139 - qs = PPA.objects.select_for_update().filter(state=PPA.AVAILABLE)
140 + qs = PPA.objects.select_for_update().filter(
141 + state=PPA.AVAILABLE, private=settings.PRIVATE_ONLY)

That looks pretty cut-and-dry to me. I don't think that could do the wrong thing, and:

190 + def testReservePrivateOnly(self):

seems to verify that block

review: Approve

[lp:~ursinha/ubuntu-ci-services-itself/ppa-add-private]

387. By Ursula Junque on 2014-03-13

Forgot this: engine health page should show total of PPAs available for privacy set

388. By Ursula Junque on 2014-03-13

Making tests check if engine health will display only private/public ppas

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:388
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/411/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/411/rebuild

review: Approve (continuous-integration)

[Francis Ginther (fginther)]

Approve

review: Approve

[lp:~ursinha/ubuntu-ci-services-itself/ppa-add-private]

389. By Ursula Junque on 2014-03-13

Adding private ppa info to the ui for now, to help us testing

[Ursula Junque (ursinha)]

I added only the "private ppa only" info to the ui, to help with the confusion enabling private ppas might bring for now.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:389
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/414/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/414/rebuild

review: Approve (continuous-integration)

=== modified file 'juju-deployer/configs/unit_config.yaml.tmpl'
--- juju-deployer/configs/unit_config.yaml.tmpl	2014-03-13 15:28:07 +0000
+++ juju-deployer/configs/unit_config.yaml.tmpl	2014-03-13 23:30:47 +0000
@@ -17,6 +17,8 @@
 
 ppa_pattern: "ci-pool-\\d+"
 
+private_only: False
+
 # for ticket system
 base_image: "http://cloud-images.ubuntu.com/releases/13.10/release-20131015/ubuntu-13.10-server-cloudimg-amd64-disk1.img"
 series: saucy

=== modified file 'ppa-assigner/ppa_assigner/api.py'
--- ppa-assigner/ppa_assigner/api.py	2014-03-12 00:46:19 +0000
+++ ppa-assigner/ppa_assigner/api.py	2014-03-13 23:30:47 +0000
@@ -115,10 +115,14 @@
         status = JSONStatus()
         status.add_true_false('launchpad configured', lp, lp)
 
-        count = PPA.objects.all().count()
+        is_private = settings.PRIVATE_ONLY
+        status.add_true_false('private ppas only', is_private, is_private)
+
+        total_ppas = PPA.objects.filter(private=is_private)
+        count = total_ppas.count()
         status.add_true_false('total ppas', count, count > 0)
 
-        count = PPA.objects.filter(state=PPA.AVAILABLE).count()
+        count = total_ppas.filter(state=PPA.AVAILABLE).count()
         status.add_true_false('available ppas', count, count > 0)
 
         try:

=== modified file 'ppa-assigner/ppa_assigner/launchpad.py'
--- ppa-assigner/ppa_assigner/launchpad.py	2014-03-12 00:46:19 +0000
+++ ppa-assigner/ppa_assigner/launchpad.py	2014-03-13 23:30:47 +0000
@@ -24,7 +24,7 @@
 
 def lp_collection(url):
     '''iterate through each item returned in an lp_collection'''
-    resp = urllib2.urlopen(url)
+    resp = _lp_get_authenticated(url)
     resp = resp.read()
     data = json.loads(resp)
 
@@ -54,12 +54,19 @@
     return {
         'Authorization': ', '.join(values),
         'Accept': 'application/json',
-        'Content-Type': 'application/x-www-form-urlencoded'
     }
 
 
 def _lp_post(url, data):
-    req = urllib2.Request(url, urllib.urlencode(data), _oauth_headers())
+    headers = _oauth_headers()
+    headers['Content-Type'] = 'application/x-www-form-urlencoded'
+    req = urllib2.Request(url, urllib.urlencode(data), headers)
+    return urllib2.urlopen(req)
+
+
+def _lp_get_authenticated(url):
+    '''An authenticated GET to obtain also private information.'''
+    req = urllib2.Request(url, headers=_oauth_headers())
     return urllib2.urlopen(req)
 
 

=== modified file 'ppa-assigner/ppa_assigner/migrations/0001_initial.py'
--- ppa-assigner/ppa_assigner/migrations/0001_initial.py	2014-03-07 22:49:17 +0000
+++ ppa-assigner/ppa_assigner/migrations/0001_initial.py	2014-03-13 23:30:47 +0000
@@ -12,6 +12,7 @@
         db.create_table(u'ppa_assigner_ppa', (
             (u'id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
             ('name', self.gf('django.db.models.fields.CharField')(unique=True, max_length=4096)),
+            ('private', self.gf('django.db.models.fields.BooleanField')(default=False)),
             ('state', self.gf('django.db.models.fields.PositiveIntegerField')(default=0)),
             ('ticket_id', self.gf('django.db.models.fields.PositiveIntegerField')()),
             ('timestamp', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)),
@@ -29,6 +30,7 @@
             'Meta': {'object_name': 'PPA'},
             u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '4096'}),
+            'private': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
             'state': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'}),
             'ticket_id': ('django.db.models.fields.PositiveIntegerField', [], {}),
             'timestamp': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'})

=== modified file 'ppa-assigner/ppa_assigner/models.py'
--- ppa-assigner/ppa_assigner/models.py	2014-03-12 00:46:19 +0000
+++ ppa-assigner/ppa_assigner/models.py	2014-03-13 23:30:47 +0000
@@ -37,6 +37,7 @@
     )
     name = models.CharField(max_length=4096, unique=True)
 
+    private = models.BooleanField(default=False)
     state = models.PositiveIntegerField(choices=STATES, default=AVAILABLE)
     ticket_id = models.PositiveIntegerField()
     timestamp = models.DateTimeField(auto_now_add=True)
@@ -58,22 +59,25 @@
         # note: LP will throttle calls to list PPAs. So you can occassionally
         # add a PPA, and not see it update for the first few calls to this
         # method, but it will eventually get in sync
-        model_ppas = set([x.name for x in PPA.objects.all()])
-        add = lp_ppas - model_ppas
-        remove = model_ppas - lp_ppas
+        lp_ppas_ = set(lp_ppas.keys())
+        model_ppas = set([ppa.name for ppa in PPA.objects.all()])
+        add = lp_ppas_ - model_ppas
+        remove = model_ppas - lp_ppas_
 
         logging.info('adding new ppas: %r', add)
-        for x in add:
-            PPA.objects.create(name=x, ticket_id=0)
+        for ppa_name in add:
+            lp_ppa = lp_ppas[ppa_name]
+            PPA.objects.create(name=ppa_name, ticket_id=0,
+                               private=lp_ppa['private'])
 
         logging.info('removing ppas: %r', remove)
-        for x in remove:
-            ppa = PPA.objects.get(name=x)
+        for ppa_name in remove:
+            ppa = PPA.objects.get(name=ppa_name)
             if ppa.state == PPA.RESERVED:
                 # log something, but we may as well delete. because if the
                 # ppa doesn't exist in LP we are already in a catastrophic
                 # situation and shouldn't let any other user re-use this
-                logging.error('ppa %s appears to be in use', x)
+                logging.error('ppa %s appears to be in use', ppa_name)
             ppa.delete()
 
     @staticmethod
@@ -83,11 +87,16 @@
         url = url % settings.LAUNCHPAD_PPA_OWNER
         logging.info('populating ppa model from: %s', url)
         try:
+            ppas = {}
             pat = re.compile(settings.PPA_PATTERN)
-            ppas = ['ppa:%s/%s' % (settings.LAUNCHPAD_PPA_OWNER, x['name'])
-                    for x in launchpad.lp_collection(url)
-                    if pat.match(x['name'])]
-            PPA._merge_ppas(set(ppas))
+            for ppa in launchpad.lp_collection(url):
+                if pat.match(ppa['name']):
+                    ppa_name = 'ppa:{}/{}'.format(settings.LAUNCHPAD_PPA_OWNER,
+                                                  ppa['name'])
+                    ppas[ppa_name] = {
+                        'private': ppa['private'],
+                    }
+            PPA._merge_ppas(ppas)
         except urllib2.URLError:
             logging.exception('Could not populate PPAs from launchpad')
             raise
@@ -97,7 +106,8 @@
         # "select_for_update" provides proper db-level locking so we
         # don't accidentally have 2 concurrent requests find and reserve the
         # same PPA
-        qs = PPA.objects.select_for_update().filter(state=PPA.AVAILABLE)
+        qs = PPA.objects.select_for_update().filter(
+            state=PPA.AVAILABLE, private=settings.PRIVATE_ONLY)
         if qs.count() == 0:
             raise PPA.DoesNotExist('No free PPAs exist in pool')
         pk = qs[0].pk

=== modified file 'ppa-assigner/ppa_assigner/settings.py'
--- ppa-assigner/ppa_assigner/settings.py	2014-03-12 01:35:01 +0000
+++ ppa-assigner/ppa_assigner/settings.py	2014-03-13 23:30:47 +0000
@@ -51,6 +51,7 @@
 OAUTH_TOKEN_SECRET = _cfg.get('oauth_token_secret', None)
 OAUTH_REALM = _cfg.get('oauth_realm', 'https://api.launchpad.net/')
 PPA_PATTERN = _cfg.get('ppa_pattern', r'ci-pool-\d+')
+PRIVATE_ONLY = _cfg.get('private_only', False)
 
 DATABASES = {
     'default': {

=== modified file 'ppa-assigner/ppa_assigner/tests.py'
--- ppa-assigner/ppa_assigner/tests.py	2014-03-12 00:46:19 +0000
+++ ppa-assigner/ppa_assigner/tests.py	2014-03-13 23:30:47 +0000
@@ -39,6 +39,7 @@
 
     def setUp(self):
         super(TestModel, self).setUp()
+        settings.PRIVATE_ONLY = False
         for n in range(5):
             name = 'ci_pool-%s' % str(n).zfill(3)
             PPA.objects.create(name=name, ticket_id=0)
@@ -57,6 +58,16 @@
         with self.assertRaises(PPA.DoesNotExist):
             PPA.reserve(1234)
 
+    def testReservePrivateOnly(self):
+        PPA.objects.create(name='ci_pool-998', ticket_id=0)
+        settings.PRIVATE_ONLY = True
+        with self.assertRaises(PPA.DoesNotExist):
+            PPA.reserve(1234)
+        PPA.objects.create(name='ci_pool-999', ticket_id=0, private=True)
+        p = PPA.reserve(1234)
+        self.assertEquals(p.ticket_id, 1234)
+        self.assertEquals(p.private, True)
+
 
 class TestConcurrency(TransactionTestCase):
     # inspired by, but heavily modified:
@@ -146,6 +157,19 @@
         obj = self.createResource('ppa/', params)
         self.assertEqual(obj['ticket_id'], params['ticket_id'])
 
+    def testReservePrivateOnly(self):
+        PPA.objects.create(name='ci-pool_996', ticket_id=0)
+        params = {'ticket_id': 1234}
+        # There are only public PPAs available.
+        settings.PRIVATE_ONLY = True
+        with self.assertRaisesRegexp(AssertionError, '404 != 201'):
+            self.post('ppa/', params)
+
+        # Creating one private PPA to be reserved.
+        PPA.objects.create(name='ci-pool_997', ticket_id=0, private=True)
+        obj = self.createResource('ppa/', params)
+        self.assertEqual(obj['ticket_id'], params['ticket_id'])
+
     def testReserveNoFree(self):
         PPA.objects.update(state=PPA.DIRTY)
         params = {'ticket_id': 1234}
@@ -184,12 +208,13 @@
     @mock.patch('urllib2.urlopen')
     def testPopulateMerge(self, urlopen):
         ignored = [
-            {'name': 'foo'},
-            {'name': 'ci-pool'},
+            {'name': 'foo', 'private': False},
+            {'name': 'ci-pool', 'private': False},
         ]
         expected = [
-            {'name': 'ci-pool-001'},
-            {'name': 'ci-pool-002'},
+            {'name': 'ci-pool-001', 'private': False},
+            {'name': 'ci-pool-002', 'private': True},
+            {'name': 'ci-pool-003', 'private': False},
         ]
         data = {
             'total_size': 2,
@@ -201,9 +226,17 @@
         urlopen.return_value = resp
         ppa_owner = settings.LAUNCHPAD_PPA_OWNER
         self.patch('ppa/', {'populate': True})
-        expected = ['ppa:%s/%s' % (ppa_owner, x['name']) for x in expected]
-        found = [x.name for x in PPA.objects.all().order_by('name')]
-        self.assertListEqual(expected, found)
+        expected = dict(('ppa:{}/{}'.format(ppa_owner, x['name']),
+                         x['private']) for x in expected)
+        found = dict((x.name, x.private) for x in PPA.objects.all())
+        self.assertEqual(len(found), len(expected))
+        self.assertListEqual(sorted(expected.keys()), sorted(found.keys()))
+        for ppa_name, ppa_privacy in found.iteritems():
+            self.assertEqual(expected[ppa_name], ppa_privacy)
+        private_ppa = PPA.objects.filter(private=True)
+        self.assertEqual(private_ppa.count(), 1)
+        self.assertEqual(PPA.objects.filter(private=False).count(), 2)
+        self.assertEqual(PPA.objects.all().count(), 3)
 
     def testCopyBadParams(self):
         resource = self._resource('ppa_copy/')
@@ -227,12 +260,15 @@
 
     def testStatus(self):
         status = self.get('status/')
-        total = PPA.objects.all().count()
-        avail = PPA.objects.filter(state=PPA.AVAILABLE).count()
-        self.assertEqual('total ppas', status[1]['label'])
-        self.assertEqual(total, status[1]['value'])
-        self.assertEqual('available ppas', status[2]['label'])
-        self.assertEqual(avail, status[2]['value'])
+        total_ppas = PPA.objects.filter(private=settings.PRIVATE_ONLY)
+        total = total_ppas.count()
+        avail = total_ppas.filter(state=PPA.AVAILABLE).count()
+        self.assertEqual('private ppas only', status[1]['label'])
+        self.assertEqual(True, status[1]['value'])
+        self.assertEqual('total ppas', status[2]['label'])
+        self.assertEqual(total, status[2]['value'])
+        self.assertEqual('available ppas', status[3]['label'])
+        self.assertEqual(avail, status[3]['value'])
 
 
 class TestCleaner(TestCase):