Merge lp:~cprov/uci-engine/ts-artifact-constraints into lp:uci-engine

[Celso Providelo]

Fixes TS to prevent orphan artifacts to be stored and the misleading error messages related with processing empty subtickets.

Fixes TS to prevent orphan artifacts to be stored and the misleading error messages related with processing empty subtickets.

Fixes SubTicketArtifact resource in TS, removing a stray (landed by mistake with my ticket-conflicts MP) 'readonly' attribute for 'subticket' FK which leads to empty (no SPU artifacts) subtickets. The underlying problem, in fact, is that artifact FKs to Ticket and SubTicket were nullable for no reason. This MP makes them NOT NULL, since it makes no sense to have a subticket-artifact for no subticket ...

CLI doesn't identify this problem because the TS request succeeds, although subticket.artifacts is empty by the time the ticket is queued. We theoretically could extend CLI checks ...

A further problem is that BSB intermediary steps work, despite of the fact they operate on an empty filelist, up to the point it tries to re-sign the upload(s).

Re-signing upload was failing, just because it has valid keys, but was returning very poor tracing (bare exception on handle_request). This is fixed in this MP too.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Ursula Junque (ursinha)]

I have only one note, that we discussed in a hangout:

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Ursula Junque (ursinha)]

Thanks!

review: Approve

[Ubuntu CI Bot (uci-bot)]

The attempt to merge lp:~cprov/uci-engine/ts-artifact-constraints into lp:uci-engine failed. Below is the output from the failed tests.

ERROR:root:/var/lib/lxc not mounted.

=== modified file 'branch-source-builder/bsbuilder/run_worker.py'
--- branch-source-builder/bsbuilder/run_worker.py	2014-09-12 13:46:47 +0000
+++ branch-source-builder/bsbuilder/run_worker.py	2014-10-06 18:30:44 +0000
@@ -101,9 +101,10 @@
         # Check that an upload isn't going to fail due to version
         (failed, subticket_status) = watch_ppa.check_ppa(
             series, ppa, archive, None, upload_list)
+        out_data['subticket_status'] = subticket_status
+
         if failed:
             # This is the end of the line, the upload is blocked.
-            out_data['subticket_status'] = subticket_status
             message = 'Source package upload failed.'
             log.error(message)
             out_data['message'] = message
@@ -132,7 +133,12 @@
             # In this sense it's not fair to *FAIL* the submitted ticket.
             # We should find a way to retry the job in another (possibly
             # saner) instance.
-            return amqp_utils.progress_failed, str(exc)
+            message = str(exc)
+            log.error(message)
+            out_data['message'] = message
+            out_data['artifacts'].append(
+                create_result_file(datastore, out_data))
+            return amqp_utils.progress_failed, out_data
 
         metric = stats.Metric(
             'source_package_upload',

=== modified file 'branch-source-builder/bsbuilder/tests/test_worker.py'
--- branch-source-builder/bsbuilder/tests/test_worker.py	2014-09-12 05:18:35 +0000
+++ branch-source-builder/bsbuilder/tests/test_worker.py	2014-10-06 18:30:44 +0000
@@ -192,3 +192,65 @@
         self.assertEqual(mocked_progress_completed, cb)
         self.assertSigned('foobar_0.1-1_source.changes')
         self.assertSigned('foobar_0.1-1.dsc')
+
+    @mock.patch('bsbuilder.upload_package.upload_source_packages')
+    @mock.patch('bsbuilder.upload_package.create_upload_list')
+    @mock.patch('bsbuilder.upload_package.get_signing_keyring')
+    @mock.patch('bsbuilder.upload_package.resign_upload')
+    @mock.patch('ci_utils.amqp_utils.progress_failed')
+    def test_handle_request_resigning_fails_gracefully(
+            self, mocked_progress_failed, mocked_keyring, mocked_resign,
+            mocked_upload_list, mocked_upload):
+        # `BSBuilderWorker.handle_request` fails gracefully if upload
+        # re-signing cannot be executed.
+
+        # Use a fake upload_list corresponding to the copied testing files.
+        mocked_upload_list.return_value = [{
+            'name': 'foobar',
+            'version': '1.0-1',
+            'location': self.tmpdir,
+            'files': [
+                'foobar_0.1-1_source.changes',
+                'foobar_0.1-1.dsc',
+                'foobar_0.1.orig.tar.gz',
+                'foobar_0.1-1.debian.tar.gz'
+            ],
+        }]
+
+        # Creating successful watch_ppa mocks.
+        self.check_ppa.return_value = (False, self.fake_subticket_status)
+        self.watch_ppa.return_value = (0, self.fake_subticket_status)
+
+        # Use the testing keyring for signing.
+        mocked_keyring.return_value = (
+            TESTING_PRIVATE_KEYRING_DATA,
+            {'uids': ['A_NAME'], 'fingerprint': 'A_FPR'},
+        )
+
+        # Fail when re-signing uploads.
+        from bsbuilder import upload_package
+        mocked_resign.side_effect = [
+            upload_package.UploadResigningError('I am dead, sorry!')
+        ]
+
+        cb, result = self.worker.handle_request(
+            self.fake_request_params, self.log)
+
+        # BSB fails gracefully, returning tracing information (dict) and
+        # complete logging stored in swift.
+        self.assertEqual(mocked_progress_failed, cb)
+        self.assertEqual(
+            {'archive': 'an_archive',
+             'artifacts': [
+                 {'name': 'package_build.output.log',
+                  'reference': 'swift_ref',
+                  'type': 'LOGS'}
+             ],
+             'message': 'I am dead, sorry!',
+             'ppa': 'a_ppa',
+             'subticket_status': [
+                 {'message': 'SUB_MESSAGE',
+                  'name': 'the_source',
+                  'status_text': 'A_STATUS',
+                  'version': '1.0'}
+             ]}, result)

=== modified file 'juju-deployer/branch-source-builder.yaml.tmpl'
--- juju-deployer/branch-source-builder.yaml.tmpl	2014-10-01 11:39:57 +0000
+++ juju-deployer/branch-source-builder.yaml.tmpl	2014-10-06 18:30:44 +0000
@@ -9,7 +9,7 @@
                 current_code: ${CI_PAYLOAD_URL}
                 available_code: ${CI_PAYLOAD_URL}
                 unit-config: include-base64://configs/unit_config.yaml
-                packages: "debhelper devscripts dput python-dput python-swiftclient lazr.enum python-txstatsd python-gnupg python-kombu"
+                packages: "debhelper devscripts dput python-dput python-swiftclient lazr.enum python-txstatsd python-gnupg python-kombu python-mock"
                 install_sources: |
                     - ${CI_PPA}
                     - "cloud:precise-icehouse"

=== modified file 'ticket_system/ticket/api.py'
--- ticket_system/ticket/api.py	2014-10-02 02:46:09 +0000
+++ ticket_system/ticket/api.py	2014-10-06 18:30:44 +0000
@@ -265,7 +265,8 @@
 
 
 class TicketArtifactResource(ModelResource):
-    ticket = fields.ToOneField(TicketResource, 'ticket', null=True, full=True)
+    ticket = fields.ToOneField(
+        TicketResource, 'ticket', full=True)
 
     class Meta:
         queryset = TicketArtifact.objects.all()
@@ -279,8 +280,8 @@
 
 class SubTicketArtifactResource(ModelResource):
 
-    subticket = fields.ToOneField(SubTicketResource, 'subticket',
-                                  readonly=True, null=True, full=True)
+    subticket = fields.ToOneField(
+        SubTicketResource, 'subticket', full=True)
 
     class Meta:
         queryset = SubTicketArtifact.objects.all()

=== added file 'ticket_system/ticket/migrations/0014_auto__chg_field_ticketartifact_ticket__chg_field_subticketartifact_sub.py'
--- ticket_system/ticket/migrations/0014_auto__chg_field_ticketartifact_ticket__chg_field_subticketartifact_sub.py	1970-01-01 00:00:00 +0000
+++ ticket_system/ticket/migrations/0014_auto__chg_field_ticketartifact_ticket__chg_field_subticketartifact_sub.py	2014-10-06 18:30:44 +0000
@@ -0,0 +1,120 @@
+# -*- coding: utf-8 -*-
+import datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+
+        # Changing field 'TicketArtifact.ticket'
+        db.alter_column('ticket_artifact', 'ticket_id', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['ticket.Ticket'], null=False))
+
+        # Changing field 'SubTicketArtifact.subticket'
+        db.alter_column('subticket_artifact', 'subticket_id', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['ticket.SubTicket'], null=False))
+
+    def backwards(self, orm):
+
+        # Changing field 'TicketArtifact.ticket'
+        db.alter_column('ticket_artifact', 'ticket_id', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['ticket.Ticket'], null=True))
+
+        # Changing field 'SubTicketArtifact.subticket'
+        db.alter_column('subticket_artifact', 'subticket_id', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['ticket.SubTicket'], null=True))
+
+    models = {
+        u'project.sourcepackage': {
+            'Meta': {'object_name': 'SourcePackage', 'db_table': "'sourcepackage'"},
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '4096'})
+        },
+        u'ticket.mergeproposal': {
+            'Meta': {'object_name': 'MergeProposal', 'db_table': "'mergeproposal'"},
+            'approved_revno': ('django.db.models.fields.IntegerField', [], {}),
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'lp_url': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'project': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'sourcepackage': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['project.SourcePackage']"}),
+            'target_branch': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'version': ('django.db.models.fields.CharField', [], {'max_length': '4096'})
+        },
+        u'ticket.review': {
+            'Meta': {'object_name': 'Review'},
+            'completed': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'review_type': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'ticket': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['ticket.Ticket']"}),
+            'workflow_step': ('django.db.models.fields.IntegerField', [], {})
+        },
+        u'ticket.sourcepackageupload': {
+            'Meta': {'object_name': 'SourcePackageUpload', 'db_table': "'sourcepackageupload'"},
+            'architecture': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'sourcepackage': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['project.SourcePackage']"}),
+            'version': ('django.db.models.fields.CharField', [], {'max_length': '4096'})
+        },
+        u'ticket.subticket': {
+            'Meta': {'ordering': "['id']", 'object_name': 'SubTicket', 'db_table': "'subticket'"},
+            'assignee': ('django.db.models.fields.EmailField', [], {'max_length': '254'}),
+            'current_workflow_step': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '4096'}),
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'merge_proposal': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': u"orm['ticket.MergeProposal']", 'null': 'True', 'blank': 'True'}),
+            'source_package_upload': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': u"orm['ticket.SourcePackageUpload']", 'null': 'True', 'blank': 'True'}),
+            'status': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '4096'}),
+            'ticket': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['ticket.Ticket']"})
+        },
+        u'ticket.subticketartifact': {
+            'Meta': {'object_name': 'SubTicketArtifact', 'db_table': "'subticket_artifact'"},
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'reference': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'subticket': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['ticket.SubTicket']"}),
+            'type': ('django.db.models.fields.CharField', [], {'max_length': '4096'})
+        },
+        u'ticket.ticket': {
+            'Meta': {'ordering': "['id']", 'object_name': 'Ticket', 'db_table': "'ticket'"},
+            'base_image': ('django.db.models.fields.CharField', [], {'default': "'http://cloud-images.ubuntu.com/utopic/current/utopic-server-cloudimg-amd64-disk1.img'", 'max_length': '4096'}),
+            'bug_id': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'blank': 'True'}),
+            'build_ppa': ('django.db.models.fields.CharField', [], {'max_length': '4096', 'blank': 'True'}),
+            'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}),
+            'creator': ('django.db.models.fields.EmailField', [], {'max_length': '254', 'null': 'True', 'blank': 'True'}),
+            'current_workflow_step': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '4096'}),
+            'description': ('django.db.models.fields.TextField', [], {}),
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'lander_unit': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
+            'master_ppa': ('django.db.models.fields.CharField', [], {'default': "'ppa:cprov/ci-master'", 'max_length': '4096'}),
+            'owner': ('django.db.models.fields.EmailField', [], {'max_length': '254'}),
+            'private': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'series': ('django.db.models.fields.CharField', [], {'default': "'utopic'", 'max_length': '4096'}),
+            'status': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '4096'}),
+            'title': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'updated': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}),
+            'uuid': ('django.db.models.fields.CharField', [], {'default': "'785afdfa-4c2a-11e4-9446-001c4229f9ac'", 'unique': 'True', 'max_length': '36'}),
+            'workflow': ('django.db.models.fields.related.ForeignKey', [], {'default': "'default'", 'to': u"orm['ticket.Workflow']"})
+        },
+        u'ticket.ticketartifact': {
+            'Meta': {'object_name': 'TicketArtifact', 'db_table': "'ticket_artifact'"},
+            u'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'reference': ('django.db.models.fields.CharField', [], {'max_length': '4096'}),
+            'ticket': ('django.db.models.fields.related.ForeignKey', [], {'to': u"orm['ticket.Ticket']"}),
+            'type': ('django.db.models.fields.CharField', [], {'max_length': '4096'})
+        },
+        u'ticket.ticketcitrainoverlay': {
+            'Meta': {'object_name': 'TicketCiTrainOverlay', 'db_table': "'ticketcitrainoverlay'"},
+            'comments': ('django.db.models.fields.TextField', [], {}),
+            'job_url': ('django.db.models.fields.URLField', [], {'max_length': '200'}),
+            'landers': ('django.db.models.fields.TextField', [], {}),
+            'sync_request': ('django.db.models.fields.CharField', [], {'max_length': '256'}),
+            'test_notes': ('django.db.models.fields.TextField', [], {}),
+            'ticket': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'citrain_overlay'", 'unique': 'True', 'primary_key': 'True', 'to': u"orm['ticket.Ticket']"})
+        },
+        u'ticket.workflow': {
+            'Meta': {'object_name': 'Workflow'},
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '4096', 'primary_key': 'True'}),
+            'steps': ('django.db.models.fields.CharField', [], {'max_length': '4096'})
+        }
+    }
+
+    complete_apps = ['ticket']

=== modified file 'ticket_system/ticket/models.py'
--- ticket_system/ticket/models.py	2014-10-02 20:04:57 +0000
+++ ticket_system/ticket/models.py	2014-10-06 18:30:44 +0000
@@ -351,7 +351,7 @@
         db_table = 'subticket_artifact'
 
     type = models.CharField(choices=SUBTICKET_ARTIFACT_TYPE, max_length=4096)
-    subticket = models.ForeignKey(SubTicket, null=True)
+    subticket = models.ForeignKey(SubTicket)
 
     def __unicode__(self):
         return "{} {}".format(self.name, self.type)
@@ -368,7 +368,7 @@
         db_table = 'ticket_artifact'
 
     type = models.CharField(choices=TICKET_ARTIFACT_TYPE, max_length=4096)
-    ticket = models.ForeignKey(Ticket, null=True)
+    ticket = models.ForeignKey(Ticket)
 
     def __unicode__(self):
         return "{} {}".format(self.name, self.type)

=== modified file 'ticket_system/ticket/tests/test_write_api.py'
--- ticket_system/ticket/tests/test_write_api.py	2014-10-02 20:04:57 +0000
+++ ticket_system/ticket/tests/test_write_api.py	2014-10-06 18:30:44 +0000
@@ -32,7 +32,6 @@
     Review,
     SourcePackageUpload,
     SubTicket,
-    SubTicketArtifact,
     SubTicketWorkflowStep,
     SubTicketWorkflowStepStatus,
     Ticket,
@@ -357,42 +356,35 @@
         self.subticket = mommy.make('SubTicket', ticket=self.ticket,
                                     source_package_upload=self.spu)
         self.artifact = mommy.make('SubTicketArtifact',
+                                   name='existing_artifact',
                                    subticket=self.subticket)
         self.resource = 'subticketartifact/'
         self.detail_url = self.resource + '{0}/'.format(self.artifact.pk)
         self.subticket_uri = '/api/v1/subticket/{0}/'.format(self.subticket.pk)
         self.post_artifact_data_subticket = {
             'type': 'SPU',
-            'name': 'my_artifact',
+            'name': 'new_artifact',
             'subticket': self.subticket_uri,
             'reference': 'http://www.example.com/m1CTLg5FHY',
         }
 
     def test_post_artifact_subticket(self):
-        """
-        Test creating an artifact for a subticket via the API
-        """
-        # Check how many are there first.
-        self.assertEqual(SubTicketArtifact.objects.count(), 1)
+        # `SubTicketArtifact` resource can be created via the API.
         self.post(resource=self.resource,
                   params=self.post_artifact_data_subticket)
-        # Verify a new one has been added.
-        self.assertEqual(SubTicketArtifact.objects.count(), 2)
-
-    def test_patch_detail(self):
-        # Grab the current data & modify it slightly.
-        original_data = self.getResource(self.detail_url)
-        new_data = original_data.copy()
-        new_data['name'] = 'my_upload'
-
-        self.assertEqual(SubTicketArtifact.objects.count(), 1)
-        resp = self.client.patch('/api/v1/' + self.detail_url, data=new_data)
+        # Retrieve `SubTicketArtifact`s via their context `SubTicket`,
+        # this way we can be sure they are properly related.
+        [existing, new] = list(self.subticket.subticketartifact_set.all())
+        self.assertEqual(existing, self.artifact)
+        self.assertEqual('SPU', new.type)
+        self.assertEqual('new_artifact', new.name)
+        self.assertEqual('http://www.example.com/m1CTLg5FHY', new.reference)
+
+    def test_patch_artifact_not_allowed(self):
+        # `SubTicketArtifact` resource cannot be patched via the API.
+        resp = self.client.patch(
+            '/api/v1/' + self.detail_url, data={'name': 'modified_artifact'})
         self.assertHttpMethodNotAllowed(resp)
-        # Make sure the count hasn't changed & we did an update.
-        self.assertEqual(SubTicketArtifact.objects.count(), 1)
-        # Check for updated data.
-        self.assertEqual(SubTicketArtifact.objects.get(
-                         pk=self.artifact.pk).name, self.artifact.name)
 
 
 class APIUpdateTicketResourceTest(TicketTastypieTestCase):