launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20682
[Merge] lp:~cjwatson/launchpad/git-update-related-bugs into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-update-related-bugs into lp:launchpad with lp:~cjwatson/launchpad/bmp-buglinktarget-ui as a prerequisite.
Commit message:
Automatically update related bugs for Git-based merge proposals based on their commit logs.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1492926 in Launchpad itself: "Git merge proposals can't be linked to bugs"
https://bugs.launchpad.net/launchpad/+bug/1492926
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-update-related-bugs/+merge/298369
Automatically update related bugs for Git-based merge proposals based on their commit logs.
I've intentionally used the same regexes that Soyuz uses to parse changelogs, for compatibility with package branches and because it's reasonably compact in commit messages.
There's a corner case here with prerequisite branches where we may end up with too many related bugs, noted in an XXX comment. I think it's tolerable for the moment.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-update-related-bugs into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2016-06-25 10:08:32 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2016-06-25 10:08:32 +0000
@@ -386,6 +386,12 @@
def getRelatedBugTasks(user):
"""Return the Bug tasks related to this merge proposal."""
+ def updateRelatedBugsFromSource():
+ """Update related bug links based on commits in the source branch.
+
+ This is currently only meaningful for Git-based merge proposals.
+ """
+
def getRevisionsSinceReviewStart():
"""Return all the revisions added since the review began.
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2016-05-14 09:54:32 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2016-06-25 10:08:32 +0000
@@ -508,8 +508,11 @@
kept up to date.
"""
- def scheduleDiffUpdates(paths):
- """Create UpdatePreviewDiffJobs for landing targets.
+ def updateLandingTargets(paths):
+ """Update landing targets (MPs where this repository is the source).
+
+ For each merge proposal, update linked bugs and create
+ `UpdatePreviewDiffJob`s.
:param paths: A list of reference paths. Any merge proposals whose
source is this repository and one of these paths will have their
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2016-06-25 10:08:32 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2016-06-25 10:08:32 +0000
@@ -123,6 +123,10 @@
get_property_cache,
)
from lp.services.xref.interfaces import IXRefSet
+from lp.soyuz.enums import (
+ re_bug_numbers,
+ re_lp_closes,
+ )
def is_valid_transition(proposal, from_state, next_state, user=None):
@@ -428,6 +432,56 @@
return super(BranchMergeProposal, self).unlinkBug(
bug, user=user, check_permissions=check_permissions)
+ def _fetchRelatedBugIDsFromSource(self):
+ """Fetch related bug IDs from the source branch."""
+ from lp.bugs.model.bug import Bug
+ # Only currently used for Git.
+ assert self.source_git_ref is not None
+ # XXX cjwatson 2016-06-11: This may return too many bugs in the case
+ # where a prerequisite branch fixes a bug which is not fixed by
+ # further commits on the source branch. To fix this, we need
+ # turnip's log API to be able to take multiple stop parameters.
+ commits = self.getUnlandedSourceBranchRevisions()
+ bug_ids = set()
+ for commit in commits:
+ if "commit_message" in commit:
+ for match in re_lp_closes.finditer(commit["commit_message"]):
+ for bug_id in re_bug_numbers.findall(match.group(0)):
+ bug_ids.add(int(bug_id))
+ # Only return bug IDs that exist.
+ return set(IStore(Bug).find(Bug.id, Bug.id.is_in(bug_ids)))
+
+ def updateRelatedBugsFromSource(self):
+ """See `IBranchMergeProposal`."""
+ # Only currently used for Git.
+ assert self.source_git_ref is not None
+ current_bug_ids_from_source = {
+ int(id): (props['metadata'] or {}).get('from_source', False)
+ for (_, id), props in getUtility(IXRefSet).findFrom(
+ (u'merge_proposal', unicode(self.id)), types=[u'bug']).items()}
+ current_bug_ids = set(current_bug_ids_from_source)
+ new_bug_ids = self._fetchRelatedBugIDsFromSource()
+ # Only remove links marked as originating in the source branch.
+ remove_bug_ids = set(
+ bug_id for bug_id in current_bug_ids - new_bug_ids
+ if current_bug_ids_from_source[bug_id])
+ if remove_bug_ids:
+ getUtility(IXRefSet).delete(
+ {(u'merge_proposal', unicode(self.id)):
+ [(u'bug', unicode(bug_id)) for bug_id in remove_bug_ids]})
+ add_bug_ids = new_bug_ids - current_bug_ids
+ # XXX cjwatson 2016-06-11: We could perhaps set creator and
+ # date_created based on commit information, but then we'd have to
+ # work out what to do in the case of multiple commits referring to
+ # the same bug, updating properties if more such commits arrive
+ # later, etc. This is simple and does the job for now.
+ if add_bug_ids:
+ getUtility(IXRefSet).create(
+ {(u'merge_proposal', unicode(self.id)):
+ {(u'bug', unicode(bug_id)):
+ {'metadata': {'from_source': True}}
+ for bug_id in add_bug_ids}})
+
@property
def address(self):
return 'mp+%d@%s' % (self.id, config.launchpad.code_domain)
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2016-05-14 09:54:32 +0000
+++ lib/lp/code/model/gitrepository.py 2016-06-25 10:08:32 +0000
@@ -966,11 +966,12 @@
store.invalidate()
return updated
- def scheduleDiffUpdates(self, paths):
+ def updateLandingTargets(self, paths):
"""See `IGitRepository`."""
from lp.code.model.branchmergeproposaljob import UpdatePreviewDiffJob
jobs = []
for merge_proposal in self.getActiveLandingTargets(paths):
+ merge_proposal.updateRelatedBugsFromSource()
jobs.append(UpdatePreviewDiffJob.create(merge_proposal))
return jobs
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2016-06-25 10:08:32 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2016-06-25 10:08:32 +0000
@@ -10,6 +10,7 @@
timedelta,
)
from difflib import unified_diff
+import hashlib
from unittest import TestCase
from lazr.lifecycle.event import (
@@ -21,7 +22,9 @@
from sqlobject import SQLObjectNotFound
from storm.locals import Store
from testtools.matchers import (
+ ContainsDict,
Equals,
+ Is,
MatchesDict,
MatchesStructure,
)
@@ -57,6 +60,7 @@
IBranchMergeProposalGetter,
notify_modified,
)
+from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.model.branchmergeproposal import (
BranchMergeProposalGetter,
is_valid_transition,
@@ -77,6 +81,7 @@
from lp.services.database.constants import UTC_NOW
from lp.services.features.testing import FeatureFixture
from lp.services.webapp import canonical_url
+from lp.services.xref.interfaces import IXRefSet
from lp.testing import (
ExpectedException,
launchpadlib_for,
@@ -90,6 +95,8 @@
)
from lp.testing.dbuser import dbuser
from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import (
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
@@ -1463,6 +1470,125 @@
def _makeBranchMergeProposal(self):
return self.factory.makeBranchMergeProposalForGit()
+ def test__fetchRelatedBugIDsFromSource(self):
+ # _fetchRelatedBugIDsFromSource makes a reasonable backend call and
+ # parses commit messages.
+ bugs = [self.factory.makeBug() for _ in range(3)]
+ bmp = self._makeBranchMergeProposal()
+ hosting_client = FakeMethod()
+ hosting_client.getLog = FakeMethod(result=[
+ {
+ u"sha1": bmp.source_git_commit_sha1,
+ u"message": u"Commit 1\n\nLP: #%d" % bugs[0].id,
+ },
+ {
+ u"sha1": unicode(hashlib.sha1("1").hexdigest()),
+ # Will not be matched.
+ u"message": u"Commit 2; see LP #%d" % bugs[1].id,
+ },
+ {
+ u"sha1": unicode(hashlib.sha1("2").hexdigest()),
+ u"message": u"Commit 3; LP: #%d" % bugs[2].id,
+ },
+ {
+ u"sha1": unicode(hashlib.sha1("3").hexdigest()),
+ # Non-existent bug ID will not be returned.
+ u"message": u"Non-existent bug; LP: #%d" % (bugs[2].id + 100),
+ },
+ ])
+ self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+ related_bugs = bmp._fetchRelatedBugIDsFromSource()
+ path = "%s:%s" % (
+ bmp.target_git_repository.getInternalPath(),
+ bmp.source_git_repository.getInternalPath())
+ self.assertEqual(
+ [((path, bmp.source_git_commit_sha1),
+ {"limit": 10, "stop": bmp.target_git_commit_sha1,
+ "logger": None})],
+ hosting_client.getLog.calls)
+ self.assertContentEqual([bugs[0].id, bugs[2].id], related_bugs)
+
+ def _setUpLog(self, bugs):
+ """Set up a fake log response referring to the given bugs."""
+ if getattr(self, "hosting_client", None) is None:
+ self.hosting_client = FakeMethod()
+ self.hosting_client.getLog = FakeMethod()
+ self.useFixture(
+ ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
+ self.hosting_client.getLog = FakeMethod(result=[
+ {
+ u"sha1": unicode(hashlib.sha1(str(i)).hexdigest()),
+ u"message": u"LP: #%d" % bug.id,
+ }
+ for i, bug in enumerate(bugs)])
+
+ def test_updateRelatedBugsFromSource_no_links(self):
+ # updateRelatedBugsFromSource does nothing if there are no related
+ # bugs in either the database or the source branch.
+ bmp = self._makeBranchMergeProposal()
+ self._setUpLog([])
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([], bmp.bugs)
+
+ def test_updateRelatedBugsFromSource_new_links(self):
+ # If the source branch has related bugs not yet reflected in the
+ # database, updateRelatedBugsFromSource creates the links.
+ bugs = [self.factory.makeBug() for _ in range(3)]
+ bmp = self._makeBranchMergeProposal()
+ self._setUpLog([bugs[0]])
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([bugs[0]], bmp.bugs)
+ self._setUpLog(bugs)
+ bmp.updateRelatedBugsFromSource()
+ self.assertContentEqual(bugs, bmp.bugs)
+
+ def test_updateRelatedBugsFromSource_same_links(self):
+ # If the database and the source branch list the same related bugs,
+ # updateRelatedBugsFromSource does nothing.
+ bug = self.factory.makeBug()
+ bmp = self._makeBranchMergeProposal()
+ self._setUpLog([bug])
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([bug], bmp.bugs)
+ # The second run is a no-op.
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([bug], bmp.bugs)
+
+ def test_updateRelatedBugsFromSource_removes_old_links(self):
+ # If the database records a source-branch-originating related bug
+ # that is no longer listed by the source branch,
+ # updateRelatedBugsFromSource removes the link.
+ bug = self.factory.makeBug()
+ bmp = self._makeBranchMergeProposal()
+ self._setUpLog([bug])
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([bug], bmp.bugs)
+ self._setUpLog([])
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([], bmp.bugs)
+
+ def test_updateRelatedBugsFromSource_leaves_manual_links(self):
+ # If a bug was linked to the merge proposal manually,
+ # updateRelatedBugsFromSource leaves the link alone regardless of
+ # whether it is listed by the source branch.
+ bug = self.factory.makeBug()
+ bmp = self._makeBranchMergeProposal()
+ bmp.linkBug(bug)
+ self._setUpLog([])
+ bmp.updateRelatedBugsFromSource()
+ self.assertEqual([bug], bmp.bugs)
+ matches_expected_xref = MatchesDict(
+ {(u"bug", unicode(bug.id)): ContainsDict({"metadata": Is(None)})})
+ self.assertThat(
+ getUtility(IXRefSet).findFrom(
+ (u"merge_proposal", unicode(bmp.id)), types=[u"bug"]),
+ matches_expected_xref)
+ self._setUpLog([bug])
+ self.assertThat(
+ getUtility(IXRefSet).findFrom(
+ (u"merge_proposal", unicode(bmp.id)), types=[u"bug"]),
+ matches_expected_xref)
+
class TestNotifyModified(TestCaseWithFactory):
=== modified file 'lib/lp/code/model/tests/test_gitjob.py'
--- lib/lp/code/model/tests/test_gitjob.py 2016-05-13 10:58:59 +0000
+++ lib/lp/code/model/tests/test_gitjob.py 2016-06-25 10:08:32 +0000
@@ -245,6 +245,7 @@
}},
}
hosting_client = FakeGitHostingClient(new_refs, [])
+ hosting_client.getLog = FakeMethod(result=[])
hosting_client.detectMerges = FakeMethod(
result={source.commit_sha1: u'0' * 40})
self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2016-05-13 10:58:59 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2016-06-25 10:08:32 +0000
@@ -989,7 +989,7 @@
class TestGitRepositoryRefs(TestCaseWithFactory):
"""Tests for ref handling."""
- layer = DatabaseFunctionalLayer
+ layer = LaunchpadFunctionalLayer
def test__convertRefInfo(self):
# _convertRefInfo converts a valid info dictionary.
@@ -1110,6 +1110,9 @@
return [UpdatePreviewDiffJob(job) for job in jobs]
def test_update_schedules_diff_update(self):
+ hosting_client = FakeMethod()
+ hosting_client.getLog = FakeMethod(result=[])
+ self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
repository = self.factory.makeGitRepository()
[ref] = self.factory.makeGitRefs(repository=repository)
self.assertRefsMatch(repository.refs, repository, [ref.path])
@@ -1819,23 +1822,44 @@
self.assertNotEqual(ref2_2, bmp2.source_git_ref)
-class TestGitRepositoryScheduleDiffUpdates(TestCaseWithFactory):
-
- layer = DatabaseFunctionalLayer
-
- def test_scheduleDiffUpdates(self):
+class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def test_updates_related_bugs(self):
+ """All non-final merge proposals have their related bugs updated."""
+ bug = self.factory.makeBug()
+ bmp = self.factory.makeBranchMergeProposalForGit()
+ self.assertEqual([], bmp.bugs)
+ self.assertEqual([], bug.linked_merge_proposals)
+ hosting_client = FakeMethod()
+ hosting_client.getLog = FakeMethod(result=[
+ {
+ u"sha1": bmp.source_git_commit_sha1,
+ u"message": u"Fix upside-down messages\n\nLP: #%d" % bug.id,
+ },
+ ])
+ self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+ bmp.source_git_repository.updateLandingTargets([bmp.source_git_path])
+ self.assertEqual([bug], bmp.bugs)
+ self.assertEqual([bmp], bug.linked_merge_proposals)
+
+ def test_schedules_diff_updates(self):
"""Create jobs for all merge proposals."""
+ hosting_client = FakeMethod()
+ hosting_client.getLog = FakeMethod(result=[])
+ self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
bmp1 = self.factory.makeBranchMergeProposalForGit()
bmp2 = self.factory.makeBranchMergeProposalForGit(
source_ref=bmp1.source_git_ref)
- jobs = bmp1.source_git_repository.scheduleDiffUpdates(
+ jobs = bmp1.source_git_repository.updateLandingTargets(
[bmp1.source_git_path])
self.assertEqual(2, len(jobs))
bmps_to_update = [
removeSecurityProxy(job).branch_merge_proposal for job in jobs]
self.assertContentEqual([bmp1, bmp2], bmps_to_update)
- def test_scheduleDiffUpdates_ignores_final(self):
+ def test_ignores_final(self):
"""Diffs for proposals in final states aren't updated."""
[source_ref] = self.factory.makeGitRefs()
for state in FINAL_STATES:
@@ -1846,7 +1870,7 @@
for bmp in source_ref.landing_targets:
if bmp.queue_status not in FINAL_STATES:
removeSecurityProxy(bmp).deleteProposal()
- jobs = source_ref.repository.scheduleDiffUpdates([source_ref.path])
+ jobs = source_ref.repository.updateLandingTargets([source_ref.path])
self.assertEqual(0, len(jobs))
=== modified file 'lib/lp/code/subscribers/git.py'
--- lib/lp/code/subscribers/git.py 2016-01-12 04:34:09 +0000
+++ lib/lp/code/subscribers/git.py 2016-06-25 10:08:32 +0000
@@ -9,6 +9,6 @@
def refs_updated(repository, event):
"""Some references in a Git repository have been updated."""
repository.updateMergeCommitIDs(event.paths)
- repository.scheduleDiffUpdates(event.paths)
+ repository.updateLandingTargets(event.paths)
repository.markRecipesStale(event.paths)
repository.detectMerges(event.paths, logger=event.logger)
Follow ups
