launchpad-reviewers team mailing list archive

[Colin Watson <mp+417806@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:cibuild-on-ref-creation into launchpad:master.

Commit message:
Request CIBuilds on ref creation as well as update

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/417806

Most of the things we do when Git refs are updated really do only make sense for refs that previously existed (e.g. a snap recipe can't become stale as a result of a ref being created, because it can't have pointed to that ref to begin with).  However, creating CI builds for new refs makes sense, so arrange for that to happen on both create and update events.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:cibuild-on-ref-creation into launchpad:master.

diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index dc3d63e..882d3da 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -851,6 +851,10 @@
       handler="lp.code.model.gitrepository.git_repository_modified"/>
   <subscriber
       for="lp.code.interfaces.gitrepository.IGitRepository
+           lp.code.interfaces.event.IGitRefsCreatedEvent"
+      handler="lp.code.subscribers.git.refs_created"/>
+  <subscriber
+      for="lp.code.interfaces.gitrepository.IGitRepository
            lp.code.interfaces.event.IGitRefsUpdatedEvent"
       handler="lp.code.subscribers.git.refs_updated"/>
 
diff --git a/lib/lp/code/event/git.py b/lib/lp/code/event/git.py
index 317c687..fbf97e4 100644
--- a/lib/lp/code/event/git.py
+++ b/lib/lp/code/event/git.py
@@ -4,13 +4,27 @@
 """Events related to Git repositories."""
 
 __all__ = [
+    'GitRefsCreatedEvent',
     'GitRefsUpdatedEvent',
     ]
 
 from zope.interface import implementer
 from zope.interface.interfaces import ObjectEvent
 
-from lp.code.interfaces.event import IGitRefsUpdatedEvent
+from lp.code.interfaces.event import (
+    IGitRefsCreatedEvent,
+    IGitRefsUpdatedEvent,
+    )
+
+
+@implementer(IGitRefsCreatedEvent)
+class GitRefsCreatedEvent(ObjectEvent):
+    """See `IGitRefsCreatedEvent`."""
+
+    def __init__(self, repository, paths, logger):
+        super().__init__(repository)
+        self.paths = paths
+        self.logger = logger
 
 
 @implementer(IGitRefsUpdatedEvent)
diff --git a/lib/lp/code/interfaces/event.py b/lib/lp/code/interfaces/event.py
index 4ce9107..c99a6a2 100644
--- a/lib/lp/code/interfaces/event.py
+++ b/lib/lp/code/interfaces/event.py
@@ -5,6 +5,7 @@
 
 __all__ = [
     'IBranchMergeProposalNeedsReviewEvent',
+    'IGitRefsCreatedEvent',
     'IGitRefsUpdatedEvent',
     'IReviewerNominatedEvent',
     ]
@@ -21,5 +22,9 @@ class IBranchMergeProposalNeedsReviewEvent(IObjectEvent):
     """The merge proposal has moved from work in progress to needs reivew."""
 
 
+class IGitRefsCreatedEvent(IObjectEvent):
+    """Some references in a Git repository have been created."""
+
+
 class IGitRefsUpdatedEvent(IObjectEvent):
     """Some references in a Git repository have been updated."""
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index c52fd5e..c2146cb 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -110,7 +110,10 @@ from lp.code.errors import (
     GitTargetError,
     NoSuchGitReference,
     )
-from lp.code.event.git import GitRefsUpdatedEvent
+from lp.code.event.git import (
+    GitRefsCreatedEvent,
+    GitRefsUpdatedEvent,
+    )
 from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_FINAL_STATES,
     )
@@ -821,6 +824,9 @@ class GitRepository(StormBase, WebhookTargetMixin, AccessTokenTargetMixin,
             created = []
 
         self.date_last_modified = UTC_NOW
+        if created:
+            notify(GitRefsCreatedEvent(
+                self, [value[1] for value in created], logger))
         if updated:
             notify(GitRefsUpdatedEvent(
                 self, [value[1] for value in updated], logger))
diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index b216a5e..7392d4c 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -8,6 +8,7 @@ from datetime import (
     timedelta,
     )
 import hashlib
+from unittest import mock
 
 from fixtures import FakeLogger
 from lazr.lifecycle.snapshot import Snapshot
@@ -157,9 +158,16 @@ class TestGitRefScanJob(TestCaseWithFactory):
         author = repository.owner
         author_date_start = datetime(2015, 1, 1, tzinfo=pytz.UTC)
         author_date_gen = time_counter(author_date_start, timedelta(days=1))
-        self.useFixture(GitHostingFixture(
-            refs=self.makeFakeRefs(paths),
-            commits=self.makeFakeCommits(author, author_date_gen, paths)))
+        hosting_fixture = self.useFixture(GitHostingFixture(
+            refs=self.makeFakeRefs(paths)))
+
+        def getCommits(path, commit_oids, filter_paths=None, **kwargs):
+            if filter_paths is not None:
+                return []
+            else:
+                return self.makeFakeCommits(author, author_date_gen, paths)
+
+        hosting_fixture.getCommits = mock.Mock(side_effect=getCommits)
         with dbuser("branchscanner"):
             JobRunner([job]).runAll()
         self.assertRefsMatch(repository.refs, repository, paths)
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 10742af..6e93fb2 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -1493,8 +1493,8 @@ class TestGitRepositoryModifications(TestCaseWithFactory):
             date_created=datetime(2015, 6, 1, tzinfo=pytz.UTC))
         [ref] = self.factory.makeGitRefs(repository=repository)
         new_refs_info = {
-            ref.path: {
-                "sha1": "0000000000000000000000000000000000000000",
+            "refs/heads/new": {
+                "sha1": ref.commit_sha1,
                 "type": ref.object_type,
                 },
             }
@@ -1503,12 +1503,13 @@ class TestGitRepositoryModifications(TestCaseWithFactory):
             repository, "date_last_modified", UTC_NOW)
 
     def test_update_ref_sets_date_last_modified(self):
+        self.useFixture(GitHostingFixture())
         repository = self.factory.makeGitRepository(
             date_created=datetime(2015, 6, 1, tzinfo=pytz.UTC))
         [ref] = self.factory.makeGitRefs(repository=repository)
         new_refs_info = {
-            "refs/heads/new": {
-                "sha1": ref.commit_sha1,
+            ref.path: {
+                "sha1": "0000000000000000000000000000000000000000",
                 "type": ref.object_type,
                 },
             }
@@ -1867,6 +1868,7 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
         self.assertThat(refs, MatchesSetwise(*matchers))
 
     def test_create(self):
+        self.useFixture(GitHostingFixture())
         repository = self.factory.makeGitRepository()
         self.assertEqual([], list(repository.refs))
         paths = ("refs/heads/master", "refs/tags/1.0")
@@ -2036,7 +2038,8 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
                 "type": GitObjectType.BLOB,
                 },
             }
-        repository.createOrUpdateRefs(refs_info)
+        with GitHostingFixture():
+            repository.createOrUpdateRefs(refs_info)
         self.useFixture(GitHostingFixture(refs={
             "refs/heads/blob": {
                 "object": {
@@ -3396,7 +3399,61 @@ class TestGitRepositoryRequestCIBuilds(TestCaseWithFactory):
 
     layer = ZopelessDatabaseLayer
 
-    def test_findByGitRepository_with_configuration(self):
+    def test_findByGitRepository_created_with_configuration(self):
+        # If a new ref has CI configuration, we request CI builds.
+        logger = BufferLogger()
+        repository = self.factory.makeGitRepository()
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
+        dases = [
+            self.factory.makeBuildableDistroArchSeries(
+                distroseries=distroseries)
+            for _ in range(2)]
+        configuration = dedent("""\
+            pipeline: [test]
+            jobs:
+                test:
+                    series: {series}
+                    architectures: [{architectures}]
+            """.format(
+                series=distroseries.name,
+                architectures=", ".join(
+                    das.architecturetag for das in dases))).encode()
+        new_commit = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
+        self.useFixture(GitHostingFixture(commits=[
+            {
+                "sha1": new_commit,
+                "blobs": {".launchpad.yaml": configuration},
+                },
+            ]))
+        with dbuser("branchscanner"):
+            repository.createOrUpdateRefs(
+                {"refs/heads/test":
+                    {"sha1": new_commit, "type": GitObjectType.COMMIT}},
+                logger=logger)
+
+        results = getUtility(ICIBuildSet).findByGitRepository(repository)
+        for result in results:
+            self.assertTrue(ICIBuild.providedBy(result))
+
+        self.assertThat(
+            results,
+            MatchesSetwise(*(
+                MatchesStructure.byEquality(
+                    git_repository=repository,
+                    commit_sha1=new_commit,
+                    distro_arch_series=das)
+                for das in dases)))
+        self.assertContentEqual(
+            [
+                "INFO Requesting CI build for {commit} on "
+                "{series}/{arch}".format(
+                    commit=new_commit, series=distroseries.name,
+                    arch=das.architecturetag)
+                for das in dases],
+            logger.getLogBuffer().splitlines())
+
+    def test_findByGitRepository_updated_with_configuration(self):
         # If a changed ref has CI configuration, we request CI builds.
         logger = BufferLogger()
         [ref] = self.factory.makeGitRefs()
diff --git a/lib/lp/code/subscribers/git.py b/lib/lp/code/subscribers/git.py
index 831ac49..18f5ac3 100644
--- a/lib/lp/code/subscribers/git.py
+++ b/lib/lp/code/subscribers/git.py
@@ -8,6 +8,16 @@ from zope.component import getUtility
 from lp.code.interfaces.cibuild import ICIBuildSet
 
 
+def _request_ci_builds(repository, event):
+    getUtility(ICIBuildSet).requestBuildsForRefs(
+        repository, event.paths, logger=event.logger)
+
+
+def refs_created(repository, event):
+    """Some references in a Git repository have been created."""
+    _request_ci_builds(repository, event)
+
+
 def refs_updated(repository, event):
     """Some references in a Git repository have been updated."""
     repository.updateMergeCommitIDs(event.paths)
@@ -16,5 +26,4 @@ def refs_updated(repository, event):
     repository.markSnapsStale(event.paths)
     repository.markCharmRecipesStale(event.paths)
     repository.detectMerges(event.paths, logger=event.logger)
-    getUtility(ICIBuildSet).requestBuildsForRefs(
-        repository, event.paths, logger=event.logger)
+    _request_ci_builds(repository, event)
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index d8179b9..7c79147 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -156,6 +156,7 @@ from lp.code.model.diff import (
     Diff,
     PreviewDiff,
     )
+from lp.code.tests.helpers import GitHostingFixture
 from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet
 from lp.oci.interfaces.ocirecipe import IOCIRecipeSet
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
@@ -1815,10 +1816,11 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                 "type": GitObjectType.COMMIT,
                 }
             for path in paths}
-        refs_by_path = {
-            ref.path: ref
-            for ref in removeSecurityProxy(repository).createOrUpdateRefs(
-                refs_info, get_objects=True)}
+        with GitHostingFixture():
+            refs_by_path = {
+                ref.path: ref
+                for ref in removeSecurityProxy(repository).createOrUpdateRefs(
+                    refs_info, get_objects=True)}
         return [refs_by_path[path] for path in paths]
 
     def makeGitRefRemote(self, repository_url=None, path=None):