launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25594
[Merge] ~pappacena/launchpad:git-repo-async-privacy-virtualrefs into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:git-repo-async-privacy-virtualrefs into launchpad:master with ~pappacena/launchpad:git-repo-async-provacy-garbo as a prerequisite.
Commit message:
Doing virtual refs sync when running git repository privacy change background task
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/393075
This branch carries code from https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390581, but it does a big chunch of refactoring of the code contained there. Mostly, the bulk operations got encapsulated in the GitHostingClient, so we would have a single point of change once Turnip will be able to run the copy/delete refs operations in bulk (instead of doing a bunch of requests).
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:git-repo-async-privacy-virtualrefs into launchpad:master.
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 290e7bc..94cb548 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -2083,7 +2083,7 @@ public.xref = SELECT, INSERT, DELETE
type=user
[privacy-change-jobs]
-groups=script
+groups=script, branchscanner
public.accessartifact = SELECT, UPDATE, DELETE, INSERT
public.accessartifactgrant = SELECT, UPDATE, DELETE, INSERT
public.accesspolicyartifact = SELECT, UPDATE, DELETE, INSERT
diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
index f2ec4ec..83c51dc 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
@@ -2336,7 +2336,7 @@ class TestLatestProposalsForEachBranchGit(
@staticmethod
def _setBranchInvisible(branch):
- removeSecurityProxy(branch.repository).transitionToInformationType(
+ removeSecurityProxy(branch.repository)._transitionToInformationType(
InformationType.USERDATA, branch.owner, verify_policy=False)
diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index bf481c9..efe1e4a 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""The interface for branch merge proposals."""
@@ -519,6 +519,50 @@ class IBranchMergeProposalView(Interface):
def getLatestDiffUpdateJob():
"""Return the latest IUpdatePreviewDiffJob for this MP."""
+ def getGitRefCopyOperations():
+ """Gets the list of GitHosting copy operations that should be done
+ in order to keep this MP's virtual refs up-to-date.
+
+ Note that, for now, the only possible virtual ref that could
+ possibly be copied is GIT_CREATE_MP_VIRTUAL_REF. So, this method
+ will return at most 1 copy operation. Once new virtual refs will be
+ created, this method should be extended to add more copy operations
+ too.
+
+ :return: A list of RefCopyOperation objects.
+ """
+
+ def getGitRefDeleteOperations(force_delete=False):
+ """Gets the list of git refs that should be deleted in order to keep
+ this MP's virtual refs up-to-date.
+
+ Note that, for now, the only existing virtual ref to be deleted is
+ GIT_CREATE_MP_VIRTUAL_REF. So this method will only ever delete at
+ most 1 virtual ref. Once new virtual refs will be created, this method
+ should be extended to delete them also.
+
+ :param force_delete: True if we should get delete operation
+ regardless of any check. False otherwise.
+ :return: A list of ref names.
+ """
+
+ def syncGitVirtualRefs(force_delete=False):
+ """Requests all copies and deletion of virtual refs to make git code
+ hosting in sync with this MP.
+
+ :param force_delete: Do not try to copy any new virtual ref. Just
+ delete all virtual refs possibly created.
+ """
+
+ def bulkSyncGitVirtualRefs(merge_proposals):
+ """Synchronizes a set of merge proposals' virtual refs.
+
+ :params merge_proposals: The set of merge proposals that should have
+ the virtual refs synced.
+ :return: A tuple with the list of (copy operations, delete operations)
+ executed.
+ """
+
class IBranchMergeProposalEdit(Interface):
diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
index 441e850..7fe0d0e 100644
--- a/lib/lp/code/interfaces/githosting.py
+++ b/lib/lp/code/interfaces/githosting.py
@@ -148,3 +148,11 @@ class IGitHostingClient(Interface):
:param refs: A list of tuples like (repo_path, ref_name) to be deleted.
:param logger: An optional logger.
"""
+
+ def bulkSyncRefs(copy_operations, delete_operations):
+ """Executes all copy operations and delete operations on Turnip
+ side.
+
+ :param copy_operations: The list of RefCopyOperation to run.
+ :param delete_operations: The list of RefDeleteOperation to run.
+ """
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 192133a..48e7dd3 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 list of ref copies requested to GitHosting.
"""
def markRecipesStale(paths):
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 061fb1e..821b15d 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
@@ -84,7 +85,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
@@ -94,6 +100,10 @@ from lp.code.model.diff import (
IncrementalDiff,
PreviewDiff,
)
+from lp.code.model.githosting import (
+ RefCopyOperation,
+ RefDeleteOperation,
+ )
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -123,6 +133,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
@@ -144,6 +155,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?
@@ -971,6 +985,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
branch_merge_proposal=self.id):
job.destroySelf()
self._preview_diffs.remove()
+
self.destroySelf()
def getUnlandedSourceBranchRevisions(self):
@@ -1217,6 +1232,89 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
for diff in diffs)
return [diff_dict.get(revisions) for revisions in revision_list]
+ @property
+ def should_have_git_virtual_refs(self):
+ """True if this MP should have virtual refs in the target
+ repository. False otherwise."""
+ # If the source repository is private and it's opening a MP to
+ # another repository, we should not risk disclosing the private code to
+ # everyone with permission to see the target repo:
+ private_source = self.source_git_repository.private
+ same_owner = self.source_git_repository.owner.inTeam(
+ self.target_git_repository.owner)
+ return not private_source or same_owner
+
+ def getGitRefCopyOperations(self):
+ """See `IBranchMergeProposal`"""
+ if (not self.target_git_repository
+ or not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF)):
+ # Not a git MP. Skip.
+ return []
+ if not self.should_have_git_virtual_refs:
+ return []
+ return [RefCopyOperation(
+ self.source_git_repository.getInternalPath(),
+ self.source_git_commit_sha1,
+ self.target_git_repository.getInternalPath(),
+ GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))]
+
+ def getGitRefDeleteOperations(self, force_delete=False):
+ """See `IBranchMergeProposal`"""
+ if self.source_git_ref is None:
+ # Not a git MP. Skip.
+ return []
+ if not force_delete and self.should_have_git_virtual_refs:
+ return []
+ return [RefDeleteOperation(
+ self.target_git_repository.getInternalPath(),
+ GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))]
+
+ def syncGitVirtualRefs(self, force_delete=False):
+ """See `IBranchMergeProposal`"""
+ 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
+ if force_delete:
+ copy_operations = []
+ else:
+ copy_operations = self.getGitRefCopyOperations()
+ delete_operations = self.getGitRefDeleteOperations(force_delete)
+
+ if copy_operations or delete_operations:
+ hosting_client = getUtility(IGitHostingClient)
+ hosting_client.bulkSyncRefs(copy_operations, delete_operations)
+
+ @classmethod
+ def bulkSyncGitVirtualRefs(cls, merge_proposals):
+ """See `IBranchMergeProposal`"""
+ 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 [], []
+ copy_operations = []
+ delete_operations = []
+ for merge_proposal in merge_proposals:
+ copy_operations += merge_proposal.getGitRefCopyOperations()
+ delete_operations += merge_proposal.getGitRefDeleteOperations()
+
+ if copy_operations or delete_operations:
+ hosting_client = getUtility(IGitHostingClient)
+ hosting_client.bulkSyncRefs(copy_operations, delete_operations)
+ return copy_operations, delete_operations
+
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 74f4ec0..b2632f6 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -7,9 +7,11 @@ __metaclass__ = type
__all__ = [
'GitHostingClient',
'RefCopyOperation',
+ 'RefDeleteOperation',
]
import base64
+from collections import defaultdict
import json
import sys
@@ -33,7 +35,6 @@ from lp.code.errors import (
GitRepositoryDeletionFault,
GitRepositoryScanFault,
GitTargetError,
- NoSuchGitReference,
)
from lp.code.interfaces.githosting import IGitHostingClient
from lp.services.config import config
@@ -55,12 +56,21 @@ class RefCopyOperation:
This class is just a helper to define copy operations parameters on
IGitHostingClient.copyRefs method.
"""
- def __init__(self, source_ref, target_repo, target_ref):
+ def __init__(self, source_repo_path, source_ref, target_repo_path,
+ target_ref):
+ self.source_repo_path = source_repo_path
self.source_ref = source_ref
- self.target_repo = target_repo
+ self.target_repo_path = target_repo_path
self.target_ref = target_ref
+class RefDeleteOperation:
+ """A description of a ref delete operation."""
+ def __init__(self, repo_path, ref):
+ self.repo_path = repo_path
+ self.ref = ref
+
+
@implementer(IGitHostingClient)
class GitHostingClient:
"""A client for the internal API provided by the Git hosting system."""
@@ -277,7 +287,7 @@ class GitHostingClient:
json_data = {
"operations": [{
"from": i.source_ref,
- "to": {"repo": i.target_repo, "ref": i.target_ref}
+ "to": {"repo": i.target_repo_path, "ref": i.target_ref}
} for i in operations]
}
try:
@@ -299,6 +309,8 @@ class GitHostingClient:
def deleteRefs(self, refs, logger=None):
"""See `IGitHostingClient`."""
+ # XXX pappacena: This needs to be done in a bulk operation,
+ # instead of several requests.
for path, ref in refs:
try:
if logger is not None:
@@ -309,3 +321,16 @@ class GitHostingClient:
raise GitReferenceDeletionFault(
"Error deleting %s from repo %s: HTTP %s" %
(ref, path, e.response.status_code))
+
+ def bulkSyncRefs(self, copy_operations, delete_operations):
+ """See `IGitHostingClient`."""
+ # XXX pappacena: This needs to be done in a bulk operation on Turnip
+ # side, instead of several requests.
+ operations_per_path = defaultdict(list)
+ for operation in copy_operations:
+ operations_per_path[operation.source_repo_path].append(operation)
+ for repo_path, operations in operations_per_path:
+ self.copyRefs(repo_path, operations)
+
+ self.deleteRefs([(i.repo_path, i.ref) for i in delete_operations])
+
diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py
index ba7b9f2..77135b3 100644
--- a/lib/lp/code/model/gitjob.py
+++ b/lib/lp/code/model/gitjob.py
@@ -12,6 +12,7 @@ __all__ = [
'ReclaimGitRepositorySpaceJob',
]
+from itertools import chain
from lazr.delegates import delegate_to
from lazr.enum import (
@@ -33,7 +34,10 @@ from zope.interface import (
provider,
)
-from lp.app.enums import InformationType
+from lp.app.enums import (
+ InformationType,
+ PRIVATE_INFORMATION_TYPES,
+ )
from lp.app.errors import NotFoundError
from lp.code.enums import (
GitActivityType,
@@ -440,6 +444,18 @@ class GitRepositoryTransitionToInformationTypeJob(GitJobDerived):
def information_type(self):
return InformationType.items[self.metadata["information_type"]]
+ def updateVirtualRefs(self):
+ copy_operations = []
+ delete_operations = []
+ merge_proposals = chain(
+ self.repository.landing_targets,
+ self.repository.landing_candidates)
+ for mp in merge_proposals:
+ copy_operations += mp.getGitRefCopyOperations()
+ delete_operations += mp.getGitRefDeleteOperations()
+ hosting_client = getUtility(IGitHostingClient)
+ hosting_client.bulkSyncRefs(copy_operations, delete_operations)
+
def run(self):
"""See `IGitRepositoryTransitionToInformationTypeJob`."""
if (self.repository.status !=
@@ -447,7 +463,17 @@ class GitRepositoryTransitionToInformationTypeJob(GitJobDerived):
raise AttributeError(
"The repository %s is not pending information type change." %
self.repository)
+ is_private = self.repository.private
+ will_be_private = self.information_type in PRIVATE_INFORMATION_TYPES
self.repository._transitionToInformationType(
self.information_type, self.user, self.verify_policy)
+
+ # When changing privacy type, we need to make sure to not leak
+ # anything though virtual refs, and create new ones that didn't
+ # exist before.
+ if is_private != will_be_private:
+ # XXX pappacena 2020-10-28: We need to implement retry rules here.
+ self.updateVirtualRefs()
+
self.repository.status = GitRepositoryStatus.AVAILABLE
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index fb506f1..731ca3f 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1205,9 +1205,12 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
def updateLandingTargets(self, paths):
"""See `IGitRepository`."""
jobs = []
- for merge_proposal in self.getActiveLandingTargets(paths):
+ merge_proposals = self.getActiveLandingTargets(paths)
+ for merge_proposal in merge_proposals:
jobs.extend(merge_proposal.scheduleDiffUpdates())
- return jobs
+ copy_ops, delete_ops = BranchMergeProposal.bulkSyncGitVirtualRefs(
+ merge_proposals)
+ return jobs, copy_ops, delete_ops
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 81b94b3..b601ad8 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -67,6 +67,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,
@@ -97,6 +101,7 @@ from lp.testing import (
ExpectedException,
launchpadlib_for,
login,
+ login_admin,
login_person,
person_logged_in,
TestCaseWithFactory,
@@ -256,6 +261,196 @@ 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_should_have_vrefs_public_repos(self):
+ mp = self.factory.makeBranchMergeProposalForGit()
+ self.assertTrue(mp.should_have_git_virtual_refs)
+
+ def test_should_have_vrefs_private_source(self):
+ 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.assertFalse(mp.should_have_git_virtual_refs)
+
+ def test_should_have_vrefs_private_target(self):
+ login_admin()
+ source_repo = self.factory.makeGitRepository()
+ target_repo = self.factory.makeGitRepository(
+ target=source_repo.target,
+ information_type=InformationType.PRIVATESECURITY)
+ [source] = self.factory.makeGitRefs(source_repo)
+ [target] = self.factory.makeGitRefs(target_repo)
+ mp = self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+ self.assertTrue(mp.should_have_git_virtual_refs)
+
+ def test_copy_git_merge_virtual_ref(self):
+ self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+ mp = self.factory.makeBranchMergeProposalForGit()
+
+ copy_operations = mp.getGitRefCopyOperations()
+ self.assertEqual(1, len(copy_operations))
+ self.assertThat(copy_operations[0], MatchesStructure(
+ source_repo_path=Equals(
+ mp.source_git_repository.getInternalPath()),
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo_path=Equals(
+ mp.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+ delete_operations = mp.getGitRefDeleteOperations()
+ self.assertEqual([], delete_operations)
+
+ self.assertEqual(1, self.hosting_fixture.bulkSyncRefs.call_count)
+ args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0]
+ copy_ops, delete_ops = args
+ self.assertEqual({}, kwargs)
+ self.assertEqual([], delete_ops)
+ self.assertEqual(1, len(copy_ops))
+ self.assertThat(copy_ops[0], MatchesStructure(
+ source_repo_path=Equals(
+ mp.source_git_repository.getInternalPath()),
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo_path=Equals(
+ mp.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+
+ def test_getGitRefCopyOperations_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.getGitRefCopyOperations())
+
+ delete_operations = mp.getGitRefDeleteOperations()
+ self.assertEqual(1, len(delete_operations))
+ self.assertThat(delete_operations[0], MatchesStructure(
+ repo_path=Equals(mp.target_git_repository.getInternalPath()),
+ ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+
+ def test_getGitRefCopyOperations_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.getGitRefCopyOperations()
+ self.assertEqual(1, len(operations))
+ self.assertThat(operations[0], MatchesStructure(
+ source_repo_path=Equals(
+ mp.source_git_repository.getInternalPath()),
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo_path=Equals(
+ mp.target_git_repository.getInternalPath()),
+ target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
+ ))
+
+ delete_operations = mp.getGitRefDeleteOperations()
+ self.assertEqual([], delete_operations)
+
+ def test_getGitRefCopyOperations_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.getGitRefCopyOperations()
+ self.assertEqual(1, len(operations))
+ self.assertThat(operations[0], MatchesStructure(
+ source_repo_path=Equals(
+ mp.source_git_repository.getInternalPath()),
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo_path=Equals(
+ mp.target_git_repository.getInternalPath()),
+ target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp))
+ ))
+
+ delete_operations = mp.getGitRefDeleteOperations()
+ self.assertEqual([], delete_operations)
+
+ def test_syncGitVirtualRefs(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.syncGitVirtualRefs should have been triggered by event.
+ # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
+ self.assertEqual(1, self.hosting_fixture.bulkSyncRefs.call_count)
+ args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0]
+ copy_ops, delete_ops = args
+ self.assertEquals({}, kwargs)
+ self.assertEqual([], delete_ops)
+ self.assertEqual(1, len(copy_ops))
+ self.assertThat(copy_ops[0], MatchesStructure(
+ source_repo_path=Equals(
+ mp.source_git_repository.getInternalPath()),
+ source_ref=Equals(mp.source_git_commit_sha1),
+ target_repo_path=Equals(
+ mp.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % mp.id),
+ ))
+
+ self.assertEqual(0, self.hosting_fixture.deleteRefs.call_count)
+
+ def test_syncGitVirtualRefs_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.syncGitVirtualRefs should have been triggered by event.
+ # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
+ self.assertEqual(1, self.hosting_fixture.bulkSyncRefs.call_count)
+ args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0]
+ copy_ops, delete_ops = args
+ self.assertEqual({}, kwargs)
+ self.assertEqual([], copy_ops)
+ self.assertEqual(1, len(delete_ops))
+ self.assertThat(delete_ops[0], MatchesStructure(
+ repo_path=Equals(target_repo.getInternalPath()),
+ ref=Equals("refs/merge/%s/head" % mp.id)))
+
+
class TestBranchMergeProposalTransitions(TestCaseWithFactory):
"""Test the state transitions of branch merge proposals."""
@@ -1185,6 +1380,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)
@@ -1203,10 +1402,25 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
expected_redacted_payload = dict(
expected_payload,
old=MatchesDict(self.getExpectedPayload(proposal, redact=True)))
+ hosting_fixture = self.useFixture(GitHostingFixture())
proposal.deleteProposal()
delivery = hook.deliveries.one()
+ self.assertIsNotNone(delivery)
self.assertCorrectDelivery(expected_payload, hook, delivery)
self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+ if not self.git:
+ self.assertEqual(0, hosting_fixture.bulkSyncRefs.call_count)
+ else:
+ self.assertEqual(1, hosting_fixture.bulkSyncRefs.call_count)
+ args, kwargs = hosting_fixture.bulkSyncRefs.calls[0]
+ self.assertEqual({}, kwargs)
+ copy_ops, delete_ops = args
+ self.assertEqual(0, len(copy_ops))
+ self.assertEqual(1, len(delete_ops))
+ self.assertThat(delete_ops[0], MatchesStructure(
+ repo_path=Equals(target.repository.getInternalPath()),
+ ref=Equals("refs/merge/%s/head" % proposal.id)
+ ))
class TestGetAddress(TestCaseWithFactory):
@@ -1507,6 +1721,26 @@ 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"}))
+ self.useFixture(GitHostingFixture())
+ proposal = self.factory.makeBranchMergeProposalForGit()
+
+ # Clean up fixture calls.
+ hosting_fixture = self.useFixture(GitHostingFixture())
+ proposal.deleteProposal()
+
+ self.assertEqual(1, hosting_fixture.bulkSyncRefs.call_count)
+ args, kwargs = hosting_fixture.bulkSyncRefs.calls[0]
+ self.assertEqual({}, kwargs)
+ copy_ops, delete_ops = args
+ self.assertEqual([], copy_ops)
+ self.assertEqual(1, len(delete_ops))
+ self.assertThat(delete_ops[0], MatchesStructure(
+ repo_path=Equals(proposal.target_git_repository.getInternalPath()),
+ ref=Equals('refs/merge/%s/head' % proposal.id)
+ ))
+
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 23d6063..fad0df3 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -42,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
@@ -414,8 +413,8 @@ class TestGitHostingClient(TestCase):
def getCopyRefOperations(self):
return [
- RefCopyOperation("1a2b3c4", "999", "refs/merge/123"),
- RefCopyOperation("9a8b7c6", "666", "refs/merge/989"),
+ RefCopyOperation("1", "1a2b3c4", "999", "refs/merge/123"),
+ RefCopyOperation("1", "9a8b7c6", "666", "refs/merge/989"),
]
def test_copyRefs(self):
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 08887f5..2296352 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -97,6 +97,7 @@ from lp.code.interfaces.gitnamespace import (
IGitNamespaceSet,
)
from lp.code.interfaces.gitrepository import (
+ GIT_CREATE_MP_VIRTUAL_REF,
IGitRepository,
IGitRepositorySet,
IGitRepositoryView,
@@ -783,6 +784,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.
@@ -824,7 +826,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
@@ -984,6 +985,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.
@@ -1115,7 +1117,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.
@@ -1126,7 +1127,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
@@ -1182,7 +1182,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
@@ -1521,6 +1520,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 = six.ensure_text(hashlib.sha1("").hexdigest())
@@ -1791,18 +1794,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
@@ -1875,9 +1877,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
@@ -1920,7 +1921,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,
@@ -1931,22 +1931,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(
@@ -1959,7 +1957,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):
@@ -2581,18 +2579,66 @@ 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(
- [bmp1.source_git_path])
+
+ # 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, ref_deletes = (
+ 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.bulkSyncRefs.call_count)
+ else:
+ self.assertEqual(
+ 1, self.hosting_fixture.bulkSyncRefs.call_count)
+ args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0]
+ self.assertEqual({}, kwargs)
+ self.assertEqual(2, len(args))
+ copy_ops, delete_ops = args
+ self.assertEqual(2, len(copy_ops))
+ self.assertEqual(0, len(delete_ops))
+ self.assertThat(copy_ops[0], MatchesStructure(
+ source_repo_path=Equals(
+ bmp1.source_git_repository.getInternalPath()),
+ source_ref=Equals(bmp1.source_git_commit_sha1),
+ target_repo_path=Equals(
+ bmp1.target_git_repository.getInternalPath()),
+ target_ref=Equals("refs/merge/%s/head" % bmp1.id),
+ ))
+ self.assertThat(copy_ops[1], MatchesStructure(
+ source_repo_path=Equals(
+ bmp2.source_git_repository.getInternalPath()),
+ source_ref=Equals(bmp2.source_git_commit_sha1),
+ target_repo_path=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(with_mp_virtual_ref=True)
+
+ def test_schedules_diff_updates_without_mp_virtual_ref(self):
+ self.assertSchedulesDiffUpdate(with_mp_virtual_ref=False)
+
def test_ignores_final(self):
"""Diffs for proposals in final states aren't updated."""
[source_ref] = self.factory.makeGitRefs()
@@ -2604,8 +2650,17 @@ 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, ref_deletes = (
+ source_ref.repository.updateLandingTargets(
+ [source_ref.path]))
self.assertEqual(0, len(jobs))
+ self.assertEqual(0, len(ref_copies))
+ self.assertEqual(0, len(ref_deletes))
+ 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..a34ca2a 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.syncGitVirtualRefs()
+
if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
payload = {
"action": "created",
@@ -149,3 +153,5 @@ def merge_proposal_deleted(merge_proposal, event):
"old": _compose_merge_proposal_webhook_payload(merge_proposal),
}
_trigger_webhook(merge_proposal, payload)
+
+ merge_proposal.syncGitVirtualRefs(force_delete=True)
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 4ef843d..3704b06 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -369,6 +369,9 @@ class GitHostingFixture(fixtures.Fixture):
self.getBlob = FakeMethod(result=blob)
self.delete = FakeMethod()
self.disable_memcache = disable_memcache
+ self.copyRefs = FakeMethod()
+ self.deleteRefs = FakeMethod()
+ self.bulkSyncRefs = FakeMethod()
def _setUp(self):
self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))
References
