Merge ~pappacena/launchpad:oci-unify-request-builds into launchpad:master

[Thiago F. Pappacena]

Use IOCIRecipeRequestBuildsJobSource when requesting builds from the web interface

d560b56... by Thiago F. Pappacena on 2020-10-01

Merge branch 'master' into oci-unify-request-builds

[Colin Watson (cjwatson)]

I'd like the internal API to be shaped a bit differently in some places, but this is along the right lines.

review: Needs Fixing

d00a405... by Thiago F. Pappacena on 2020-10-02

Refactoring template display of pending OCI build requests

85294fe... by Thiago F. Pappacena on 2020-10-02

Refactoring OCIRecipe.requestBuilds to receive a list of architectures

7d63a1d... by Thiago F. Pappacena on 2020-10-02

Refactoring the way we get pending OCI build jobs

ecb5a47... by Thiago F. Pappacena on 2020-10-02

Adding back requestBuild to OCIRecipe interface

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes.

5c5893c... by Thiago F. Pappacena on 2020-10-05

Merge branch 'master' into oci-unify-request-builds

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index cf11bfb..460b108 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -64,7 +64,6 @@ from lp.oci.interfaces.ocirecipe import (
     NoSuchOCIRecipe,
     OCI_RECIPE_ALLOW_CREATE,
     OCI_RECIPE_WEBHOOKS_FEATURE_FLAG,
-    OCIRecipeBuildAlreadyPending,
     OCIRecipeFeatureDisabled,
     )
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
@@ -74,7 +73,6 @@ from lp.oci.interfaces.ociregistrycredentials import (
     user_can_edit_credentials_for_owner,
     )
 from lp.services.features import getFeatureFlag
-from lp.services.helpers import english_list
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     canonical_url,
@@ -644,36 +642,17 @@ class OCIRecipeRequestBuildsView(LaunchpadFormView):
                 'distro_arch_series',
                 'You need to select at least one architecture.')
 
-    def requestBuilds(self, data):
-        """User action for requesting a number of builds.
-
-        We raise exceptions for most errors, but if there's already a
-        pending build for a particular architecture, we simply record that
-        so that other builds can be queued and a message displayed to the
-        caller.
-        """
-        informational = {}
-        builds = []
-        already_pending = []
-        for arch in data['distro_arch_series']:
-            try:
-                build = self.context.requestBuild(self.user, arch)
-                builds.append(build)
-            except OCIRecipeBuildAlreadyPending:
-                already_pending.append(arch)
-        if already_pending:
-            informational['already_pending'] = (
-                "An identical build is already pending for %s." %
-                english_list(arch.architecturetag for arch in already_pending))
-        return builds, informational
-
     @action('Request builds', name='request')
     def request_action(self, action, data):
-        builds, informational = self.requestBuilds(data)
-        already_pending = informational.get('already_pending')
-        notification_text = new_builds_notification_text(
-            builds, already_pending)
-        self.request.response.addNotification(notification_text)
+        if data.get('distro_arch_series'):
+            architectures = [
+                arch.architecturetag for arch in data['distro_arch_series']]
+        else:
+            architectures = None
+        self.context.requestBuilds(self.user, architectures)
+        self.request.response.addNotification(
+            "Your builds were scheduled and should start soon. "
+            "Refresh this page for details.")
         self.next_url = self.cancel_url
 
 
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index 72f9c83..82422f1 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -50,18 +50,22 @@ from lp.oci.interfaces.ocirecipe import (
     IOCIRecipeSet,
     OCI_RECIPE_ALLOW_CREATE,
     )
+from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
 from lp.oci.interfaces.ociregistrycredentials import (
     IOCIRegistryCredentialsSet,
     )
 from lp.oci.tests.helpers import OCIConfigHelperMixin
 from lp.registry.interfaces.person import IPersonSet
+from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.interfaces import IStore
 from lp.services.features.testing import FeatureFixture
+from lp.services.job.runner import JobRunner
 from lp.services.propertycache import get_property_cache
 from lp.services.webapp import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
+    admin_logged_in,
     anonymous_logged_in,
     BrowserTestCase,
     login,
@@ -71,6 +75,7 @@ from lp.testing import (
     TestCaseWithFactory,
     time_counter,
     )
+from lp.testing.dbuser import dbuser
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -938,6 +943,30 @@ class TestOCIRecipeRequestBuildsView(BaseTestOCIRecipeView):
         self.assertRaises(
             Unauthorized, self.getViewBrowser, self.recipe, "+request-builds")
 
+    def runRequestBuildJobs(self):
+        with admin_logged_in():
+            jobs = getUtility(IOCIRecipeRequestBuildsJobSource).iterReady()
+            with dbuser(config.IOCIRecipeRequestBuildsJobSource.dbuser):
+                JobRunner(jobs).runAll()
+
+    def test_pending_build_requests_not_shown_if_absent(self):
+        self.recipe.requestBuilds(self.recipe.owner)
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        content = extract_text(find_main_content(browser.contents))
+        self.assertIn(
+            "You have 1 pending build request. "
+            "The builds should start automatically soon.",
+            content.replace("\n", " "))
+
+        # Run the build request, so we don't have any pending one. The
+        # message should be gone then.
+        self.runRequestBuildJobs()
+        browser = self.getViewBrowser(self.recipe, user=self.person)
+        content = extract_text(find_main_content(browser.contents))
+        self.assertNotIn(
+            "The builds should start automatically soon.",
+            content.replace("\n", " "))
+
     def test_request_builds_action(self):
         # Requesting a build creates pending builds.
         browser = self.getViewBrowser(
@@ -946,6 +975,8 @@ class TestOCIRecipeRequestBuildsView(BaseTestOCIRecipeView):
         self.assertTrue(browser.getControl("i386").selected)
         browser.getControl("Request builds").click()
 
+        self.runRequestBuildJobs()
+
         login_person(self.person)
         builds = self.recipe.pending_builds
         self.assertContentEqual(
@@ -954,19 +985,6 @@ class TestOCIRecipeRequestBuildsView(BaseTestOCIRecipeView):
         self.assertContentEqual(
             [2510], set(build.buildqueue_record.lastscore for build in builds))
 
-    def test_request_builds_rejects_duplicate(self):
-        # A duplicate build request causes a notification.
-        self.recipe.requestBuild(self.person, self.distroseries["amd64"])
-        browser = self.getViewBrowser(
-            self.recipe, "+request-builds", user=self.person)
-        self.assertTrue(browser.getControl("amd64").selected)
-        self.assertTrue(browser.getControl("i386").selected)
-        browser.getControl("Request builds").click()
-        main_text = extract_text(find_main_content(browser.contents))
-        self.assertIn("1 new build has been queued.", main_text)
-        self.assertIn(
-            "An identical build is already pending for amd64.", main_text)
-
     def test_request_builds_no_architectures(self):
         # Selecting no architectures causes a validation failure.
         browser = self.getViewBrowser(
diff --git a/lib/lp/oci/interfaces/ocirecipe.py b/lib/lp/oci/interfaces/ocirecipe.py
index 718cdd3..0bd3cd2 100644
--- a/lib/lp/oci/interfaces/ocirecipe.py
+++ b/lib/lp/oci/interfaces/ocirecipe.py
@@ -54,6 +54,7 @@ from zope.schema import (
     Dict,
     Int,
     List,
+    Set,
     Text,
     TextLine,
     )
@@ -173,6 +174,10 @@ class IOCIRecipeBuildRequest(Interface):
         value_type=Reference(schema=Interface),
         required=True, readonly=True)))
 
+    architectures = Set(
+        title=_("If set, limit builds to these architecture tags."),
+        value_type=TextLine(), required=False, readonly=True)
+
 
 class IOCIRecipeView(Interface):
     """`IOCIRecipe` attributes that require launchpad.View permission."""
@@ -202,6 +207,9 @@ class IOCIRecipeView(Interface):
         "The architectures that are available to be enabled or disabled for "
         "this OCI recipe.")
 
+    pending_build_requests = Attribute(
+        "The list of build requests that didn't trigger builds yet.")
+
     # This should only be set by using IOCIProject.setOfficialRecipe
     official = Bool(
         title=_("OCI project official"),
@@ -265,6 +273,10 @@ class IOCIRecipeView(Interface):
     def requestBuild(requester, architecture):
         """Request that the OCI recipe is built.
 
+        Please, note that this method does not associate the created build
+        with an OCIRecipeBuildRequest. So, prefer using the
+        OCIRecipe.requestBuilds (plural).
+
         :param requester: The person requesting the build.
         :param architecture: The architecture to build for.
         :return: `IOCIRecipeBuild`.
@@ -273,7 +285,7 @@ class IOCIRecipeView(Interface):
     @call_with(requester=REQUEST_USER)
     @export_factory_operation(IOCIRecipeBuildRequest, [])
     @operation_for_version("devel")
-    def requestBuilds(requester):
+    def requestBuilds(requester, architectures=None):
         """Request that the OCI recipe is built for all available
         architectures.
 
@@ -281,7 +293,7 @@ class IOCIRecipeView(Interface):
         :return: A `IOCIRecipeBuildRequest` instance.
         """
 
-    def requestBuildsFromJob(requester):
+    def requestBuildsFromJob(requester, architectures=None):
         """Synchronous part of requesting builds, that should be called as a
         Celery task.
 
diff --git a/lib/lp/oci/interfaces/ocirecipejob.py b/lib/lp/oci/interfaces/ocirecipejob.py
index 17d9fd5..104a2ff 100644
--- a/lib/lp/oci/interfaces/ocirecipejob.py
+++ b/lib/lp/oci/interfaces/ocirecipejob.py
@@ -81,13 +81,26 @@ class IOCIRecipeRequestBuildsJob(IRunnableJob):
 
 class IOCIRecipeRequestBuildsJobSource(IJobSource):
 
-    def create(oci_recipe, requester):
+    def create(oci_recipe, requester, architectures=None):
         """Request builds of an OCI Recipe.
 
         :param oci_recipe: The OCI recipe to build.
         :param requester: The person requesting the builds.
+        :param architectures: Build only for this list of
+            architectures, if they are available for the recipe. If
+            None, build for all available architectures.
         """
 
-    def getByOCIRecipeAndID(self, oci_recipe, job_id):
+    def getByOCIRecipeAndID(recipe, job_id):
         """Retrieve the build job by OCI recipe and the given job ID.
         """
+
+    def findByOCIRecipe(recipe, statuses=None, job_ids=None):
+        """Find jobs for an OCI recipe.
+
+        :param oci_recipe: An OCI recipe to search for.
+        :param statuses: An optional iterable of `JobStatus`es to search for.
+        :param job_ids: An optional iterable of job IDs to search for.
+        :return: A sequence of `OCIRecipeRequestBuildsJob`s with the specified
+            OCI recipe.
+        """
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 3ca817c..15fa5be 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -421,32 +421,42 @@ class OCIRecipe(Storm, WebhookTargetMixin):
     def getBuildRequest(self, job_id):
         return OCIRecipeBuildRequest(self, job_id)
 
-    def requestBuildsFromJob(self, requester, build_request=None):
+    def requestBuildsFromJob(self, requester, build_request=None,
+                             architectures=None):
         self._checkRequestBuild(requester)
-        distro_arch_series_to_build = set(self.getAllowedArchitectures())
+        distro_arch_series = set(self.getAllowedArchitectures())
 
         builds = []
-        for das in distro_arch_series_to_build:
+        for das in distro_arch_series:
+            if (architectures is not None
+                    and das.architecturetag not in architectures):
+                continue
             try:
                 builds.append(self.requestBuild(
                     requester, das, build_request=build_request))
             except OCIRecipeBuildAlreadyPending:
                 pass
 
-        # If we have distro_arch_series_to_build, but they all failed to due
+        # If we have distro_arch_series, but they all failed to due
         # to pending builds, we fail the job.
-        if len(distro_arch_series_to_build) > 0 and len(builds) == 0:
+        if len(distro_arch_series) > 0 and len(builds) == 0:
             raise OCIRecipeBuildAlreadyPending
 
         return builds
 
-    def requestBuilds(self, requester):
+    def requestBuilds(self, requester, architectures=None):
         self._checkRequestBuild(requester)
         job = getUtility(IOCIRecipeRequestBuildsJobSource).create(
-            self, requester)
+            self, requester, architectures)
         return self.getBuildRequest(job.job_id)
 
     @property
+    def pending_build_requests(self):
+        util = getUtility(IOCIRecipeRequestBuildsJobSource)
+        return util.findByOCIRecipe(
+            self, statuses=(JobStatus.WAITING, JobStatus.RUNNING))
+
+    @property
     def push_rules(self):
         rules = IStore(self).find(
             OCIPushRule,
diff --git a/lib/lp/oci/model/ocirecipejob.py b/lib/lp/oci/model/ocirecipejob.py
index 095181e..15eac4f 100644
--- a/lib/lp/oci/model/ocirecipejob.py
+++ b/lib/lp/oci/model/ocirecipejob.py
@@ -17,6 +17,7 @@ from lazr.enum import (
     )
 import six
 from storm.databases.postgres import JSON
+from storm.expr import Desc
 from storm.properties import Int
 from storm.references import Reference
 from storm.store import EmptyResultSet
@@ -162,9 +163,13 @@ class OCIRecipeRequestBuildsJob(OCIRecipeJobDerived):
     config = config.IOCIRecipeRequestBuildsJobSource
 
     @classmethod
-    def create(cls, recipe, requester):
+    def create(cls, recipe, requester, architectures=None):
         """See `OCIRecipeRequestBuildsJob`."""
-        metadata = {"requester": requester.id}
+        metadata = {
+            "requester": requester.id,
+            "architectures": (
+                list(architectures) if architectures is not None else None),
+        }
         recipe_job = OCIRecipeJob(recipe, cls.class_job_type, metadata)
         job = cls(recipe_job)
         job.celeryRunOnCommit()
@@ -180,6 +185,20 @@ class OCIRecipeRequestBuildsJob(OCIRecipeJobDerived):
             raise NotFoundError("Could not find job ID %s" % job_id)
         return cls(job)
 
+    @classmethod
+    def findByOCIRecipe(cls, recipe, statuses=None, job_ids=None):
+        conditions = [
+            OCIRecipeJob.recipe == recipe,
+            OCIRecipeJob.job_type == cls.class_job_type]
+        if statuses is not None:
+            conditions.append(Job._status.is_in(statuses))
+        if job_ids is not None:
+            conditions.append(OCIRecipeJob.job_id.is_in(job_ids))
+        return IStore(OCIRecipeJob).find(
+            (OCIRecipeJob, Job),
+            OCIRecipeJob.job_id == Job.id,
+            *conditions).order_by(Desc(OCIRecipeJob.job_id))
+
     def getOperationDescription(self):
         return "requesting builds of %s" % self.recipe
 
@@ -234,6 +253,11 @@ class OCIRecipeRequestBuildsJob(OCIRecipeJobDerived):
         """See `OCIRecipeRequestBuildsJob`."""
         self.metadata["builds"] = [build.id for build in builds]
 
+    @property
+    def architectures(self):
+        architectures = self.metadata["architectures"]
+        return set(architectures) if architectures is not None else None
+
     def run(self):
         """See `IRunnableJob`."""
         requester = self.requester
@@ -243,7 +267,8 @@ class OCIRecipeRequestBuildsJob(OCIRecipeJobDerived):
             return
         try:
             self.builds = self.recipe.requestBuildsFromJob(
-                requester, build_request=self.build_request)
+                requester, build_request=self.build_request,
+                architectures=self.architectures)
             self.error_message = None
         except self.retry_error_types:
             raise
diff --git a/lib/lp/oci/templates/ocirecipe-index.pt b/lib/lp/oci/templates/ocirecipe-index.pt
index 5c3ca56..9a9aefc 100644
--- a/lib/lp/oci/templates/ocirecipe-index.pt
+++ b/lib/lp/oci/templates/ocirecipe-index.pt
@@ -84,6 +84,15 @@
     </div>
 
     <h2>Latest builds</h2>
+    <div tal:define="count context/pending_build_requests/count|nothing;
+                  plural string: build requests;
+                  singular string: build request;"
+         tal:condition="count">
+      You have <span tal:replace="count" /> pending
+      <tal:plural
+        metal:use-macro="context/@@+base-layout-macros/plural-message"/>.
+      The builds should start automatically soon.
+    </div>
     <table id="latest-builds-listing" class="listing"
            style="margin-bottom: 1em;">
       <thead>