launchpad-reviewers team mailing list archive

[Ines Almeida <mp+475349@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:rocks-re-add-recipe-preloading into launchpad:master.

Commit message:
Re-add rock recipe preloading
    
Recipe preloading was previously failing after allowing remote git repositories. We now skip preloading the git refs for remote repositories, but still preload everything else

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/475349

This stems from us removing these pre-loading here: https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/474516

These were removed because it broke some pages.

This took some investigation, but in the end the change is not very large - from what I can see, we can simply skip fetching git refs for remote repositories since we don't store them in the DB eitherway.

Ran all tests that contained the words "rocks" and "charms" as a check
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:rocks-re-add-recipe-preloading into launchpad:master.

diff --git a/lib/lp/rocks/browser/rockrecipelisting.py b/lib/lp/rocks/browser/rockrecipelisting.py
index 01191a5..4ab4c11 100644
--- a/lib/lp/rocks/browser/rockrecipelisting.py
+++ b/lib/lp/rocks/browser/rockrecipelisting.py
@@ -9,6 +9,8 @@ __all__ = [
     "ProjectRockRecipeListingView",
 ]
 
+from functools import partial
+
 from zope.component import getUtility
 
 from lp.rocks.interfaces.rockrecipe import IRockRecipeSet
@@ -41,15 +43,10 @@ class RockRecipeListingView(LaunchpadView, FeedsMixin):
         recipes = getUtility(IRockRecipeSet).findByContext(
             self.context, visible_by_user=self.user
         )
-        # XXX jugmac00 2024-10-06: we need to skip preloading until the
-        # function is able to handle rock recipes with external git
-        # repositories, see https://warthogs.atlassian.net/browse/LP-1972
-        #
-        # loader = partial(
-        #     getUtility(IRockRecipeSet).preloadDataForRecipes, user=self.user
-        # )
-        # self.recipes = DecoratedResultSet(recipes, pre_iter_hook=loader)
-        self.recipes = DecoratedResultSet(recipes)
+        loader = partial(
+            getUtility(IRockRecipeSet).preloadDataForRecipes, user=self.user
+        )
+        self.recipes = DecoratedResultSet(recipes, pre_iter_hook=loader)
 
     @cachedproperty
     def batchnav(self):
diff --git a/lib/lp/rocks/browser/tests/test_rockrecipelisting.py b/lib/lp/rocks/browser/tests/test_rockrecipelisting.py
index 8782263..67e0443 100644
--- a/lib/lp/rocks/browser/tests/test_rockrecipelisting.py
+++ b/lib/lp/rocks/browser/tests/test_rockrecipelisting.py
@@ -129,6 +129,27 @@ class TestRockRecipeListing(BrowserTestCase):
             text,
         )
 
+    def test_person_recipe_listing_remote_repos(self):
+        # We can see rock recipes for a person, evewhen recipes refer to remote
+        # repositories
+        owner = self.factory.makePerson(displayname="Rock Owner")
+        remote_ref = self.factory.makeGitRefRemote()
+        with MemoryFeatureFixture({ROCK_RECIPE_ALLOW_CREATE: "on"}):
+            self.factory.makeRockRecipe(
+                registrant=owner,
+                owner=owner,
+                git_ref=remote_ref,
+                date_created=ONE_DAY_AGO,
+            )
+        text = self.getMainText(owner, "+rock-recipes")
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            """
+            Rock recipes for Rock Owner
+            Name            Source                  Registered
+            rock-name.*    ~.*:.*                  .*""",
+            text,
+        )
+
     def test_project_recipe_listing(self):
         # We can see rock recipes for a project.
         project = self.factory.makeProduct(displayname="Rockable")
diff --git a/lib/lp/rocks/model/rockrecipe.py b/lib/lp/rocks/model/rockrecipe.py
index 65a1627..1fdda11 100644
--- a/lib/lp/rocks/model/rockrecipe.py
+++ b/lib/lp/rocks/model/rockrecipe.py
@@ -1033,8 +1033,15 @@ class RockRecipeSet:
         if repositories:
             GenericGitCollection.preloadDataForRepositories(repositories)
 
+        # We only store git refs from Launchpad git repositories, not from
+        # remote git repositories. As such, skip fetching git refs from remote
+        # reositories, i.e., that don't have the `git_repository` property set.
         git_refs = GitRef.findByReposAndPaths(
-            [(recipe.git_repository, recipe.git_path) for recipe in recipes]
+            [
+                (recipe.git_repository, recipe.git_path)
+                for recipe in recipes
+                if recipe.git_repository
+            ]
         )
         for recipe in recipes:
             git_ref = git_refs.get((recipe.git_repository, recipe.git_path))
diff --git a/lib/lp/rocks/model/rockrecipebuild.py b/lib/lp/rocks/model/rockrecipebuild.py
index 496903e..99a5e66 100644
--- a/lib/lp/rocks/model/rockrecipebuild.py
+++ b/lib/lp/rocks/model/rockrecipebuild.py
@@ -32,6 +32,7 @@ from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.person import Person
+from lp.rocks.interfaces.rockrecipe import IRockRecipeSet
 from lp.rocks.interfaces.rockrecipebuild import (
     IRockFile,
     IRockRecipeBuild,
@@ -429,7 +430,7 @@ class RockRecipeBuildSet(SpecificBuildFarmJobSourceMixin):
 
     def preloadBuildsData(self, builds):
         # Circular import.
-        # from lp.rocks.model.rockrecipe import RockRecipe
+        from lp.rocks.model.rockrecipe import RockRecipe
 
         load_related(Person, builds, ["requester_id"])
         lfas = load_related(LibraryFileAlias, builds, ["log_id"])
@@ -441,12 +442,8 @@ class RockRecipeBuildSet(SpecificBuildFarmJobSourceMixin):
             DistroSeries, distroarchserieses, ["distroseries_id"]
         )
         load_related(Distribution, distroserieses, ["distribution_id"])
-        # XXX jugmac00 2024-10-06: we need to skip preloading until the
-        # function is able to handle rock recipes with external git
-        # repositories, see https://warthogs.atlassian.net/browse/LP-1972
-        #
-        # recipes = load_related(RockRecipe, builds, ["recipe_id"])
-        # getUtility(IRockRecipeSet).preloadDataForRecipes(recipes)
+        recipes = load_related(RockRecipe, builds, ["recipe_id"])
+        getUtility(IRockRecipeSet).preloadDataForRecipes(recipes)
 
     def getByBuildFarmJobs(self, build_farm_jobs):
         """See `ISpecificBuildFarmJobSource`."""