launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18618
Re: [Merge] lp:~cjwatson/launchpad/git-repository-delete into lp:launchpad
Review: Approve code
Diff comments:
> === modified file 'database/schema/security.cfg'
> --- database/schema/security.cfg 2015-05-19 00:07:44 +0000
> +++ database/schema/security.cfg 2015-05-19 11:33:34 +0000
> @@ -2039,6 +2039,7 @@
> [reclaim-branch-space]
> groups=script
> public.branchjob = SELECT
> +public.gitjob = SELECT
> public.job = SELECT, UPDATE
> type=user
>
>
> === modified file 'lib/lp/code/configure.zcml'
> --- lib/lp/code/configure.zcml 2015-04-22 14:52:10 +0000
> +++ lib/lp/code/configure.zcml 2015-05-19 11:33:34 +0000
> @@ -204,7 +204,9 @@
> permission="launchpad.Edit"
> interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalEdit"
> set_attributes="description whiteboard merged_revno commit_message
> - root_message_id prerequisite_branch"/>
> + root_message_id prerequisite_branch
> + prerequisite_git_repository prerequisite_git_path
> + prerequisite_git_commit_sha1"/>
> <require
> permission="launchpad.AnyAllowedPerson"
> interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalAnyAllowedPerson"/>
> @@ -941,10 +943,19 @@
> provides="lp.code.interfaces.gitjob.IGitRefScanJobSource">
> <allow interface="lp.code.interfaces.gitjob.IGitRefScanJobSource" />
> </securedutility>
> + <securedutility
> + component="lp.code.model.gitjob.ReclaimGitRepositorySpaceJob"
> + provides="lp.code.interfaces.gitjob.IReclaimGitRepositorySpaceJobSource">
> + <allow interface="lp.code.interfaces.gitjob.IReclaimGitRepositorySpaceJobSource" />
> + </securedutility>
> <class class="lp.code.model.gitjob.GitRefScanJob">
> <allow interface="lp.code.interfaces.gitjob.IGitJob" />
> <allow interface="lp.code.interfaces.gitjob.IGitRefScanJob" />
> </class>
> + <class class="lp.code.model.gitjob.ReclaimGitRepositorySpaceJob">
> + <allow interface="lp.code.interfaces.gitjob.IGitJob" />
> + <allow interface="lp.code.interfaces.gitjob.IReclaimGitRepositorySpaceJob" />
> + </class>
>
> <lp:help-folder folder="help" name="+help-code" />
>
>
> === modified file 'lib/lp/code/errors.py'
> --- lib/lp/code/errors.py 2015-05-14 13:57:51 +0000
> +++ lib/lp/code/errors.py 2015-05-19 11:33:34 +0000
> @@ -20,6 +20,7 @@
> 'BuildNotAllowedForDistro',
> 'BranchMergeProposalExists',
> 'CannotDeleteBranch',
> + 'CannotDeleteGitRepository',
> 'CannotHaveLinkedBranch',
> 'CannotUpgradeBranch',
> 'CannotUpgradeNonHosted',
> @@ -34,6 +35,7 @@
> 'GitRepositoryCreationForbidden',
> 'GitRepositoryCreatorNotMemberOfOwnerTeam',
> 'GitRepositoryCreatorNotOwner',
> + 'GitRepositoryDeletionFault',
> 'GitRepositoryExists',
> 'GitRepositoryScanFault',
> 'GitTargetError',
> @@ -347,6 +349,11 @@
> GitRepositoryCreationException.__init__(self, message)
>
>
> +@error_status(httplib.BAD_REQUEST)
> +class CannotDeleteGitRepository(Exception):
> + """The Git repository cannot be deleted at this time."""
> +
> +
> class GitRepositoryCreationForbidden(GitRepositoryCreationException):
> """A visibility policy forbids Git repository creation.
>
> @@ -381,6 +388,10 @@
> """Raised when there is a fault scanning a repository."""
>
>
> +class GitRepositoryDeletionFault(Exception):
> + """Raised when there is a fault deleting a repository."""
> +
> +
> class GitTargetError(Exception):
> """Raised when there is an error determining a Git repository target."""
>
>
> === modified file 'lib/lp/code/githosting.py'
> --- lib/lp/code/githosting.py 2015-04-30 17:10:28 +0000
> +++ lib/lp/code/githosting.py 2015-05-19 11:33:34 +0000
> @@ -15,6 +15,7 @@
>
> from lp.code.errors import (
> GitRepositoryCreationFault,
> + GitRepositoryDeletionFault,
> GitRepositoryScanFault,
> )
>
> @@ -127,9 +128,23 @@
> if response.status_code != 200:
> raise GitRepositoryScanFault(
> "Failed to get merge diff from Git repository: %s" %
> - unicode(e))
> + response.text)
> try:
> return response.json()
> except ValueError as e:
> raise GitRepositoryScanFault(
> "Failed to decode merge-diff response: %s" % unicode(e))
> +
> + def delete(self, path, logger=None):
> + """Delete a repository."""
> + try:
> + if logger is not None:
> + logger.info("Deleting repository %s" % path)
> + response = self._makeSession().delete(
> + urljoin(self.endpoint, "/repo/%s" % path))
> + except Exception as e:
> + raise GitRepositoryDeletionFault(
> + "Failed to delete Git repository: %s" % unicode(e))
> + if response.status_code != 200:
> + raise GitRepositoryDeletionFault(
> + "Failed to delete Git repository: %s" % response.text)
>
> === modified file 'lib/lp/code/interfaces/branch.py'
> --- lib/lp/code/interfaces/branch.py 2015-04-30 21:49:59 +0000
> +++ lib/lp/code/interfaces/branch.py 2015-05-19 11:33:34 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Branch interfaces."""
> @@ -1167,15 +1167,9 @@
> branch.
> """
>
> + @call_with(break_references=True)
> @export_destructor_operation()
> @operation_for_version('beta')
> - def destroySelfBreakReferences():
> - """Delete the specified branch.
> -
> - BranchRevisions associated with this branch will also be deleted as
> - well as any items with mandatory references.
> - """
> -
> def destroySelf(break_references=False):
> """Delete the specified branch.
>
>
> === modified file 'lib/lp/code/interfaces/gitjob.py'
> --- lib/lp/code/interfaces/gitjob.py 2015-03-12 15:21:27 +0000
> +++ lib/lp/code/interfaces/gitjob.py 2015-05-19 11:33:34 +0000
> @@ -9,6 +9,8 @@
> 'IGitJob',
> 'IGitRefScanJob',
> 'IGitRefScanJobSource',
> + 'IReclaimGitRepositorySpaceJob',
> + 'IReclaimGitRepositorySpaceJobSource',
> ]
>
> from lazr.restful.fields import Reference
> @@ -16,6 +18,7 @@
> Attribute,
> Interface,
> )
> +from zope.schema import Text
>
> from lp import _
> from lp.code.interfaces.gitrepository import IGitRepository
> @@ -35,7 +38,7 @@
>
> repository = Reference(
> title=_("The Git repository to use for this job."),
> - schema=IGitRepository, required=True, readonly=True)
> + schema=IGitRepository, required=False, readonly=True)
>
> metadata = Attribute(_("A dict of data about the job."))
>
> @@ -51,3 +54,24 @@
>
> :param repository: The database repository to scan.
> """
> +
> +
> +class IReclaimGitRepositorySpaceJob(IRunnableJob):
> + """A Job that deletes a repository from storage after it has been
> + deleted from the database."""
> +
> + repository_path = Text(
> + title=_("The storage path of the now-deleted repository."))
> +
> +
> +class IReclaimGitRepositorySpaceJobSource(IJobSource):
> +
> + def create(repository_name, repository_path):
> + """Construct a new object that implements
> + IReclaimGitRepositorySpaceJob.
> +
> + :param repository_name: The unique name of the repository to remove
> + from storage.
> + :param repository_path: The storage path of the repository to remove
> + from storage.
> + """
>
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py 2015-05-14 13:57:51 +0000
> +++ lib/lp/code/interfaces/gitrepository.py 2015-05-19 11:33:34 +0000
> @@ -449,6 +449,18 @@
> and their subscriptions.
> """
>
> + # XXX cjwatson 2015-04-16: These names are too awful to set in stone by
> + # exporting them on the webservice; find better names before exporting.
> + landing_targets = Attribute(
> + "A collection of the merge proposals where this repository is the "
> + "source.")
> + landing_candidates = Attribute(
> + "A collection of the merge proposals where this repository is the "
> + "target.")
> + dependent_landings = Attribute(
> + "A collection of the merge proposals that are dependent on this "
> + "repository.")
> +
> def isRepositoryMergeable(other):
> """Is the other repository mergeable into this one (or vice versa)?"""
>
> @@ -532,10 +544,35 @@
> def setTarget(target, user):
> """Set the target of the repository."""
>
> + @export_read_operation()
> + @operation_for_version("devel")
> + def canBeDeleted():
> + """Can this repository be deleted in its current state?
> +
> + A repository is considered deletable if it is not linked to any
> + merge proposals.
> + """
> +
> + def getDeletionRequirements():
> + """Determine what is required to delete this branch.
> +
> + :return: a dict of {object: (operation, reason)}, where object is the
> + object that must be deleted or altered, operation is either
> + "delete" or "alter", and reason is a string explaining why the
> + object needs to be touched.
> + """
> +
> + @call_with(break_references=True)
> @export_destructor_operation()
> @operation_for_version("devel")
> - def destroySelf():
> - """Delete the specified repository."""
> + def destroySelf(break_references=False):
> + """Delete the specified repository.
> +
> + :param break_references: If supplied, break any references to this
> + repository by deleting items with mandatory references and
> + NULLing other references.
> + :raise: CannotDeleteGitRepository if the repository cannot be deleted.
> + """
>
>
> class IGitRepository(IGitRepositoryView, IGitRepositoryModerateAttributes,
>
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py 2015-04-30 21:49:59 +0000
> +++ lib/lp/code/model/branch.py 2015-05-19 11:33:34 +0000
> @@ -1250,10 +1250,6 @@
> datetime.now(pytz.timezone('UTC'))
> + increment * 2 ** (self.mirror_failures - 1))
>
> - def destroySelfBreakReferences(self):
> - """See `IBranch`."""
> - return self.destroySelf(break_references=True)
> -
> def _deleteBranchSubscriptions(self):
> """Delete subscriptions for this branch prior to deleting branch."""
> subscriptions = Store.of(self).find(
>
> === modified file 'lib/lp/code/model/gitjob.py'
> --- lib/lp/code/model/gitjob.py 2015-05-14 09:46:25 +0000
> +++ lib/lp/code/model/gitjob.py 2015-05-19 11:33:34 +0000
> @@ -7,6 +7,7 @@
> 'GitJob',
> 'GitJobType',
> 'GitRefScanJob',
> + 'ReclaimGitRepositorySpaceJob',
> ]
>
> from lazr.delegates import delegates
> @@ -19,6 +20,7 @@
> Int,
> JSON,
> Reference,
> + SQL,
> Store,
> )
> from zope.interface import (
> @@ -32,6 +34,8 @@
> IGitJob,
> IGitRefScanJob,
> IGitRefScanJobSource,
> + IReclaimGitRepositorySpaceJob,
> + IReclaimGitRepositorySpaceJobSource,
> )
> from lp.services.config import config
> from lp.services.database.enumcol import EnumCol
> @@ -63,6 +67,13 @@
> This job scans a repository for its current list of references.
> """)
>
> + RECLAIM_REPOSITORY_SPACE = DBItem(1, """
> + Reclaim repository space
> +
> + This job removes a repository that has been deleted from the
> + database from storage.
> + """)
> +
>
> class GitJob(StormBase):
> """See `IGitJob`."""
> @@ -74,7 +85,7 @@
> job_id = Int(name='job', primary=True, allow_none=False)
> job = Reference(job_id, 'Job.id')
>
> - repository_id = Int(name='repository', allow_none=False)
> + repository_id = Int(name='repository', allow_none=True)
> repository = Reference(repository_id, 'GitRepository.id')
>
> job_type = EnumCol(enum=GitJobType, notNull=True)
> @@ -97,7 +108,8 @@
> self.repository = repository
> self.job_type = job_type
> self.metadata = metadata
> - self.metadata["repository_name"] = repository.unique_name
> + if repository is not None:
> + self.metadata["repository_name"] = repository.unique_name
>
> def makeDerived(self):
> return GitJobDerived.makeSubclass(self)
> @@ -205,3 +217,43 @@
> log.info(
> "Skipping repository %s because it has been deleted." %
> self._cached_repository_name)
> +
> +
> +class ReclaimGitRepositorySpaceJob(GitJobDerived):
> + """A Job that deletes a repository from storage after it has been
> + deleted from the database."""
> +
> + implements(IReclaimGitRepositorySpaceJob)
> +
> + classProvides(IReclaimGitRepositorySpaceJobSource)
> + class_job_type = GitJobType.RECLAIM_REPOSITORY_SPACE
> +
> + config = config.IReclaimGitRepositorySpaceJobSource
> +
> + @classmethod
> + def create(cls, repository_name, repository_path):
> + "See `IReclaimGitRepositorySpaceJobSource`."""
> + metadata = {
> + "repository_name": repository_name,
> + "repository_path": repository_path,
> + }
> + # The GitJob has a repository of None, as there is no repository
> + # left in the database to refer to.
> + start = SQL("CURRENT_TIMESTAMP AT TIME ZONE 'UTC' + '7 days'")
> + git_job = GitJob(
> + None, cls.class_job_type, metadata, scheduled_start=start)
Does scheduled_start actually work with celery? If not, we probably need a non-celery runner for these.
> + job = cls(git_job)
> + job.celeryRunOnCommit()
> + return job
> +
> + def __init__(self, git_job):
> + super(ReclaimGitRepositorySpaceJob, self).__init__(git_job)
> + self._hosting_client = GitHostingClient(
> + config.codehosting.internal_git_api_endpoint)
Any reason not to do this in run()?
> +
> + @property
> + def repository_path(self):
> + return self.metadata["repository_path"]
> +
> + def run(self):
> + self._hosting_client.delete(self.repository_path, logger=log)
>
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py 2015-05-14 13:57:51 +0000
> +++ lib/lp/code/model/gitrepository.py 2015-05-19 11:33:34 +0000
> @@ -20,6 +20,7 @@
> Coalesce,
> Insert,
> Join,
> + Not,
> Or,
> Select,
> SQL,
> @@ -39,8 +40,12 @@
> from zope.component import getUtility
> from zope.interface import implements
> from zope.security.interfaces import Unauthorized
> -from zope.security.proxy import removeSecurityProxy
> +from zope.security.proxy import (
> + ProxyFactory,
> + removeSecurityProxy,
> + )
>
> +from lp import _ as msg
> from lp.app.enums import (
> InformationType,
> PRIVATE_INFORMATION_TYPES,
> @@ -58,9 +63,13 @@
> from lp.app.interfaces.services import IService
> from lp.code.enums import GitObjectType
> from lp.code.errors import (
> + CannotDeleteGitRepository,
> GitDefaultConflict,
> GitTargetError,
> )
> +from lp.code.interfaces.branchmergeproposal import (
> + BRANCH_MERGE_PROPOSAL_FINAL_STATES,
> + )
> from lp.code.interfaces.gitcollection import (
> IAllGitRepositories,
> IGitCollection,
> @@ -78,6 +87,7 @@
> )
> from lp.code.interfaces.revision import IRevisionSet
> from lp.code.mail.branch import send_git_repository_modified_notifications
> +from lp.code.model.branchmergeproposal import BranchMergeProposal
> from lp.code.model.gitref import GitRef
> from lp.code.model.gitsubscription import GitSubscription
> from lp.registry.enums import PersonVisibility
> @@ -756,6 +766,31 @@
> recipients.add(subscription.person, subscription, rationale)
> return recipients
>
> + @property
> + def landing_targets(self):
> + """See `IGitRepository`."""
> + return Store.of(self).find(
> + BranchMergeProposal,
> + BranchMergeProposal.source_git_repository == self)
> +
> + @property
> + def landing_candidates(self):
> + """See `IGitRepository`."""
> + return Store.of(self).find(
> + BranchMergeProposal,
> + BranchMergeProposal.target_git_repository == self,
> + Not(BranchMergeProposal.queue_status.is_in(
> + BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
> +
> + @property
> + def dependent_landings(self):
> + """See `IGitRepository`."""
> + return Store.of(self).find(
> + BranchMergeProposal,
> + BranchMergeProposal.prerequisite_git_repository,
> + Not(BranchMergeProposal.queue_status.is_in(
> + BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
> +
> def isRepositoryMergeable(self, other):
> """See `IGitRepository`."""
> return self.namespace.areRepositoriesMergeable(other.namespace)
> @@ -775,8 +810,164 @@
> Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
> return not jobs.is_empty()
>
> - def destroySelf(self):
> - raise NotImplementedError
> + def canBeDeleted(self):
> + """See `IGitRepository`."""
> + # Can't delete if the repository is associated with anything.
> + return len(self.getDeletionRequirements()) == 0
> +
> + def _getDeletionRequirements(self):
> + """Determine what operations must be performed to delete this branch.
> +
> + Two dictionaries are returned, one for items that must be deleted,
> + one for items that must be altered. The item in question is the
> + key, and the value is a user-facing string explaining why the item
> + is affected.
> +
> + As well as the dictionaries, this method returns two list of callables
> + that may be called to perform the alterations and deletions needed.
> + """
> + alteration_operations = []
> + deletion_operations = []
> + # Merge proposals require their source and target repositories to
> + # exist.
> + for merge_proposal in self.landing_targets:
> + deletion_operations.append(
> + DeletionCallable(
> + merge_proposal,
> + msg("This repository is the source repository of this "
> + "merge proposal."),
> + merge_proposal.deleteProposal))
> + # Cannot use self.landing_candidates, because it ignores merged
> + # merge proposals.
> + for merge_proposal in BranchMergeProposal.selectBy(
> + target_git_repository=self):
> + deletion_operations.append(
> + DeletionCallable(
> + merge_proposal,
> + msg("This repository is the target repository of this "
> + "merge proposal."),
> + merge_proposal.deleteProposal))
> + for merge_proposal in BranchMergeProposal.selectBy(
> + prerequisite_git_repository=self):
> + alteration_operations.append(
> + ClearPrerequisiteRepository(merge_proposal))
> +
> + return (alteration_operations, deletion_operations)
> +
> + def getDeletionRequirements(self):
> + """See `IGitRepository`."""
> + alteration_operations, deletion_operations = (
> + self._getDeletionRequirements())
> + result = {
> + operation.affected_object: ("alter", operation.rationale)
> + for operation in alteration_operations}
> + # Deletion entries should overwrite alteration entries.
> + result.update({
> + operation.affected_object: ("delete", operation.rationale)
> + for operation in deletion_operations})
> + return result
> +
> + def _breakReferences(self):
> + """Break all external references to this repository.
> +
> + NULLable references will be NULLed. References which are not NULLable
> + will cause the item holding the reference to be deleted.
> +
> + This function is guaranteed to perform the operations predicted by
> + getDeletionRequirements, because it uses the same backing function.
> + """
> + alteration_operations, deletion_operations = (
> + self._getDeletionRequirements())
> + for operation in alteration_operations:
> + operation()
> + for operation in deletion_operations:
> + operation()
> + Store.of(self).flush()
> +
> + def _deleteRepositorySubscriptions(self):
> + """Delete subscriptions for this repository prior to deleting it."""
> + subscriptions = Store.of(self).find(
> + GitSubscription, GitSubscription.repository == self)
> + subscriptions.remove()
> +
> + def _deleteJobs(self):
> + """Delete jobs for this repository prior to deleting it.
> +
> + This deletion includes `GitJob`s associated with the branch.
> + """
> + # Circular import.
> + from lp.code.model.gitjob import GitJob
> +
> + # Remove GitJobs.
> + affected_jobs = Select(
> + [GitJob.job_id],
> + And(GitJob.job == Job.id, GitJob.repository == self))
> + Store.of(self).find(Job, Job.id.is_in(affected_jobs)).remove()
> +
> + def destroySelf(self, break_references=False):
> + """See `IGitRepository`."""
> + # Circular import.
> + from lp.code.interfaces.gitjob import (
> + IReclaimGitRepositorySpaceJobSource,
> + )
> +
> + if break_references:
> + self._breakReferences()
> + if not self.canBeDeleted():
> + raise CannotDeleteGitRepository(
> + "Cannot delete Git repository: %s" % self.unique_name)
> +
> + self.refs.remove()
> + self._deleteRepositorySubscriptions()
> + self._deleteJobs()
> +
> + # Now destroy the repository.
> + repository_name = self.unique_name
> + repository_path = self.getInternalPath()
> + Store.of(self).remove(self)
> + # And now create a job to remove the repository from storage when
> + # it's done.
> + getUtility(IReclaimGitRepositorySpaceJobSource).create(
> + repository_name, repository_path)
> +
> +
> +class DeletionOperation:
> + """Represent an operation to perform as part of branch deletion."""
> +
> + def __init__(self, affected_object, rationale):
> + self.affected_object = ProxyFactory(affected_object)
> + self.rationale = rationale
> +
> + def __call__(self):
> + """Perform the deletion operation."""
> + raise NotImplementedError(DeletionOperation.__call__)
> +
> +
> +class DeletionCallable(DeletionOperation):
> + """Deletion operation that invokes a callable."""
> +
> + def __init__(self, affected_object, rationale, func):
> + super(DeletionCallable, self).__init__(affected_object, rationale)
> + self.func = func
> +
> + def __call__(self):
> + self.func()
> +
> +
> +class ClearPrerequisiteRepository(DeletionOperation):
> + """Delete operation that clears a merge proposal's prerequisite
> + repository."""
> +
> + def __init__(self, merge_proposal):
> + DeletionOperation.__init__(
> + self, merge_proposal,
> + msg("This repository is the prerequisite repository of this merge "
> + "proposal."))
> +
> + def __call__(self):
> + self.affected_object.prerequisite_git_repository = None
> + self.affected_object.prerequisite_git_path = None
> + self.affected_object.prerequisite_git_commit_sha1 = None
>
>
> class GitRepositorySet:
>
> === modified file 'lib/lp/code/model/tests/test_gitjob.py'
> --- lib/lp/code/model/tests/test_gitjob.py 2015-05-14 13:57:51 +0000
> +++ lib/lp/code/model/tests/test_gitjob.py 2015-05-19 11:33:34 +0000
> @@ -16,18 +16,22 @@
> MatchesSetwise,
> MatchesStructure,
> )
> +from zope.security.proxy import removeSecurityProxy
>
> from lp.code.enums import GitObjectType
> from lp.code.interfaces.gitjob import (
> IGitJob,
> IGitRefScanJob,
> + IReclaimGitRepositorySpaceJob,
> )
> from lp.code.model.gitjob import (
> GitJob,
> GitJobDerived,
> GitJobType,
> GitRefScanJob,
> + ReclaimGitRepositorySpaceJob,
> )
> +from lp.services.database.constants import UTC_NOW
> from lp.testing import (
> TestCaseWithFactory,
> time_counter,
> @@ -65,7 +69,10 @@
> self.assertIsNone(derived.getOopsMailController("x"))
>
>
> -class TestGitRefScanJobMixin:
> +class TestGitRefScanJob(TestCaseWithFactory):
> + """Tests for `GitRefScanJob`."""
> +
> + layer = LaunchpadZopelessLayer
>
> @staticmethod
> def makeFakeRefs(paths):
> @@ -107,12 +114,6 @@
> for path in paths]
> self.assertThat(refs, MatchesSetwise(*matchers))
>
> -
> -class TestGitRefScanJob(TestGitRefScanJobMixin, TestCaseWithFactory):
> - """Tests for `GitRefScanJob`."""
> -
> - layer = LaunchpadZopelessLayer
> -
> def test_provides_interface(self):
> # `GitRefScanJob` objects provide `IGitRefScanJob`.
> repository = self.factory.makeGitRepository()
> @@ -156,5 +157,64 @@
> self.assertEqual([], list(repository.refs))
>
>
> -# XXX cjwatson 2015-03-12: We should test that the job works via Celery too,
> +class TestReclaimGitRepositorySpaceJob(TestCaseWithFactory):
> + """Tests for `ReclaimGitRepositorySpaceJob`."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def test_provides_interface(self):
> + # `ReclaimGitRepositorySpaceJob` objects provide
> + # `IReclaimGitRepositorySpaceJob`.
> + self.assertProvides(
> + ReclaimGitRepositorySpaceJob.create("/~owner/+git/gone", "1"),
> + IReclaimGitRepositorySpaceJob)
> +
> + def test___repr__(self):
> + # `ReclaimGitRepositorySpaceJob` objects have an informative
> + # __repr__.
> + name = "/~owner/+git/gone"
> + job = ReclaimGitRepositorySpaceJob.create(name, "1")
> + self.assertEqual(
> + "<ReclaimGitRepositorySpaceJob for %s>" % name, repr(job))
> +
> + def test_scheduled_in_future(self):
> + # A freshly created ReclaimGitRepositorySpaceJob is scheduled to run
> + # in a week's time.
> + job = ReclaimGitRepositorySpaceJob.create("/~owner/+git/gone", "1")
> + self.assertEqual(
> + timedelta(days=7), job.job.scheduled_start - job.job.date_created)
> +
> + def test_stores_name_and_path(self):
> + # An instance of ReclaimGitRepositorySpaceJob stores the name and
> + # path of the repository that has been deleted.
> + name = "/~owner/+git/gone"
> + path = "1"
> + job = ReclaimGitRepositorySpaceJob.create(name, path)
> + self.assertEqual(name, job._cached_repository_name)
> + self.assertEqual(path, job.repository_path)
> +
> + def makeJobReady(self, job):
> + """Force `job` to be scheduled to run now.
> +
> + New `ReclaimGitRepositorySpaceJob`s are scheduled to run a week
> + after creation, so to be able to test running the job we have to
> + force them to be scheduled now.
> + """
> + removeSecurityProxy(job).job.scheduled_start = UTC_NOW
> +
> + def test_run(self):
> + # Running a job to reclaim space sends a request to the hosting
> + # service.
> + name = "/~owner/+git/gone"
> + path = "1"
> + job = ReclaimGitRepositorySpaceJob.create(name, path)
> + self.makeJobReady(job)
> + [job] = list(ReclaimGitRepositorySpaceJob.iterReady())
> + with dbuser("reclaim-branch-space"):
> + job._hosting_client.delete = FakeMethod()
> + job.run()
> + self.assertEqual([(path,)], job._hosting_client.delete.extract_args())
> +
> +
> +# XXX cjwatson 2015-03-12: We should test that the jobs work via Celery too,
> # but that isn't feasible until we have a proper turnip fixture.
>
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py 2015-05-14 13:57:51 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py 2015-05-19 11:33:34 +0000
> @@ -14,6 +14,8 @@
> from bzrlib import urlutils
> from lazr.lifecycle.event import ObjectModifiedEvent
> from lazr.lifecycle.snapshot import Snapshot
> +from sqlobject import SQLObjectNotFound
> +from storm.store import Store
> import transaction
> import pytz
> from testtools.matchers import (
> @@ -27,6 +29,7 @@
> from zope.security.interfaces import Unauthorized
> from zope.security.proxy import removeSecurityProxy
>
> +from lp import _
> from lp.app.enums import (
> InformationType,
> PRIVATE_INFORMATION_TYPES,
> @@ -40,6 +43,7 @@
> GitObjectType,
> )
> from lp.code.errors import (
> + CannotDeleteGitRepository,
> GitRepositoryCreatorNotMemberOfOwnerTeam,
> GitRepositoryCreatorNotOwner,
> GitRepositoryExists,
> @@ -47,6 +51,7 @@
> )
> from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
> from lp.code.interfaces.gitjob import IGitRefScanJobSource
> +from lp.code.interfaces.gitlookup import IGitLookup
> from lp.code.interfaces.gitnamespace import (
> IGitNamespacePolicy,
> IGitNamespaceSet,
> @@ -56,7 +61,19 @@
> IGitRepositorySet,
> )
> from lp.code.interfaces.revision import IRevisionSet
> -from lp.code.model.gitrepository import GitRepository
> +from lp.code.model.branchmergeproposal import BranchMergeProposal
> +from lp.code.model.codereviewcomment import CodeReviewComment
> +from lp.code.model.gitjob import (
> + GitJob,
> + GitJobType,
> + ReclaimGitRepositorySpaceJob,
> + )
> +from lp.code.model.gitrepository import (
> + ClearPrerequisiteRepository,
> + DeletionCallable,
> + DeletionOperation,
> + GitRepository,
> + )
> from lp.code.xmlrpc.git import GitAPI
> from lp.registry.enums import (
> BranchSharingPolicy,
> @@ -84,6 +101,7 @@
> ANONYMOUS,
> api_url,
> celebrity_logged_in,
> + login_person,
> person_logged_in,
> TestCaseWithFactory,
> verifyObject,
> @@ -93,6 +111,7 @@
> from lp.testing.layers import (
> DatabaseFunctionalLayer,
> LaunchpadFunctionalLayer,
> + LaunchpadZopelessLayer,
> )
> from lp.testing.pages import webservice_for_person
>
> @@ -284,6 +303,305 @@
> repository.getRepositoryIdentities())
>
>
> +class TestGitRepositoryDeletion(TestCaseWithFactory):
> + """Test the different cases that make a repository deletable or not."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + super(TestGitRepositoryDeletion, self).setUp()
> + self.user = self.factory.makePerson()
> + self.project = self.factory.makeProduct(owner=self.user)
> + self.repository = self.factory.makeGitRepository(
> + name=u"to-delete", owner=self.user, target=self.project)
> + [self.ref] = self.factory.makeGitRefs(repository=self.repository)
> + # The owner of the repository is subscribed to the repository when
> + # it is created. The tests here assume no initial connections, so
> + # unsubscribe the repository owner here.
> + self.repository.unsubscribe(
> + self.repository.owner, self.repository.owner)
> + # Make sure that the tests all flush the database changes.
> + self.addCleanup(Store.of(self.repository).flush)
> + login_person(self.user)
> +
> + def test_deletable(self):
> + # A newly created repository can be deleted without any problems.
> + self.assertTrue(
> + self.repository.canBeDeleted(),
> + "A newly created repository should be able to be deleted.")
> + repository_id = self.repository.id
> + self.repository.destroySelf()
> + self.assertIsNone(
> + getUtility(IGitLookup).get(repository_id),
> + "The repository has not been deleted.")
> +
> + def test_subscription_does_not_disable_deletion(self):
> + # A repository that has a subscription can be deleted.
> + self.repository.subscribe(
> + self.user, BranchSubscriptionNotificationLevel.NOEMAIL, None,
> + CodeReviewNotificationLevel.NOEMAIL, self.user)
> + self.assertTrue(self.repository.canBeDeleted())
> +
> + def test_landing_target_disables_deletion(self):
> + # A repository with a landing target cannot be deleted.
> + [merge_target] = self.factory.makeGitRefs(
> + name=u"landing-target", owner=self.user, target=self.project)
> + self.ref.addLandingTarget(self.user, merge_target)
> + self.assertFalse(
> + self.repository.canBeDeleted(),
> + "A repository with a landing target is not deletable.")
> + self.assertRaises(
> + CannotDeleteGitRepository, self.repository.destroySelf)
> +
> + def test_landing_candidate_disables_deletion(self):
> + # A repository with a landing candidate cannot be deleted.
> + [merge_source] = self.factory.makeGitRefs(
> + name=u"landing-candidate", owner=self.user, target=self.project)
> + merge_source.addLandingTarget(self.user, self.ref)
> + self.assertFalse(
> + self.repository.canBeDeleted(),
> + "A repository with a landing candidate is not deletable.")
> + self.assertRaises(
> + CannotDeleteGitRepository, self.repository.destroySelf)
> +
> + def test_prerequisite_repository_disables_deletion(self):
> + # A repository that is a prerequisite repository cannot be deleted.
> + [merge_source] = self.factory.makeGitRefs(
> + name=u"landing-candidate", owner=self.user, target=self.project)
> + [merge_target] = self.factory.makeGitRefs(
> + name=u"landing-target", owner=self.user, target=self.project)
> + merge_source.addLandingTarget(self.user, merge_target, self.ref)
> + self.assertFalse(
> + self.repository.canBeDeleted(),
> + "A repository with a prerequisite target is not deletable.")
> + self.assertRaises(
> + CannotDeleteGitRepository, self.repository.destroySelf)
> +
> + def test_related_GitJobs_deleted(self):
> + # A repository with an associated job will delete those jobs.
> + repository = self.factory.makeGitRepository()
> + GitAPI(None, None).notify(repository.getInternalPath())
> + store = Store.of(repository)
> + repository.destroySelf()
> + # Need to commit the transaction to fire off the constraint checks.
> + transaction.commit()
> + jobs = store.find(GitJob, GitJob.job_type == GitJobType.REF_SCAN)
> + self.assertEqual([], list(jobs))
> +
> + def test_creates_job_to_reclaim_space(self):
> + # When a repository is deleted from the database, a job is created
> + # to remove the repository from disk as well.
> + repository = self.factory.makeGitRepository()
> + repository_path = repository.getInternalPath()
> + store = Store.of(repository)
> + repository.destroySelf()
> + jobs = store.find(
> + GitJob,
> + GitJob.job_type == GitJobType.RECLAIM_REPOSITORY_SPACE)
> + self.assertEqual(
> + [repository_path],
> + [ReclaimGitRepositorySpaceJob(job).repository_path
> + for job in jobs])
> +
> + def test_destroySelf_with_inline_comments_draft(self):
> + # Draft inline comments related to a deleted repository (source or
> + # target MP repository) also get removed.
> + merge_proposal = self.factory.makeBranchMergeProposalForGit(
> + registrant=self.user, target_ref=self.ref)
> + preview_diff = self.factory.makePreviewDiff(
> + merge_proposal=merge_proposal)
> + transaction.commit()
> + merge_proposal.saveDraftInlineComment(
> + previewdiff_id=preview_diff.id,
> + person=self.user,
> + comments={"1": "Should vanish."})
> + self.repository.destroySelf(break_references=True)
> +
> + def test_destroySelf_with_inline_comments_published(self):
> + # Published inline comments related to a deleted repository (source
> + # or target MP repository) also get removed.
> + merge_proposal = self.factory.makeBranchMergeProposalForGit(
> + registrant=self.user, target_ref=self.ref)
> + preview_diff = self.factory.makePreviewDiff(
> + merge_proposal=merge_proposal)
> + transaction.commit()
> + merge_proposal.createComment(
> + owner=self.user,
> + subject="Delete me!",
> + previewdiff_id=preview_diff.id,
> + inline_comments={"1": "Must disappear."},
> + )
> + self.repository.destroySelf(break_references=True)
> +
> +
> +class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
> + """Test determination and application of repository deletion
> + consequences."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + super(TestGitRepositoryDeletionConsequences, self).setUp(
> + user="test@xxxxxxxxxxxxx")
> + self.repository = self.factory.makeGitRepository()
> + [self.ref] = self.factory.makeGitRefs(repository=self.repository)
> + # The owner of the repository is subscribed to the repository when
> + # it is created. The tests here assume no initial connections, so
> + # unsubscribe the repository owner here.
> + self.repository.unsubscribe(
> + self.repository.owner, self.repository.owner)
> +
> + def test_plain_repository(self):
> + # A fresh repository has no deletion requirements.
> + self.assertEqual({}, self.repository.getDeletionRequirements())
> +
> + def makeMergeProposals(self):
> + # Produce a merge proposal for testing purposes.
> + [merge_target] = self.factory.makeGitRefs(target=self.ref.target)
> + [merge_prerequisite] = self.factory.makeGitRefs(target=self.ref.target)
> + # Remove the implicit subscriptions.
> + merge_target.repository.unsubscribe(
> + merge_target.owner, merge_target.owner)
> + merge_prerequisite.repository.unsubscribe(
> + merge_prerequisite.owner, merge_prerequisite.owner)
> + merge_proposal1 = self.ref.addLandingTarget(
> + self.ref.owner, merge_target, merge_prerequisite)
> + # Disable this merge proposal, to allow creating a new identical one.
> + lp_admins = getUtility(ILaunchpadCelebrities).admin
> + merge_proposal1.rejectBranch(lp_admins, "null:")
> + merge_proposal2 = self.ref.addLandingTarget(
> + self.ref.owner, merge_target, merge_prerequisite)
> + return merge_proposal1, merge_proposal2
> +
> + def test_repository_with_merge_proposal(self):
> + # Ensure that deletion requirements with a merge proposal are right.
> + #
> + # Each repository related to the merge proposal is tested to ensure
> + # it produces a unique, correct result.
> + merge_proposal1, merge_proposal2 = self.makeMergeProposals()
> + self.assertEqual(
> + {
> + merge_proposal1:
> + ("delete",
> + _("This repository is the source repository of this merge "
> + "proposal.")),
> + merge_proposal2:
> + ("delete",
> + _("This repository is the source repository of this merge "
> + "proposal.")),
> + },
> + self.repository.getDeletionRequirements())
> + target = merge_proposal1.target_git_repository
> + self.assertEqual(
> + {
> + merge_proposal1:
> + ("delete",
> + _("This repository is the target repository of this merge "
> + "proposal.")),
> + merge_proposal2:
> + ("delete",
> + _("This repository is the target repository of this merge "
> + "proposal.")),
> + },
> + target.getDeletionRequirements())
> + prerequisite = merge_proposal1.prerequisite_git_repository
> + self.assertEqual(
> + {
> + merge_proposal1:
> + ("alter",
> + _("This repository is the prerequisite repository of this "
> + "merge proposal.")),
> + merge_proposal2:
> + ("alter",
> + _("This repository is the prerequisite repository of this "
> + "merge proposal.")),
> + },
> + prerequisite.getDeletionRequirements())
> +
> + def test_delete_merge_proposal_source(self):
> + # Merge proposal source repositories can be deleted with
> + # break_references.
> + merge_proposal1, merge_proposal2 = self.makeMergeProposals()
> + merge_proposal1_id = merge_proposal1.id
> + BranchMergeProposal.get(merge_proposal1_id)
> + self.repository.destroySelf(break_references=True)
> + self.assertRaises(
> + SQLObjectNotFound, BranchMergeProposal.get, merge_proposal1_id)
> +
> + def test_delete_merge_proposal_target(self):
> + # Merge proposal target repositories can be deleted with
> + # break_references.
> + merge_proposal1, merge_proposal2 = self.makeMergeProposals()
> + merge_proposal1_id = merge_proposal1.id
> + BranchMergeProposal.get(merge_proposal1_id)
> + merge_proposal1.target_git_repository.destroySelf(
> + break_references=True)
> + self.assertRaises(SQLObjectNotFound,
> + BranchMergeProposal.get, merge_proposal1_id)
> +
> + def test_delete_merge_proposal_prerequisite(self):
> + # Merge proposal prerequisite repositories can be deleted with
> + # break_references.
> + merge_proposal1, merge_proposal2 = self.makeMergeProposals()
> + merge_proposal1.prerequisite_git_repository.destroySelf(
> + break_references=True)
> + self.assertIsNone(merge_proposal1.prerequisite_git_repository)
> +
> + def test_delete_source_CodeReviewComment(self):
> + # Deletion of source repositories that have CodeReviewComments works.
> + comment = self.factory.makeCodeReviewComment(git=True)
> + comment_id = comment.id
> + repository = comment.branch_merge_proposal.source_git_repository
> + repository.destroySelf(break_references=True)
> + self.assertRaises(
> + SQLObjectNotFound, CodeReviewComment.get, comment_id)
> +
> + def test_delete_target_CodeReviewComment(self):
> + # Deletion of target repositories that have CodeReviewComments works.
> + comment = self.factory.makeCodeReviewComment(git=True)
> + comment_id = comment.id
> + repository = comment.branch_merge_proposal.target_git_repository
> + repository.destroySelf(break_references=True)
> + self.assertRaises(
> + SQLObjectNotFound, CodeReviewComment.get, comment_id)
> +
> + def test_sourceBranchWithCodeReviewVoteReference(self):
> + # break_references handles CodeReviewVoteReference source repository.
> + merge_proposal = self.factory.makeBranchMergeProposalForGit()
> + merge_proposal.nominateReviewer(
> + self.factory.makePerson(), self.factory.makePerson())
> + merge_proposal.source_git_repository.destroySelf(break_references=True)
> +
> + def test_targetBranchWithCodeReviewVoteReference(self):
> + # break_references handles CodeReviewVoteReference target repository.
> + merge_proposal = self.factory.makeBranchMergeProposalForGit()
> + merge_proposal.nominateReviewer(
> + self.factory.makePerson(), self.factory.makePerson())
> + merge_proposal.target_git_repository.destroySelf(break_references=True)
> +
> + def test_ClearPrerequisiteRepository(self):
> + # ClearPrerequisiteRepository.__call__ must clear the prerequisite
> + # repository.
> + merge_proposal = removeSecurityProxy(self.makeMergeProposals()[0])
> + with person_logged_in(
> + merge_proposal.prerequisite_git_repository.owner):
> + ClearPrerequisiteRepository(merge_proposal)()
> + self.assertIsNone(merge_proposal.prerequisite_git_repository)
> +
> + def test_DeletionOperation(self):
> + # DeletionOperation.__call__ is not implemented.
> + self.assertRaises(NotImplementedError, DeletionOperation("a", "b"))
> +
> + def test_DeletionCallable(self):
> + # DeletionCallable must invoke the callable.
> + merge_proposal = self.factory.makeBranchMergeProposalForGit()
> + merge_proposal_id = merge_proposal.id
> + DeletionCallable(
> + merge_proposal, "blah", merge_proposal.deleteProposal)()
> + self.assertRaises(
> + SQLObjectNotFound, BranchMergeProposal.get, merge_proposal_id)
> +
> +
> class TestGitRepositoryModifications(TestCaseWithFactory):
> """Tests for Git repository modification notifications."""
>
>
> === modified file 'lib/lp/services/config/schema-lazr.conf'
> --- lib/lp/services/config/schema-lazr.conf 2015-04-22 16:00:09 +0000
> +++ lib/lp/services/config/schema-lazr.conf 2015-05-19 11:33:34 +0000
> @@ -1818,6 +1818,10 @@
> module: lp.code.interfaces.branchjob
> dbuser: reclaim-branch-space
>
> +[IReclaimGitRepositorySpaceJobSource]
> +module: lp.code.interfaces.gitjob
> +dbuser: reclaim-branch-space
> +
> [IRemoveArtifactSubscriptionsJobSource]
> module: lp.registry.interfaces.sharingjob
> dbuser: sharing-jobs
>
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py 2015-05-19 00:41:13 +0000
> +++ lib/lp/testing/factory.py 2015-05-19 11:33:34 +0000
> @@ -1567,10 +1567,13 @@
> Diff.fromFile(StringIO(diff_text), len(diff_text)))
>
> def makePreviewDiff(self, conflicts=u'', merge_proposal=None,
> - date_created=None, size='small'):
> + date_created=None, size='small', git=False):
> diff = self.makeDiff(size)
> if merge_proposal is None:
> - merge_proposal = self.makeBranchMergeProposal()
> + if git:
> + merge_proposal = self.makeBranchMergeProposalForGit()
> + else:
> + merge_proposal = self.makeBranchMergeProposal()
> preview_diff = PreviewDiff()
> preview_diff.branch_merge_proposal = merge_proposal
> preview_diff.conflicts = conflicts
> @@ -2438,7 +2441,8 @@
>
> def makeCodeReviewComment(self, sender=None, subject=None, body=None,
> vote=None, vote_tag=None, parent=None,
> - merge_proposal=None, date_created=DEFAULT):
> + merge_proposal=None, date_created=DEFAULT,
> + git=False):
> if sender is None:
> sender = self.makePerson()
> if subject is None:
> @@ -2448,6 +2452,9 @@
> if merge_proposal is None:
> if parent:
> merge_proposal = parent.branch_merge_proposal
> + elif git:
> + merge_proposal = self.makeBranchMergeProposalForGit(
> + registrant=sender)
> else:
> merge_proposal = self.makeBranchMergeProposal(
> registrant=sender)
> @@ -2456,8 +2463,11 @@
> sender, subject, body, vote, vote_tag, parent,
> _date_created=date_created)
>
> - def makeCodeReviewVoteReference(self):
> - bmp = removeSecurityProxy(self.makeBranchMergeProposal())
> + def makeCodeReviewVoteReference(self, git=False):
> + if git:
> + bmp = removeSecurityProxy(self.makeBranchMergeProposalForGit())
> + else:
> + bmp = removeSecurityProxy(self.makeBranchMergeProposal())
> candidate = self.makePerson()
> return bmp.nominateReviewer(candidate, bmp.registrant)
>
>
--
https://code.launchpad.net/~cjwatson/launchpad/git-repository-delete/+merge/259487
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
