launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24826
[Merge] ~pappacena/launchpad:repo-creation-on-git-push into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:repo-creation-on-git-push into launchpad:master with ~pappacena/launchpad:gitrepo-status as a prerequisite.
Commit message:
Preventing repo creation on code hosting, and returning this information on translatePath call.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/385231
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:repo-creation-on-git-push into launchpad:master.
diff --git a/lib/lp/code/interfaces/gitnamespace.py b/lib/lp/code/interfaces/gitnamespace.py
index eb4ebc4..53ea14b 100644
--- a/lib/lp/code/interfaces/gitnamespace.py
+++ b/lib/lp/code/interfaces/gitnamespace.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Interface for a Git repository namespace."""
@@ -40,7 +40,7 @@ class IGitNamespace(Interface):
def createRepository(repository_type, registrant, name,
information_type=None, date_created=None,
target_default=False, owner_default=False,
- with_hosting=False):
+ with_hosting=False, status=None):
"""Create and return an `IGitRepository` in this namespace."""
def isNameUsed(name):
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index b2cbaae..c88fca1 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -213,6 +213,9 @@ class IGitRepositoryView(IHasRecipes):
shortened_path = Attribute(
"The shortest reasonable version of the path to this repository.")
+ def getClonedFrom():
+ """Returns from which repository the given repo is a clone from."""
+
@operation_parameters(
reviewer=Reference(
title=_("A person for which the reviewer status is in question."),
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index 96ab8c0..a535c72 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementations of `IGitNamespace`."""
@@ -40,7 +40,6 @@ from lp.code.errors import (
GitRepositoryCreatorNotMemberOfOwnerTeam,
GitRepositoryCreatorNotOwner,
GitRepositoryExists,
- GitTargetError,
)
from lp.code.interfaces.gitcollection import IAllGitRepositories
from lp.code.interfaces.gitnamespace import (
@@ -76,7 +75,7 @@ class _BaseGitNamespace:
reviewer=None, information_type=None,
date_created=DEFAULT, description=None,
target_default=False, owner_default=False,
- with_hosting=False):
+ with_hosting=False, status=None):
"""See `IGitNamespace`."""
repository_set = getUtility(IGitRepositorySet)
@@ -91,7 +90,7 @@ class _BaseGitNamespace:
repository = GitRepository(
repository_type, registrant, self.owner, self.target, name,
information_type, date_created, reviewer=reviewer,
- description=description)
+ description=description, status=status)
repository._reconcileAccess()
# The owner of the repository should also be automatically subscribed
@@ -122,24 +121,7 @@ class _BaseGitNamespace:
IStore(repository).flush()
assert repository.id is not None
- # If repository has target_default, clone from default.
- clone_from_repository = None
- try:
- default = repository_set.getDefaultRepository(
- repository.target)
- if default is not None and default.visibleByUser(registrant):
- clone_from_repository = default
- else:
- default = repository_set.getDefaultRepositoryForOwner(
- repository.owner, repository.target)
- if (default is not None and
- default.visibleByUser(registrant)):
- clone_from_repository = default
- except GitTargetError:
- pass # Ignore Personal repositories.
- if clone_from_repository == repository:
- clone_from_repository = None
-
+ clone_from_repository = repository.getClonedFrom()
repository._createOnHostingService(
clone_from_repository=clone_from_repository)
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 0421884..6d94a5c 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -370,6 +370,31 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
getUtility(IGitHostingClient).create(
hosting_path, clone_from=clone_from_path)
+ def getClonedFrom(self):
+ """See `IGitRepository`"""
+ repository_set = getUtility(IGitRepositorySet)
+ registrant = self.registrant
+
+ # If repository has target_default, clone from default.
+ clone_from_repository = None
+ try:
+ default = repository_set.getDefaultRepository(
+ self.target)
+ if default is not None and default.visibleByUser(registrant):
+ clone_from_repository = default
+ else:
+ default = repository_set.getDefaultRepositoryForOwner(
+ self.owner, self.target)
+ if (default is not None and
+ default.visibleByUser(registrant)):
+ clone_from_repository = default
+ except GitTargetError:
+ pass # Ignore Personal repositories.
+ if clone_from_repository == self:
+ clone_from_repository = None
+
+ return clone_from_repository
+
@property
def valid_webhook_event_types(self):
return ["git:push:0.1", "merge-proposal:0.1"]
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index b34307a..6163c48 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementations of the XML-RPC APIs for Git."""
@@ -31,6 +31,7 @@ from lp.app.validators import LaunchpadValidationError
from lp.code.enums import (
GitGranteeType,
GitPermissionType,
+ GitRepositoryStatus,
GitRepositoryType,
)
from lp.code.errors import (
@@ -214,9 +215,13 @@ class GitAPI(LaunchpadXMLRPCView):
raise faults.Unauthorized()
def _performLookup(self, requester, path, auth_params):
+ """Perform a translation path lookup.
+
+ :return: A tuple with the repository object and a dict with
+ translation information."""
repository, extra_path = getUtility(IGitLookup).getByPath(path)
if repository is None:
- return None
+ return None, None
verified = self._verifyAuthParams(requester, repository, auth_params)
naked_repository = removeSecurityProxy(repository)
@@ -236,7 +241,7 @@ class GitAPI(LaunchpadXMLRPCView):
try:
hosting_path = repository.getInternalPath()
except Unauthorized:
- return None
+ return naked_repository, None
writable = (
repository.repository_type == GitRepositoryType.HOSTED and
check_permission("launchpad.Edit", repository))
@@ -249,7 +254,7 @@ class GitAPI(LaunchpadXMLRPCView):
writable = True
private = repository.private
- return {
+ return naked_repository, {
"path": hosting_path,
"writable": writable,
"trailing": extra_path,
@@ -347,10 +352,14 @@ class GitAPI(LaunchpadXMLRPCView):
try:
try:
+ # Creates the repository on the database, but do not create
+ # it on hosting service. It should be created by the hosting
+ # service as a result of pathTranslate call indicating that
+ # the repo was just created in Launchpad.
namespace.createRepository(
GitRepositoryType.HOSTED, requester, repository_name,
target_default=target_default, owner_default=owner_default,
- with_hosting=True)
+ with_hosting=False, status=GitRepositoryStatus.CREATING)
except LaunchpadValidationError as e:
# Despite the fault name, this just passes through the
# exception text so there's no need for a new Git-specific
@@ -377,11 +386,22 @@ class GitAPI(LaunchpadXMLRPCView):
if requester == LAUNCHPAD_ANONYMOUS:
requester = None
try:
- result = self._performLookup(requester, path, auth_params)
+ repo, result = self._performLookup(requester, path, auth_params)
+ if repo and repo.status == GitRepositoryStatus.CREATING:
+ raise faults.GitRepositoryBeingCreated(path)
+
if (result is None and requester is not None and
permission == "write"):
self._createRepository(requester, path)
- result = self._performLookup(requester, path, auth_params)
+ repo, result = self._performLookup(
+ requester, path, auth_params)
+ clone_from = repo.getClonedFrom()
+ result["creation_params"] = {
+ "should_create": True,
+ "repository_id": repo.id,
+ "clone_from": (clone_from.getInternalPath() if clone_from
+ else None)
+ }
if result is None:
raise faults.GitRepositoryNotFound(path)
if permission != "read" and not result["writable"]:
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 6f12f1f..8fc24e4 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -29,10 +29,10 @@ from lp.app.enums import InformationType
from lp.buildmaster.enums import BuildStatus
from lp.code.enums import (
GitGranteeType,
+ GitRepositoryStatus,
GitRepositoryType,
TargetRevisionControlSystems,
)
-from lp.code.errors import GitRepositoryCreationFault
from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
from lp.code.interfaces.gitcollection import IAllGitRepositories
@@ -292,22 +292,28 @@ class TestGitAPIMixin:
requester, path.lstrip("/"))
self.assertIsNotNone(repository)
self.assertEqual(requester, repository.registrant)
- self.assertEqual(
- {"path": repository.getInternalPath(), "writable": True,
- "trailing": "", "private": private},
- translation)
- self.assertEqual(
- (repository.getInternalPath(),),
- self.hosting_fixture.create.extract_args()[0])
+
+ cloned_from = repository.getClonedFrom()
+ self.assertEqual({
+ "path": repository.getInternalPath(),
+ "writable": True,
+ "trailing": "",
+ "private": private,
+ "creation_params": {
+ "should_create": True,
+ "repository_id": repository.id,
+ "clone_from": (cloned_from.getInternalPath() if cloned_from
+ else None)}
+ }, translation)
+ self.assertEqual(0, self.hosting_fixture.create.call_count)
self.assertEqual(GitRepositoryType.HOSTED, repository.repository_type)
+ self.assertEqual(GitRepositoryStatus.CREATING, repository.status)
return repository
def assertCreatesFromClone(self, requester, path, cloned_from,
can_authenticate=False):
self.assertCreates(requester, path, can_authenticate)
- self.assertEqual(
- {"clone_from": cloned_from.getInternalPath()},
- self.hosting_fixture.create.extract_kwargs()[0])
+ self.assertEqual(0, self.hosting_fixture.create.call_count)
def assertHasRefPermissions(self, requester, repository, ref_paths,
permissions, macaroon_raw=None):
@@ -744,6 +750,21 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
self.assertCreates(
requester, u"/~%s/%s/+git/random" % (requester.name, project.name))
+ def test_translatePath_create_project_blocks_duplicate_calls(self):
+ # translatePath creates a project repository that doesn't exist,
+ # but blocks any further request to create the same repository.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct()
+ path = u"/~%s/%s/+git/random" % (requester.name, project.name)
+ self.assertCreates(requester, path)
+
+ auth_params = _make_auth_params(
+ requester, can_authenticate=True)
+ request_id = auth_params["request-id"]
+ self.assertFault(
+ faults.GitRepositoryBeingCreated,
+ request_id, "translatePath", path, "write", auth_params)
+
def test_translatePath_create_project_clone_from_target_default(self):
# translatePath creates a project repository cloned from the target
# default if it exists.
@@ -1062,27 +1083,6 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
self.assertTrue(repository.owner_default)
self.assertEqual(team, repository.owner)
- def test_translatePath_create_broken_hosting_service(self):
- # If the hosting service is down, trying to create a repository
- # fails and doesn't leave junk around in the Launchpad database.
- self.hosting_fixture.create.failure = GitRepositoryCreationFault(
- "nothing here", path="123")
- requester = self.factory.makePerson()
- initial_count = getUtility(IAllGitRepositories).count()
- oops_id = self.assertOopsOccurred(
- requester, u"/~%s/+git/random" % requester.name,
- permission="write")
- login(ANONYMOUS)
- self.assertEqual(
- initial_count, getUtility(IAllGitRepositories).count())
- # The error report OOPS ID should match the fault, and the traceback
- # text should show the underlying exception.
- self.assertEqual(1, len(self.oopses))
- self.assertEqual(oops_id, self.oopses[0]["id"])
- self.assertIn(
- "GitRepositoryCreationFault: nothing here",
- self.oopses[0]["tb_text"])
-
def test_translatePath_code_import(self):
# A code import worker with a suitable macaroon can write to a
# repository associated with a running code import job.
diff --git a/lib/lp/xmlrpc/faults.py b/lib/lp/xmlrpc/faults.py
index 7211377..9df4afb 100644
--- a/lib/lp/xmlrpc/faults.py
+++ b/lib/lp/xmlrpc/faults.py
@@ -1,4 +1,4 @@
-# Copyright 2009 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).
"""Launchpad XMLRPC faults."""
@@ -387,6 +387,13 @@ class GitRepositoryNotFound(PathTranslationError):
msg_template = "Repository '%(path)s' not found."
+class GitRepositoryBeingCreated(PathTranslationError):
+ """Raised when a Git repository path is currently beign created on
+ hosting service."""
+
+ msg_template = "Repository '%(path)s' creation is in progress."
+
+
class InvalidPath(LaunchpadFault):
"""Raised when `translatePath` is passed something that's not a path."""
Follow ups
