Merge lp:~cjwatson/launchpad/uefi-ppa-no-unapproved into lp:launchpad

[Colin Watson]

Always auto-approve buildd uploads to PPAs, even if they contain UEFI custom files.

== Summary ==

UEFI custom uploads to PPAs land in the UNAPPROVED queue, even though PPAs aren't supposed to have an approval workflow and have no good way to handle the approval. (See bug 1036599 for a more complete exploration of the rationale.)

== Proposed fix ==

Ensure that BuildDaemonUploadPolicy.autoApprove always returns True for uploads to PPAs.

== Implementation details ==

I turned uploadpolicy.txt into unit tests to recover LoC.

== Tests ==

bin/test -vvc lp.archiveuploader.tests.test_uploadpolicy

== Demo and Q/A ==

Upload a new revision of efilinux to a dogfood PPA (based on what's currently in quantal there), wait for it to build, and check that it is automatically accepted by process-upload rather than landing in the UNAPPROVED queue.

[Steve Kowalik (stevenk)]

Looks good to me. And I approve of the -5. :-)

review: Approve (code)

=== modified file 'lib/lp/archiveuploader/tests/test_uploadpolicy.py'
--- lib/lp/archiveuploader/tests/test_uploadpolicy.py	2012-01-01 02:58:52 +0000
+++ lib/lp/archiveuploader/tests/test_uploadpolicy.py	2012-08-14 14:08:16 +0000
@@ -1,20 +1,64 @@
 #!/usr/bin/python
 #
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from zope.component import getUtility
+from zope.component.interfaces import ComponentLookupError
+
 from lp.app.errors import NotFoundError
+from lp.archiveuploader.nascentuploadfile import CustomUploadFile
 from lp.archiveuploader.uploadpolicy import (
     AbstractUploadPolicy,
     ArchiveUploadType,
+    findPolicyByName,
+    IArchiveUploadPolicy,
+    InsecureUploadPolicy,
     )
+from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.series import SeriesStatus
+from lp.services.database.sqlbase import flush_database_updates
 from lp.testing import (
+    celebrity_logged_in,
     TestCase,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
+class FakeNascentUpload:
+
+    def __init__(self, sourceful, binaryful, is_ppa):
+        self.sourceful = sourceful
+        self.binaryful = binaryful
+        self.is_ppa = is_ppa
+        self.rejections = []
+
+    def reject(self, msg):
+        self.rejections.append(msg)
+
+
+def make_fake_upload(sourceful=False, binaryful=False, is_ppa=False):
+    return FakeNascentUpload(sourceful, binaryful, is_ppa)
+
+
+def make_policy(accepted_type):
+    policy = AbstractUploadPolicy()
+    policy.accepted_type = accepted_type
+    return policy
+
+
+class FakeOptions:
+    def __init__(self, distroseries=None):
+        self.distro = "ubuntu"
+        self.distroseries = distroseries
+
+
+class FakeChangesFile:
+    def __init__(self, custom_files=[]):
+        self.custom_files = custom_files
+
+
 class TestUploadPolicy_validateUploadType(TestCase):
     """Test what kind (sourceful/binaryful/mixed) of uploads are accepted."""
 
@@ -99,6 +143,49 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_getUtility_returns_class(self):
+        # Since upload policies need to be changed according to
+        # user-specified arguments, the utility for looking up policies
+        # returns the class itself rather than an instance of it.
+        policy_cls = getUtility(IArchiveUploadPolicy, "insecure")
+        self.assertIs(InsecureUploadPolicy, policy_cls)
+
+    def test_findPolicyByName_returns_instance(self):
+        # There is a helper function that returns an instance of the policy
+        # with the given name.  It is preferred over using getUtility()
+        # directly.
+        insecure_policy = findPolicyByName("insecure")
+        self.assertIsInstance(insecure_policy, InsecureUploadPolicy)
+
+    def test_policy_names(self):
+        self.assertEqual("insecure", findPolicyByName("insecure").name)
+        self.assertEqual("buildd", findPolicyByName("buildd").name)
+
+    def test_cannot_look_up_abstract_policy(self):
+        self.assertRaises(ComponentLookupError, findPolicyByName, "abstract")
+
+    def test_policy_attributes(self):
+        # Some attributes are expected to exist but vary between policies.
+        insecure_policy = findPolicyByName("insecure")
+        buildd_policy = findPolicyByName("buildd")
+        self.assertFalse(insecure_policy.unsigned_changes_ok)
+        self.assertTrue(buildd_policy.unsigned_changes_ok)
+        self.assertFalse(insecure_policy.unsigned_dsc_ok)
+        self.assertTrue(buildd_policy.unsigned_dsc_ok)
+
+    def test_setOptions_distro_name(self):
+        # Policies pick up the distribution name from options.
+        for policy_name in "insecure", "buildd":
+            policy = findPolicyByName(policy_name)
+            policy.setOptions(FakeOptions())
+            self.assertEqual("ubuntu", policy.distro.name)
+
+    def test_setOptions_distroseries_name(self):
+        # If a distroseries name is set, the policy picks it up from option.
+        buildd_policy = findPolicyByName("buildd")
+        buildd_policy.setOptions(FakeOptions(distroseries="hoary"))
+        self.assertEqual("hoary", buildd_policy.distroseries.name)
+
     def test_setDistroSeriesAndPocket_distro_not_found(self):
         policy = AbstractUploadPolicy()
         policy.distro = self.factory.makeDistribution()
@@ -106,24 +193,79 @@
             NotFoundError, policy.setDistroSeriesAndPocket,
             'nonexistent_security')
 
-
-class FakeNascentUpload:
-
-    def __init__(self, sourceful, binaryful):
-        self.sourceful = sourceful
-        self.binaryful = binaryful
-        self.is_ppa = False
-        self.rejections = []
-
-    def reject(self, msg):
-        self.rejections.append(msg)
-
-
-def make_fake_upload(sourceful=False, binaryful=False):
-    return FakeNascentUpload(sourceful, binaryful)
-
-
-def make_policy(accepted_type):
-    policy = AbstractUploadPolicy()
-    policy.accepted_type = accepted_type
-    return policy
+    def setHoaryStatus(self, status):
+        ubuntu = getUtility(IDistributionSet)["ubuntu"]
+        with celebrity_logged_in("admin"):
+            ubuntu["hoary"].status = status
+        flush_database_updates()
+
+    def test_insecure_approves_release(self):
+        # Uploads to the RELEASE pocket of non-FROZEN distroseries are
+        # approved by the insecure policy.
+        insecure_policy = findPolicyByName("insecure")
+        insecure_policy.setOptions(FakeOptions(distroseries="hoary"))
+        self.assertEqual(
+            SeriesStatus.DEVELOPMENT, insecure_policy.distroseries.status)
+        self.assertTrue(insecure_policy.autoApprove(make_fake_upload()))
+        self.assertTrue(insecure_policy.autoApprove(
+            make_fake_upload(is_ppa=True)))
+
+    def test_insecure_approves_proposed(self):
+        # Uploads to the PROPOSED pocket of non-FROZEN distroseries are
+        # approved by the insecure policy.
+        insecure_policy = findPolicyByName("insecure")
+        insecure_policy.setOptions(FakeOptions(distroseries="hoary-proposed"))
+        self.assertEqual(
+            SeriesStatus.DEVELOPMENT, insecure_policy.distroseries.status)
+        self.assertTrue(insecure_policy.autoApprove(make_fake_upload()))
+
+    def test_insecure_does_not_approve_proposed_post_release(self):
+        # Uploads to the PROPOSED pocket are not auto-approved after
+        # release.
+        self.setHoaryStatus(SeriesStatus.CURRENT)
+        insecure_policy = findPolicyByName("insecure")
+        insecure_policy.setOptions(FakeOptions(distroseries="hoary-proposed"))
+        self.assertFalse(insecure_policy.autoApprove(make_fake_upload()))
+
+    def test_insecure_does_not_approve_frozen(self):
+        # When the distroseries is FROZEN, uploads to the primary archive
+        # wait in the UNAPPROVED queue, but PPA uploads are still approved.
+        self.setHoaryStatus(SeriesStatus.FROZEN)
+        insecure_policy = findPolicyByName("insecure")
+        insecure_policy.setOptions(FakeOptions(distroseries="hoary-proposed"))
+        self.assertFalse(insecure_policy.autoApprove(make_fake_upload()))
+        self.assertTrue(insecure_policy.autoApprove(
+            make_fake_upload(is_ppa=True)))
+
+    def test_insecure_does_not_approve_updates(self):
+        # Uploads to the UPDATES pocket are not auto-approved by the
+        # insecure policy.  Despite not being allowed yet (see
+        # UploadPolicy.checkUpload), PPA uploads to post-release pockets
+        # would still be auto-approved.
+        self.setHoaryStatus(SeriesStatus.CURRENT)
+        insecure_policy = findPolicyByName("insecure")
+        insecure_policy.setOptions(FakeOptions(distroseries="hoary-updates"))
+        self.assertFalse(insecure_policy.autoApprove(make_fake_upload()))
+        self.assertTrue(insecure_policy.autoApprove(
+            make_fake_upload(is_ppa=True)))
+
+    def test_buildd_does_not_approve_uefi(self):
+        # Uploads to the primary archive containing UEFI custom files are
+        # not approved.
+        buildd_policy = findPolicyByName("buildd")
+        uploadfile = CustomUploadFile(
+            "uefi.tar.gz", None, 0, "main/raw-uefi", "extra", buildd_policy,
+            None)
+        upload = make_fake_upload(binaryful=True)
+        upload.changes = FakeChangesFile(custom_files=[uploadfile])
+        self.assertFalse(buildd_policy.autoApprove(upload))
+
+    def test_buildd_approves_uefi_ppa(self):
+        # Uploads to PPAs containing UEFI custom files are auto-approved.
+        buildd_policy = findPolicyByName("buildd")
+        uploadfile = CustomUploadFile(
+            "uefi.tar.gz", None, 0, "main/raw-uefi", "extra", buildd_policy,
+            None)
+        upload = make_fake_upload(binaryful=True, is_ppa=True)
+        upload.changes = FakeChangesFile(custom_files=[uploadfile])
+        self.assertTrue(buildd_policy.autoApprove(upload))

=== removed file 'lib/lp/archiveuploader/tests/uploadpolicy.txt'
--- lib/lp/archiveuploader/tests/uploadpolicy.txt	2012-03-28 16:07:40 +0000
+++ lib/lp/archiveuploader/tests/uploadpolicy.txt	1970-01-01 00:00:00 +0000
@@ -1,149 +0,0 @@
-The uploader policies
----------------------
-
-When the uploader is invoked, it is given a policy to work in. This governs
-such things as what tests get run at what stages of the upload, and whether or
-not there is a build to be created, or one in existence to be used. These
-policies are in the lp.archiveuploader package, in the uploadpolicy module,
-and implement IArchiveUploadPolicy. They are registered as named utilities,
-but since the policy themselves need to be changed according to user-specified
-arguments, we register the classes as the component of the utilities, which
-means a call to getUtility(IArchiveUploadPolicy, name) will return the class
-itself rather than an instance of it.
-
-    >>> from lp.archiveuploader.uploadpolicy import (
-    ...     IArchiveUploadPolicy, findPolicyByName)
-    >>> policy_cls = getUtility(IArchiveUploadPolicy, 'insecure')
-    >>> policy_cls
-    <class '...InsecureUploadPolicy'>
-
-There's a helper function which returns an instance of the policy with the
-given name, though, and it's preferred over using getUtility() directly.
-
-    >>> insecure_policy = findPolicyByName('insecure')
-    >>> insecure_policy
-    <lp...InsecureUploadPolicy object...
-
-Two of the policies defined so far are the insecure and buildd policies.
-
-    >>> insecure_policy.name
-    'insecure'
-    >>> buildd_policy = findPolicyByName('buildd')
-    >>> buildd_policy.name
-    'buildd'
-    >>> abstract_policy = findPolicyByName('abstract')
-    Traceback (most recent call last):
-    ...
-    ComponentLookupError:...
-
-There is a bunch of attributes which we expect to have and which can vary
-from policy to policy.
-
-    >>> insecure_policy.unsigned_changes_ok
-    False
-    >>> buildd_policy.unsigned_changes_ok
-    True
-    >>> insecure_policy.unsigned_dsc_ok
-    False
-    >>> buildd_policy.unsigned_dsc_ok
-    True
-
-The policies require certain values to be present in the options at times...
-
-    >>> class MockAbstractOptions:
-    ...     distro = 'ubuntu'
-    ...     distroseries = None
-    >>> class MockOptions(MockAbstractOptions):
-    ...     builds = True
-
-    >>> ab_opts = MockAbstractOptions()
-    >>> bd_opts = MockOptions()
-
-    >>> insecure_policy.setOptions(ab_opts)
-    >>> insecure_policy.distro.name
-    u'ubuntu'
-    >>> buildd_policy.setOptions(ab_opts)
-    >>> buildd_policy.setOptions(bd_opts)
-    >>> buildd_policy.distro.name
-    u'ubuntu'
-
-Policies can think about distroseriess...
-
-    >>> buildd_policy.setDistroSeriesAndPocket("hoary")
-    >>> print buildd_policy.distroseries.name
-    hoary
-
-Policies can make decisions based on whether or not they want to
-approve an upload automatically (I.E. move it straight to ACCEPTED
-instead of UNAPPROVED)
-
-    >>> from lp.registry.interfaces.distribution import IDistributionSet
-    >>> from lp.registry.interfaces.series import SeriesStatus
-    >>> ubuntu = getUtility(IDistributionSet)['ubuntu']
-    >>> hoary = ubuntu['hoary']
-
-    >>> class FakeUpload:
-    ...   def __init__(self, ppa=False):
-    ...       self.is_ppa = ppa
-
-    >>> print hoary.status.name
-    DEVELOPMENT
-
-Uploads to the RELEASE pocket of not FROZEN distroseries are approved
-by the insecure policy:
-
-    >>> insecure_policy.setDistroSeriesAndPocket('hoary')
-    >>> insecure_policy.autoApprove(FakeUpload())
-    True
-
-    >>> insecure_policy.autoApprove(FakeUpload(ppa=True))
-    True
-
-So are uploads to the PROPOSED pocket:
-
-    >>> insecure_policy.setDistroSeriesAndPocket('hoary-proposed')
-    >>> insecure_policy.autoApprove(FakeUpload())
-    True
-
-When the distroseries is FROZEN the uploads should wait in UNAPPROVED queue:
-
-    >>> login('[e-mail]')
-    >>> hoary.status = SeriesStatus.FROZEN
-    >>> from lp.services.database.sqlbase import flush_database_updates
-    >>> flush_database_updates()
-
-    >>> insecure_policy.autoApprove(FakeUpload())
-    False
-
-PPA uploads continue to be auto-approved:
-
-    >>> insecure_policy.autoApprove(FakeUpload(ppa=True))
-    True
-
-Reset the policy so that we can try again...
-
-    >>> insecure_policy.policy = None
-    >>> insecure_policy.distroseries = None
-
-Uploads to the UPDATES pocket are not auto-approved by the insecure policy
-
-    >>> insecure_policy.setDistroSeriesAndPocket('hoary-updates')
-    >>> insecure_policy.autoApprove(FakeUpload())
-    False
-
-Despite of not being allowed yet (see UploadPolicy.checkUpload) PPA
-uploads to post-release pockets would also be auto-approved:
-
-    >>> insecure_policy.autoApprove(FakeUpload(ppa=True))
-    True
-
-Uploads to the PROPOSED pocket are also not auto-approved by the insecure
-policy, either before or after release:
-
-    >>> insecure_policy.setDistroSeriesAndPocket('hoary-proposed')
-    >>> insecure_policy.autoApprove(FakeUpload())
-    False
-    >>> hoary.status = SeriesStatus.CURRENT
-    >>> flush_database_updates()
-    >>> insecure_policy.autoApprove(FakeUpload())
-    False

=== modified file 'lib/lp/archiveuploader/uploadpolicy.py'
--- lib/lp/archiveuploader/uploadpolicy.py	2012-07-20 08:29:06 +0000
+++ lib/lp/archiveuploader/uploadpolicy.py	2012-08-14 14:08:16 +0000
@@ -322,6 +322,8 @@
 
     def autoApprove(self, upload):
         """Check that all custom files in this upload can be auto-approved."""
+        if upload.is_ppa:
+            return True
         if upload.binaryful:
             for custom_file in upload.changes.custom_files:
                 if not custom_file.autoApprove():