launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25332
[Merge] ~pappacena/launchpad:create-mp-refs into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:create-mp-refs into launchpad:master with ~pappacena/launchpad:githosting-copy-and-delete-refs as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390581
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:create-mp-refs into launchpad:master.
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 344bc43..9e4c129 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -8,6 +8,8 @@ __metaclass__ = type
__all__ = [
'ContributorGitIdentity',
'GitIdentityMixin',
+ 'GIT_CREATE_MP_VIRTUAL_REF',
+ 'GIT_MP_VIRTUAL_REF_FORMAT',
'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE',
'git_repository_name_validator',
'IGitRepository',
@@ -105,6 +107,11 @@ valid_git_repository_name_pattern = re.compile(
r"^(?i)[a-z0-9][a-z0-9+\.\-@_]*\Z")
+# Virtual ref where we automatically put a copy of any open merge proposal.
+GIT_MP_VIRTUAL_REF_FORMAT = 'refs/merge/{mp.id}/head'
+GIT_CREATE_MP_VIRTUAL_REF = 'git.mergeproposal_virtualref.enabled'
+
+
def valid_git_repository_name(name):
"""Return True iff the name is valid as a Git repository name.
@@ -595,11 +602,15 @@ class IGitRepositoryView(IHasRecipes):
def updateLandingTargets(paths):
"""Update landing targets (MPs where this repository is the source).
- For each merge proposal, create `UpdatePreviewDiffJob`s.
+ For each merge proposal, create `UpdatePreviewDiffJob`s, and runs
+ the appropriate GitHosting.copyRefs operation to allow having
+ virtual merge refs with the updated branch version.
:param paths: A list of reference paths. Any merge proposals whose
source is this repository and one of these paths will have their
diffs updated.
+ :return: Returns a tuple with the list of background jobs created,
+ and the lits of refs copy requested to GitHosting.
"""
def markRecipesStale(paths):
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 2a346b4..8a29a99 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -12,6 +12,7 @@ __all__ = [
from collections import defaultdict
from email.utils import make_msgid
+import logging
from operator import attrgetter
import sys
@@ -83,7 +84,12 @@ from lp.code.interfaces.codereviewcomment import ICodeReviewComment
from lp.code.interfaces.codereviewinlinecomment import (
ICodeReviewInlineCommentSet,
)
+from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitref import IGitRef
+from lp.code.interfaces.gitrepository import (
+ GIT_CREATE_MP_VIRTUAL_REF,
+ GIT_MP_VIRTUAL_REF_FORMAT,
+ )
from lp.code.mail.branch import RecipientReason
from lp.code.model.branchrevision import BranchRevision
from lp.code.model.codereviewcomment import CodeReviewComment
@@ -93,6 +99,7 @@ from lp.code.model.diff import (
IncrementalDiff,
PreviewDiff,
)
+from lp.code.model.githosting import RefCopyOperation
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -122,6 +129,7 @@ from lp.services.database.sqlbase import (
quote,
SQLBase,
)
+from lp.services.features import getFeatureFlag
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
@@ -143,6 +151,9 @@ from lp.soyuz.enums import (
)
+logger = logging.getLogger(__name__)
+
+
def is_valid_transition(proposal, from_state, next_state, user=None):
"""Is it valid for this user to move this proposal to to next_state?
@@ -968,6 +979,8 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
branch_merge_proposal=self.id):
job.destroySelf()
self._preview_diffs.remove()
+ self.deleteGitHostingVirtualRefs()
+
self.destroySelf()
def getUnlandedSourceBranchRevisions(self):
@@ -1214,6 +1227,66 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
for diff in diffs)
return [diff_dict.get(revisions) for revisions in revision_list]
+ def getGitHostingRefCopyOperations(self):
+ """Gets the list of GitHosting copy operations that should be done
+ in order to keep this MP's virtual refs up-to-date."""
+ if (not self.target_git_repository
+ or not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF)):
+ return []
+ # If the source repository is private and it's opening a MP to
+ # another repository, we should not risk to to have the private code
+ # undisclosed to everyone with permission to see the target repo:
+ private_source = self.source_git_repository.private
+ same_repo = self.source_git_repository == self.target_git_repository
+ same_owner = self.source_git_repository.owner.inTeam(
+ self.target_git_repository.owner)
+ if private_source and not (same_repo or same_owner):
+ return []
+ return [RefCopyOperation(
+ self.source_git_commit_sha1,
+ self.target_git_repository.getInternalPath(),
+ GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))]
+
+ def copyGitHostingVirtualRefs(self):
+ """Requests virtual refs copy operations on GitHosting in order to
+ keep them up-to-date with current MP's state.
+
+ :return: The list of copy operations requested."""
+ copy_operations = self.getGitHostingRefCopyOperations()
+ if copy_operations:
+ hosting_client = getUtility(IGitHostingClient)
+ hosting_client.copyRefs(
+ self.source_git_repository.getInternalPath(),
+ copy_operations)
+ return copy_operations
+
+ def deleteGitHostingVirtualRefs(self, except_refs=None):
+ """Deletes on git code hosting service all virtual refs, except
+ those ones in the given list."""
+ if self.source_git_ref is None:
+ return
+ if not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF):
+ # XXX pappacena 2020-09-15: Do not try to remove virtual refs if
+ # the feature is disabled. It might be disabled due to bug on
+ # the first versions, for example, and we should have a way to
+ # skip this feature entirely.
+ # Once the feature is stable, we should actually always delete
+ # the virtual refs, since we don't know if the feature was
+ # enabled or not when the branch was created.
+ return
+ ref = GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self)
+ if ref not in (except_refs or []):
+ hosting_client = getUtility(IGitHostingClient)
+ hosting_client.deleteRef(
+ self.target_git_repository.getInternalPath(), ref)
+
+ def syncGitHostingVirtualRefs(self):
+ """Requests all copies and deletion of virtual refs to make git code
+ hosting in sync with this MP."""
+ operations = self.copyGitHostingVirtualRefs()
+ copied_refs = [i.target_ref for i in operations]
+ self.deleteGitHostingVirtualRefs(except_refs=copied_refs)
+
def scheduleDiffUpdates(self, return_jobs=True):
"""See `IBranchMergeProposal`."""
from lp.code.model.branchmergeproposaljob import (
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index b957574..12a7d2f 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -32,7 +32,6 @@ from lp.code.errors import (
GitRepositoryDeletionFault,
GitRepositoryScanFault,
GitTargetError,
- NoSuchGitReference,
)
from lp.code.interfaces.githosting import IGitHostingClient
from lp.services.config import config
@@ -289,6 +288,13 @@ class GitHostingClient:
url = "/repo/%s/%s" % (path, ref)
self._delete(url)
except requests.RequestException as e:
+ if (e.response is not None and
+ e.response.status_code == requests.codes.NOT_FOUND):
+ if logger is not None:
+ logger.info(
+ "Tried to delete a ref that doesn't exist: %s (%s)" %
+ (path, ref))
+ return
raise GitReferenceDeletionFault(
"Error deleting %s from repo %s: HTTP %s" %
(ref, path, e.response.status_code))
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 3d2408e..e2451fa 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1161,9 +1161,15 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
def updateLandingTargets(self, paths):
"""See `IGitRepository`."""
jobs = []
+ copy_operations = []
for merge_proposal in self.getActiveLandingTargets(paths):
jobs.extend(merge_proposal.scheduleDiffUpdates())
- return jobs
+ copy_operations += merge_proposal.getGitHostingRefCopyOperations()
+
+ if copy_operations:
+ hosting_client = getUtility(IGitHostingClient)
+ hosting_client.copyRefs(self.getInternalPath(), copy_operations)
+ return jobs, copy_operations
def _getRecipes(self, paths=None):
"""Undecorated version of recipes for use by `markRecipesStale`."""
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 0d2457d..54cf516 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -66,6 +66,10 @@ from lp.code.interfaces.branchmergeproposal import (
IBranchMergeProposalGetter,
IBranchMergeProposalJobSource,
)
+from lp.code.interfaces.gitrepository import (
+ GIT_CREATE_MP_VIRTUAL_REF,
+ GIT_MP_VIRTUAL_REF_FORMAT,
+ )
from lp.code.model.branchmergeproposal import (
BranchMergeProposal,
BranchMergeProposalGetter,
@@ -96,6 +100,7 @@ from lp.testing import (
ExpectedException,
launchpadlib_for,
login,
+ login_admin,
login_person,
person_logged_in,
TestCaseWithFactory,
@@ -255,6 +260,136 @@ class TestBranchMergeProposalPrivacy(TestCaseWithFactory):
self.assertContentEqual([owner, team], subscriptions)
+class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory):
+ """Ensure that BranchMergeProposal creation run the appropriate copy
+ and delete of virtual refs, like ref/merge/<id>/head."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestGitBranchMergeProposalVirtualRefs, self).setUp()
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+
+ def test_copy_git_merge_virtual_ref(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ mp = self.factory.makeBranchMergeProposalForGit()
+
+ copy_operations = mp.getGitHostingRefCopyOperations()
+ self.assertEqual(1, len(copy_operations))
+ self.assertThat(copy_operations[0], MatchesStructure(
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+
+ self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+ args, kwargs = self.hosting_fixture.copyRefs.calls[0]
+ arg_path, arg_copy_operations = args
+ self.assertEqual({}, kwargs)
+ self.assertEqual(mp.source_git_repository.getInternalPath(), arg_path)
+ self.assertEqual(1, len(arg_copy_operations))
+ self.assertThat(arg_copy_operations[0], MatchesStructure(
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+
+ def test_getGitHostingRefCopyOperations_private_source(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ login_admin()
+ source_repo = self.factory.makeGitRepository(
+ information_type=InformationType.PRIVATESECURITY)
+ target_repo = self.factory.makeGitRepository(target=source_repo.target)
+ [source] = self.factory.makeGitRefs(source_repo)
+ [target] = self.factory.makeGitRefs(target_repo)
+ mp = self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+ self.assertEqual([], mp.getGitHostingRefCopyOperations())
+
+ def test_getGitHostingRefCopyOperations_private_source_same_repo(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ login_admin()
+ repo = self.factory.makeGitRepository(
+ information_type=InformationType.PRIVATESECURITY)
+ [source, target] = self.factory.makeGitRefs(
+ repo, ['refs/heads/bugfix', 'refs/heads/master'])
+ mp = self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+ operations = mp.getGitHostingRefCopyOperations()
+ self.assertEqual(1, len(operations))
+ self.assertThat(operations[0], MatchesStructure(
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
+ target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
+ ))
+
+ def test_getGitHostingRefCopyOperations_private_source_same_owner(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ login_admin()
+ source_repo = self.factory.makeGitRepository(
+ information_type=InformationType.PRIVATESECURITY)
+ target_repo = self.factory.makeGitRepository(
+ target=source_repo.target, owner=source_repo.owner)
+ [source] = self.factory.makeGitRefs(source_repo)
+ [target] = self.factory.makeGitRefs(target_repo)
+ mp = self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+ operations = mp.getGitHostingRefCopyOperations()
+ self.assertEqual(1, len(operations))
+ self.assertThat(operations[0], MatchesStructure(
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
+ target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
+ ))
+
+ def test_syncGitHostingVirtualRefs(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ login_admin()
+ login_admin()
+ source_repo = self.factory.makeGitRepository()
+ target_repo = self.factory.makeGitRepository(target=source_repo.target)
+ [source] = self.factory.makeGitRefs(source_repo)
+ [target] = self.factory.makeGitRefs(target_repo)
+ mp = self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+
+ # mp.syncGitHostingVirtualRefs should have been triggered by event.
+ # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
+ self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+ args, kwargs = self.hosting_fixture.copyRefs.calls[0]
+ self.assertEquals({}, kwargs)
+ self.assertEqual(args[0], source_repo.getInternalPath())
+ self.assertEqual(1, len(args[1]))
+ self.assertThat(args[1][0], MatchesStructure(
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo=Equals(mp.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+
+ self.assertEqual(0, self.hosting_fixture.deleteRef.call_count)
+
+ def test_syncGitHostingVirtualRefs_private_source_deletes_ref(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ login_admin()
+ source_repo = self.factory.makeGitRepository(
+ information_type=InformationType.PRIVATESECURITY)
+ target_repo = self.factory.makeGitRepository(target=source_repo.target)
+ [source] = self.factory.makeGitRefs(source_repo)
+ [target] = self.factory.makeGitRefs(target_repo)
+ mp = self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+
+ # mp.syncGitHostingVirtualRefs should have been triggered by event.
+ # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
+ self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
+ self.assertEqual(1, self.hosting_fixture.deleteRef.call_count)
+ args, kwargs = self.hosting_fixture.deleteRef.calls[0]
+ self.assertEqual({}, kwargs)
+ self.assertEqual(
+ (target_repo.getInternalPath(), "refs/merge/%s/head" % mp.id),
+ args)
+
+
class TestBranchMergeProposalTransitions(TestCaseWithFactory):
"""Test the state transitions of branch merge proposals."""
@@ -1184,6 +1319,10 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
def test_delete_triggers_webhooks(self):
# When an existing merge proposal is deleted, any relevant webhooks
# are triggered.
+ self.useFixture(FeatureFixture({
+ GIT_CREATE_MP_VIRTUAL_REF: "on",
+ BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"}))
+ hosting_fixture = self.useFixture(GitHostingFixture())
logger = self.useFixture(FakeLogger())
source = self.makeBranch()
target = self.makeBranch(same_target_as=source)
@@ -1204,8 +1343,11 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
old=MatchesDict(self.getExpectedPayload(proposal, redact=True)))
proposal.deleteProposal()
delivery = hook.deliveries.one()
+ self.assertIsNotNone(delivery)
self.assertCorrectDelivery(expected_payload, hook, delivery)
self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+ self.assertEqual(
+ 1 if self.git else 0, hosting_fixture.deleteRef.call_count)
class TestGetAddress(TestCaseWithFactory):
@@ -1506,6 +1648,18 @@ class TestBranchMergeProposalDeletion(TestCaseWithFactory):
self.assertRaises(
SQLObjectNotFound, BranchMergeProposalJob.get, job_id)
+ def test_deleteProposal_for_git_removes_virtual_ref(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ hosting_fixture = self.useFixture(GitHostingFixture())
+ proposal = self.factory.makeBranchMergeProposalForGit()
+ proposal.deleteProposal()
+
+ self.assertEqual(1, hosting_fixture.deleteRef.call_count)
+ args = hosting_fixture.deleteRef.calls[0]
+ self.assertEqual((
+ (proposal.target_git_repository.getInternalPath(),
+ 'refs/merge/%s/head' % proposal.id), {}), args)
+
class TestBranchMergeProposalBugs(WithVCSScenarios, TestCaseWithFactory):
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index 04f7a8e..bacbd66 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -23,7 +23,6 @@ from lazr.restful.utils import get_current_browser_request
import responses
from six.moves.urllib.parse import (
parse_qsl,
- urljoin,
urlsplit,
)
from testtools.matchers import (
@@ -43,7 +42,6 @@ from lp.code.errors import (
GitRepositoryDeletionFault,
GitRepositoryScanFault,
GitTargetError,
- NoSuchGitReference,
)
from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.model.githosting import RefCopyOperation
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 62bf865..b0116e1 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -96,6 +96,7 @@ from lp.code.interfaces.gitnamespace import (
IGitNamespaceSet,
)
from lp.code.interfaces.gitrepository import (
+ GIT_CREATE_MP_VIRTUAL_REF,
IGitRepository,
IGitRepositorySet,
IGitRepositoryView,
@@ -782,6 +783,7 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
# Make sure that the tests all flush the database changes.
self.addCleanup(Store.of(self.repository).flush)
login_person(self.user)
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
def test_deletable(self):
# A newly created repository can be deleted without any problems.
@@ -823,7 +825,6 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
def test_code_import_does_not_disable_deletion(self):
# A repository that has an attached code import can be deleted.
- self.useFixture(GitHostingFixture())
code_import = self.factory.makeCodeImport(
target_rcs_type=TargetRevisionControlSystems.GIT)
repository = code_import.git_repository
@@ -983,6 +984,7 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
# unsubscribe the repository owner here.
self.repository.unsubscribe(
self.repository.owner, self.repository.owner)
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
def test_plain_repository(self):
# A fresh repository has no deletion requirements.
@@ -1114,7 +1116,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
def test_code_import_requirements(self):
# Code imports are not included explicitly in deletion requirements.
- self.useFixture(GitHostingFixture())
code_import = self.factory.makeCodeImport(
target_rcs_type=TargetRevisionControlSystems.GIT)
# Remove the implicit repository subscription first.
@@ -1125,7 +1126,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
def test_code_import_deletion(self):
# break_references allows deleting a code import repository.
- self.useFixture(GitHostingFixture())
code_import = self.factory.makeCodeImport(
target_rcs_type=TargetRevisionControlSystems.GIT)
code_import_id = code_import.id
@@ -1181,7 +1181,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
def test_DeleteCodeImport(self):
# DeleteCodeImport.__call__ must delete the CodeImport.
- self.useFixture(GitHostingFixture())
code_import = self.factory.makeCodeImport(
target_rcs_type=TargetRevisionControlSystems.GIT)
code_import_id = code_import.id
@@ -1520,6 +1519,10 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
+ def setUp(self):
+ super(TestGitRepositoryRefs, self).setUp()
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+
def test__convertRefInfo(self):
# _convertRefInfo converts a valid info dictionary.
sha1 = unicode(hashlib.sha1("").hexdigest())
@@ -1788,18 +1791,17 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
# planRefChanges excludes some ref prefixes by default, and can be
# configured otherwise.
repository = self.factory.makeGitRepository()
- hosting_fixture = self.useFixture(GitHostingFixture())
repository.planRefChanges("dummy")
self.assertEqual(
[{"exclude_prefixes": ["refs/changes/"]}],
- hosting_fixture.getRefs.extract_kwargs())
- hosting_fixture.getRefs.calls = []
+ self.hosting_fixture.getRefs.extract_kwargs())
+ self.hosting_fixture.getRefs.calls = []
self.pushConfig(
"codehosting", git_exclude_ref_prefixes="refs/changes/ refs/pull/")
repository.planRefChanges("dummy")
self.assertEqual(
[{"exclude_prefixes": ["refs/changes/", "refs/pull/"]}],
- hosting_fixture.getRefs.extract_kwargs())
+ self.hosting_fixture.getRefs.extract_kwargs())
def test_fetchRefCommits(self):
# fetchRefCommits fetches detailed tip commit metadata for the
@@ -1871,9 +1873,8 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
def test_fetchRefCommits_empty(self):
# If given an empty refs dictionary, fetchRefCommits returns early
# without contacting the hosting service.
- hosting_fixture = self.useFixture(GitHostingFixture())
GitRepository.fetchRefCommits("dummy", {})
- self.assertEqual([], hosting_fixture.getCommits.calls)
+ self.assertEqual([], self.hosting_fixture.getCommits.calls)
def test_synchroniseRefs(self):
# synchroniseRefs copes with synchronising a repository where some
@@ -1916,7 +1917,6 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
self.assertThat(repository.refs, MatchesSetwise(*matchers))
def test_set_default_branch(self):
- hosting_fixture = self.useFixture(GitHostingFixture())
repository = self.factory.makeGitRepository()
self.factory.makeGitRefs(
repository=repository,
@@ -1927,22 +1927,20 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
self.assertEqual(
[((repository.getInternalPath(),),
{"default_branch": "refs/heads/new"})],
- hosting_fixture.setProperties.calls)
+ self.hosting_fixture.setProperties.calls)
self.assertEqual("refs/heads/new", repository.default_branch)
def test_set_default_branch_unchanged(self):
- hosting_fixture = self.useFixture(GitHostingFixture())
repository = self.factory.makeGitRepository()
self.factory.makeGitRefs(
repository=repository, paths=["refs/heads/master"])
removeSecurityProxy(repository)._default_branch = "refs/heads/master"
with person_logged_in(repository.owner):
repository.default_branch = "master"
- self.assertEqual([], hosting_fixture.setProperties.calls)
+ self.assertEqual([], self.hosting_fixture.setProperties.calls)
self.assertEqual("refs/heads/master", repository.default_branch)
def test_set_default_branch_imported(self):
- hosting_fixture = self.useFixture(GitHostingFixture())
repository = self.factory.makeGitRepository(
repository_type=GitRepositoryType.IMPORTED)
self.factory.makeGitRefs(
@@ -1955,7 +1953,7 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
"Cannot modify non-hosted Git repository %s." %
repository.display_name,
setattr, repository, "default_branch", "new")
- self.assertEqual([], hosting_fixture.setProperties.calls)
+ self.assertEqual([], self.hosting_fixture.setProperties.calls)
self.assertEqual("refs/heads/master", repository.default_branch)
def test_exception_unset_default_branch(self):
@@ -2576,18 +2574,59 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
- def test_schedules_diff_updates(self):
+ def setUp(self):
+ super(TestGitRepositoryUpdateLandingTargets, self).setUp()
+ self.hosting_fixture = self.useFixture(GitHostingFixture())
+
+ def assertSchedulesDiffUpdate(self, with_mp_virtual_ref):
"""Create jobs for all merge proposals."""
bmp1 = self.factory.makeBranchMergeProposalForGit()
bmp2 = self.factory.makeBranchMergeProposalForGit(
source_ref=bmp1.source_git_ref)
- jobs = bmp1.source_git_repository.updateLandingTargets(
+
+ # Only enable this virtual ref here, since
+ # self.factory.makeBranchMergeProposalForGit also tries to create
+ # the virtual refs.
+ if with_mp_virtual_ref:
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ else:
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: ""}))
+ jobs, ref_copies = 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)
+ if not with_mp_virtual_ref:
+ self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
+ else:
+ self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
+ args, kwargs = self.hosting_fixture.copyRefs.calls[0]
+ self.assertEqual({}, kwargs)
+ self.assertEqual(2, len(args))
+ path, operations = args
+ self.assertEqual(
+ path, bmp1.source_git_repository.getInternalPath())
+ self.assertThat(operations[0], MatchesStructure(
+ source_ref=Equals(bmp1.source_git_commit_sha1),
+ target_repo=Equals(
+ bmp1.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % bmp1.id),
+ ))
+ self.assertThat(operations[1], MatchesStructure(
+ source_ref=Equals(bmp2.source_git_commit_sha1),
+ target_repo=Equals(
+ bmp2.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % bmp2.id),
+ ))
+
+ def test_schedules_diff_updates_with_mp_virtual_ref(self):
+ self.assertSchedulesDiffUpdate(True)
+
+ def test_schedules_diff_updates_without_mp_virtual_ref(self):
+ self.assertSchedulesDiffUpdate(False)
+
def test_ignores_final(self):
"""Diffs for proposals in final states aren't updated."""
[source_ref] = self.factory.makeGitRefs()
@@ -2599,8 +2638,15 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
for bmp in source_ref.landing_targets:
if bmp.queue_status not in FINAL_STATES:
removeSecurityProxy(bmp).deleteProposal()
- jobs = source_ref.repository.updateLandingTargets([source_ref.path])
+
+ # Enable the feature here, since factory.makeBranchMergeProposalForGit
+ # would also trigger the copy refs call.
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ jobs, ref_copies = source_ref.repository.updateLandingTargets(
+ [source_ref.path])
self.assertEqual(0, len(jobs))
+ self.assertEqual(0, len(ref_copies))
+ self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
class TestGitRepositoryMarkRecipesStale(TestCaseWithFactory):
diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py
index a214c8f..b394388 100644
--- a/lib/lp/code/subscribers/branchmergeproposal.py
+++ b/lib/lp/code/subscribers/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Event subscribers for branch merge proposals."""
@@ -63,9 +63,13 @@ def _trigger_webhook(merge_proposal, payload):
def merge_proposal_created(merge_proposal, event):
"""A new merge proposal has been created.
- Create a job to update the diff for the merge proposal; trigger webhooks.
+ Create a job to update the diff for the merge proposal; trigger webhooks
+ and copy virtual refs as needed.
"""
getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
+
+ merge_proposal.syncGitHostingVirtualRefs()
+
if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
payload = {
"action": "created",
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index a4f8416..fb21ee9 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -369,6 +369,8 @@ class GitHostingFixture(fixtures.Fixture):
self.getBlob = FakeMethod(result=blob)
self.delete = FakeMethod()
self.disable_memcache = disable_memcache
+ self.copyRefs = FakeMethod()
+ self.deleteRef = FakeMethod()
def _setUp(self):
self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))
Follow ups
