launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24032
[Merge] ~cjwatson/launchpad:export-git-repositories-new into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:export-git-repositories-new into launchpad:master.
Commit message:
Export IGitRepositorySet.new on the webservice
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1824399 in Launchpad itself: "Add Git HTTPS push tokens for snapcraft experiment"
https://bugs.launchpad.net/launchpad/+bug/1824399
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373763
For example, this allows snapcraft to create a temporary repository, issue an access token to it, and push code to it, all without needing to configure an SSH key.
In order to make this possible, I had to do some preliminary refactoring to push on-disk repository creation down from the XML-RPC endpoint to the model.
This is essentially the same as https://code.launchpad.net/~cjwatson/launchpad/export-git-repositories-new/+merge/373267, converted to git and rebased on master.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:export-git-repositories-new into launchpad:master.
diff --git a/lib/lp/code/errors.py b/lib/lp/code/errors.py
index 56521b5..daba338 100644
--- a/lib/lp/code/errors.py
+++ b/lib/lp/code/errors.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Errors used in the lp/code modules."""
@@ -432,6 +432,10 @@ class GitRepositoryCreatorNotOwner(GitRepositoryCreationException):
class GitRepositoryCreationFault(Exception):
"""Raised when there is a hosting fault creating a Git repository."""
+ def __init__(self, message, path):
+ super(GitRepositoryCreationFault, self).__init__(message)
+ self.path = path
+
class GitRepositoryScanFault(Exception):
"""Raised when there is a fault scanning a repository."""
diff --git a/lib/lp/code/interfaces/gitnamespace.py b/lib/lp/code/interfaces/gitnamespace.py
index 1867e2b..99841d7 100644
--- a/lib/lp/code/interfaces/gitnamespace.py
+++ b/lib/lp/code/interfaces/gitnamespace.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2019 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."""
@@ -32,10 +32,14 @@ class IGitNamespace(Interface):
name = Attribute(
"The name of the namespace. This is prepended to the repository name.")
+ owner = Attribute("The `IPerson` who owns this namespace.")
+
target = Attribute("The `IHasGitRepositories` for this namespace.")
def createRepository(repository_type, registrant, name,
- information_type=None, date_created=None):
+ information_type=None, date_created=None,
+ target_default=False, owner_default=False,
+ with_hosting=False):
"""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 d03d25d..2fca251 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -25,6 +25,7 @@ from lazr.restful.declarations import (
export_as_webservice_collection,
export_as_webservice_entry,
export_destructor_operation,
+ export_factory_operation,
export_operation_as,
export_read_operation,
export_write_operation,
@@ -927,10 +928,21 @@ class IGitRepositorySet(Interface):
export_as_webservice_collection(IGitRepository)
- def new(registrant, owner, target, name, information_type=None,
- date_created=None):
+ @call_with(
+ repository_type=GitRepositoryType.HOSTED,
+ registrant=REQUEST_USER,
+ with_hosting=True)
+ @operation_parameters(
+ information_type=copy_field(
+ IGitRepositoryView["information_type"], required=False))
+ @export_factory_operation(IGitRepository, ["owner", "target", "name"])
+ @operation_for_version("devel")
+ def new(repository_type, registrant, owner, target, name,
+ information_type=None, date_created=None, with_hosting=False):
"""Create a Git repository and return it.
+ :param repository_type: The `GitRepositoryType` of the new
+ repository.
:param registrant: The `IPerson` who registered the new repository.
:param owner: The `IPerson` who owns the new repository.
:param target: The `IProduct`, `IDistributionSourcePackage`, or
@@ -939,6 +951,7 @@ class IGitRepositorySet(Interface):
:param information_type: Set the repository's information type to
one different from the target's default. The type must conform
to the target's code sharing policy. (optional)
+ :param with_hosting: Create the repository on the hosting service.
"""
# Marker for references to Git URL layouts: ##GITNAMESPACE##
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index 72c7bda..d895967 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Communication with the Git hosting service."""
@@ -98,7 +98,7 @@ class GitHostingClient:
self._post("/repo", json=request)
except requests.RequestException as e:
raise GitRepositoryCreationFault(
- "Failed to create Git repository: %s" % unicode(e))
+ "Failed to create Git repository: %s" % unicode(e), path)
def getProperties(self, path):
"""See `IGitHostingClient`."""
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index c2b5f00..75ea427 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementations of `IGitNamespace`."""
@@ -39,6 +39,7 @@ from lp.code.errors import (
GitRepositoryCreatorNotMemberOfOwnerTeam,
GitRepositoryCreatorNotOwner,
GitRepositoryExists,
+ GitTargetError,
)
from lp.code.interfaces.gitcollection import IAllGitRepositories
from lp.code.interfaces.gitnamespace import (
@@ -72,8 +73,11 @@ class _BaseGitNamespace:
def createRepository(self, repository_type, registrant, name,
reviewer=None, information_type=None,
- date_created=DEFAULT, description=None):
+ date_created=DEFAULT, description=None,
+ target_default=False, owner_default=False,
+ with_hosting=False):
"""See `IGitNamespace`."""
+ repository_set = getUtility(IGitRepositorySet)
self.validateRegistrant(registrant)
self.validateRepositoryName(name)
@@ -102,6 +106,42 @@ class _BaseGitNamespace:
notify(ObjectCreatedEvent(repository))
+ if target_default:
+ repository_set.setDefaultRepository(self.target, repository)
+ if owner_default:
+ repository_set.setDefaultRepositoryForOwner(
+ self.owner, self.target, repository, registrant)
+
+ if with_hosting:
+ # Ask the hosting service to create the repository on disk. Do
+ # this as late as possible, since if this succeeds it can't
+ # easily be rolled back with the rest of the transaction.
+
+ # Flush to make sure that repository.id is populated.
+ 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
+
+ repository._createOnHostingService(
+ clone_from_repository=clone_from_repository)
+
return repository
def isNameUsed(self, repository_name):
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 5d4607a..7c7d451 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -197,7 +197,7 @@ from lp.services.propertycache import (
cachedproperty,
get_property_cache,
)
-from lp.services.webapp.authorization import available_with_permission
+from lp.services.webapp.authorization import check_permission
from lp.services.webapp.interfaces import ILaunchBag
from lp.services.webhooks.interfaces import IWebhookSet
from lp.services.webhooks.model import WebhookTargetMixin
@@ -342,6 +342,16 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
self.owner_default = False
self.target_default = False
+ def _createOnHostingService(self, clone_from_repository=None):
+ """Create this repository on the hosting service."""
+ hosting_path = self.getInternalPath()
+ if clone_from_repository is not None:
+ clone_from_path = clone_from_repository.getInternalPath()
+ else:
+ clone_from_path = None
+ getUtility(IGitHostingClient).create(
+ hosting_path, clone_from=clone_from_path)
+
@property
def valid_webhook_event_types(self):
return ["git:push:0.1", "merge-proposal:0.1"]
@@ -1648,13 +1658,15 @@ class DeleteCodeImport(DeletionOperation):
class GitRepositorySet:
"""See `IGitRepositorySet`."""
- def new(self, registrant, owner, target, name, information_type=None,
- date_created=DEFAULT, description=None):
+ def new(self, repository_type, registrant, owner, target, name,
+ information_type=None, date_created=DEFAULT, description=None,
+ with_hosting=False):
"""See `IGitRepositorySet`."""
namespace = get_git_namespace(target, owner)
return namespace.createRepository(
- registrant, name, information_type=information_type,
- date_created=date_created, description=description)
+ repository_type, registrant, name,
+ information_type=information_type, date_created=date_created,
+ description=description, with_hosting=with_hosting)
def getByPath(self, user, path):
"""See `IGitRepositorySet`."""
@@ -1724,13 +1736,16 @@ class GitRepositorySet:
"Personal repositories cannot be defaults for any target.")
return IStore(GitRepository).find(GitRepository, *clauses).one()
- @available_with_permission('launchpad.Edit', 'target')
def setDefaultRepository(self, target, repository):
"""See `IGitRepositorySet`."""
if IPerson.providedBy(target):
raise GitTargetError(
"Cannot set a default Git repository for a person, only "
"for a project or a package.")
+ if not check_permission("launchpad.Edit", target):
+ raise Unauthorized(
+ "You cannot set the default Git repository for %s." %
+ target.display_name)
if repository is not None and repository.target != target:
raise GitTargetError(
"Cannot set default Git repository to one attached to "
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index e679142..da75ad9 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -26,6 +26,7 @@ from storm.exceptions import LostObjectError
from storm.store import Store
from testtools.matchers import (
AnyMatch,
+ ContainsDict,
EndsWith,
Equals,
Is,
@@ -2990,6 +2991,35 @@ class TestGitRepositorySet(TestCaseWithFactory):
super(TestGitRepositorySet, self).setUp()
self.repository_set = getUtility(IGitRepositorySet)
+ def test_new(self):
+ # By default, GitRepositorySet.new creates a new repository in the
+ # database but not on the hosting service.
+ hosting_fixture = self.useFixture(GitHostingFixture())
+ owner = self.factory.makePerson()
+ target = self.factory.makeProduct()
+ name = self.factory.getUniqueUnicode()
+ repository = self.repository_set.new(
+ GitRepositoryType.HOSTED, owner, owner, target, name)
+ self.assertThat(repository, MatchesStructure.byEquality(
+ registrant=owner, owner=owner, target=target, name=name))
+ self.assertEqual(0, hosting_fixture.create.call_count)
+
+ def test_new_with_hosting(self):
+ # GitRepositorySet.new(with_hosting=True) creates a new repository
+ # in both the database and the hosting service.
+ hosting_fixture = self.useFixture(GitHostingFixture())
+ owner = self.factory.makePerson()
+ target = self.factory.makeProduct()
+ name = self.factory.getUniqueUnicode()
+ repository = self.repository_set.new(
+ GitRepositoryType.HOSTED, owner, owner, target, name,
+ with_hosting=True)
+ self.assertThat(repository, MatchesStructure.byEquality(
+ registrant=owner, owner=owner, target=target, name=name))
+ self.assertEqual(
+ [((repository.getInternalPath(),), {"clone_from": None})],
+ hosting_fixture.create.calls)
+
def test_provides_IGitRepositorySet(self):
# GitRepositorySet instances provide IGitRepositorySet.
verifyObject(IGitRepositorySet, self.repository_set)
@@ -3370,6 +3400,42 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
"git+ssh://git.launchpad.test/~person/project/+git/repository",
repository["git_ssh_url"])
+ def assertNewWorks(self, target_db):
+ hosting_fixture = self.useFixture(GitHostingFixture())
+ if IPerson.providedBy(target_db):
+ owner_db = target_db
+ else:
+ owner_db = self.factory.makePerson()
+ owner_url = api_url(owner_db)
+ target_url = api_url(target_db)
+ name = "repository"
+ webservice = webservice_for_person(
+ owner_db, permission=OAuthPermission.WRITE_PUBLIC)
+ webservice.default_api_version = "devel"
+ response = webservice.named_post(
+ "/+git", "new", owner=owner_url, target=target_url, name=name)
+ self.assertEqual(201, response.status)
+ repository = webservice.get(response.getHeader("Location")).jsonBody()
+ self.assertThat(repository, ContainsDict({
+ "repository_type": Equals("Hosted"),
+ "registrant_link": EndsWith(owner_url),
+ "owner_link": EndsWith(owner_url),
+ "target_link": EndsWith(target_url),
+ "name": Equals(name),
+ "owner_default": Is(False),
+ "target_default": Is(False),
+ }))
+ self.assertEqual(1, hosting_fixture.create.call_count)
+
+ def test_new_project(self):
+ self.assertNewWorks(self.factory.makeProduct())
+
+ def test_new_package(self):
+ self.assertNewWorks(self.factory.makeDistributionSourcePackage())
+
+ def test_new_person(self):
+ self.assertNewWorks(self.factory.makePerson())
+
def assertGetRepositoriesWorks(self, target_db):
if IPerson.providedBy(target_db):
owner_db = target_db
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index cf20432..d85aae7 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -13,7 +13,6 @@ import sys
from pymacaroons import Macaroon
import six
from six.moves import xmlrpc_client
-from storm.store import Store
import transaction
from zope.component import (
ComponentLookupError,
@@ -36,7 +35,6 @@ from lp.code.errors import (
GitRepositoryCreationFault,
GitRepositoryCreationForbidden,
GitRepositoryExists,
- GitTargetError,
InvalidNamespace,
)
from lp.code.interfaces.codehosting import (
@@ -44,7 +42,6 @@ from lp.code.interfaces.codehosting import (
LAUNCHPAD_SERVICES,
)
from lp.code.interfaces.gitapi import IGitAPI
-from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitjob import IGitRefScanJobSource
from lp.code.interfaces.gitlookup import (
IGitLookup,
@@ -282,20 +279,15 @@ class GitAPI(LaunchpadXMLRPCView):
if repository_name is None and not namespace.has_defaults:
raise InvalidNamespace(path)
if repository_name is None:
- def default_func(new_repository):
- if owner is None:
- self.repository_set.setDefaultRepository(
- target, new_repository)
- if (owner is not None or
- self.repository_set.getDefaultRepositoryForOwner(
- repository_owner, target) is None):
- self.repository_set.setDefaultRepositoryForOwner(
- repository_owner, target, new_repository, requester)
-
repository_name = namespace.findUnusedName(target.name)
- return namespace, repository_name, default_func
+ target_default = owner is None
+ owner_default = (
+ owner is None or
+ self.repository_set.getDefaultRepositoryForOwner(
+ repository_owner, target) is None)
+ return namespace, repository_name, target_default, owner_default
else:
- return namespace, repository_name, None
+ return namespace, repository_name, False, False
def _reportError(self, path, exception, hosting_path=None):
properties = [
@@ -310,7 +302,7 @@ class GitAPI(LaunchpadXMLRPCView):
def _createRepository(self, requester, path, clone_from=None):
try:
- namespace, repository_name, default_func = (
+ namespace, repository_name, target_default, owner_default = (
self._getGitNamespaceExtras(path, requester))
except InvalidNamespace:
raise faults.PermissionDenied(
@@ -331,56 +323,27 @@ class GitAPI(LaunchpadXMLRPCView):
raise faults.PermissionDenied(unicode(e))
try:
- repository = namespace.createRepository(
- GitRepositoryType.HOSTED, requester, repository_name)
- 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 fault.
- raise faults.InvalidBranchName(e)
- except GitRepositoryExists as e:
- # We should never get here, as we just tried to translate the
- # path and found nothing (not even an inaccessible private
- # repository). Log an OOPS for investigation.
- self._reportError(path, e)
- except GitRepositoryCreationException as e:
- raise faults.PermissionDenied(unicode(e))
-
- try:
- if default_func:
- try:
- default_func(repository)
- except Unauthorized:
- raise faults.PermissionDenied(
- "You cannot set the default Git repository for '%s'." %
- path)
-
- # Flush to make sure that repository.id is populated.
- Store.of(repository).flush()
- assert repository.id is not None
-
- # If repository has target_default, clone from default.
- target_path = None
- try:
- default = self.repository_set.getDefaultRepository(
- repository.target)
- if default is not None and default.visibleByUser(requester):
- target_path = default.getInternalPath()
- else:
- default = self.repository_set.getDefaultRepositoryForOwner(
- repository.owner, repository.target)
- if (default is not None and
- default.visibleByUser(requester)):
- target_path = default.getInternalPath()
- except GitTargetError:
- pass # Ignore Personal repositories.
-
- hosting_path = repository.getInternalPath()
try:
- getUtility(IGitHostingClient).create(
- hosting_path, clone_from=target_path)
+ namespace.createRepository(
+ GitRepositoryType.HOSTED, requester, repository_name,
+ target_default=target_default, owner_default=owner_default,
+ with_hosting=True)
+ 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
+ # fault.
+ raise faults.InvalidBranchName(e)
+ except GitRepositoryExists as e:
+ # We should never get here, as we just tried to translate
+ # the path and found nothing (not even an inaccessible
+ # private repository). Log an OOPS for investigation.
+ self._reportError(path, e)
+ except (GitRepositoryCreationException, Unauthorized) as e:
+ raise faults.PermissionDenied(unicode(e))
except GitRepositoryCreationFault as e:
# The hosting service failed. Log an OOPS for investigation.
- self._reportError(path, e, hosting_path=hosting_path)
+ self._reportError(path, e, hosting_path=e.path)
+ raise
except Exception:
# We don't want to keep the repository we created.
transaction.abort()
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 5d939d5..757443a 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -949,7 +949,7 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
# 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")
+ "nothing here", path="123")
requester = self.factory.makePerson()
initial_count = getUtility(IAllGitRepositories).count()
oops_id = self.assertOopsOccurred(
