launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-set-git-path into lp:launchpad.

Commit message:
Allow setting Snap.git_path directly on the webservice.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-set-git-path/+merge/329360

This allows the backfill requested in https://github.com/canonical-websites/build.snapcraft.io/issues/917.

I thought about adding similar setters for the other components of the snap's source, but it got rather complex and less broadly-applicable, whereas changing the branch should be lightweight and easy.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-set-git-path into lp:launchpad.

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2017-05-22 11:27:17 +0000
+++ lib/lp/code/model/gitref.py	2017-08-22 12:01:09 +0000
@@ -592,7 +592,8 @@
         """See `GitRefDatabaseBackedMixin`."""
         path = self.repository.default_branch
         if path is None:
-            raise NotFoundError("Repository '%s' has no default branch")
+            raise NotFoundError(
+                "Repository '%s' has no default branch" % self.repository)
         ref = IStore(GitRef).get(GitRef, (self.repository_id, path))
         if ref is None:
             raise NotFoundError(

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2017-04-24 13:38:04 +0000
+++ lib/lp/snappy/interfaces/snap.py	2017-08-22 12:01:09 +0000
@@ -7,6 +7,7 @@
 
 __all__ = [
     'BadSnapSearchContext',
+    'BadSnapSource',
     'CannotAuthorizeStoreUploads',
     'CannotModifySnapProcessor',
     'CannotRequestAutoBuilds',
@@ -185,6 +186,11 @@
 
 
 @error_status(httplib.BAD_REQUEST)
+class BadSnapSource(Exception):
+    """The elements of the source for a snap package are inconsistent."""
+
+
+@error_status(httplib.BAD_REQUEST)
 class SnapPrivacyMismatch(Exception):
     """Snap package privacy does not match its content."""
 
@@ -461,11 +467,19 @@
         allow_fragment=False,
         trailing_slash=False))
 
-    git_path = exported(TextLine(
-        title=_("Git branch path"), required=False, readonly=True,
+    git_path = TextLine(
+        title=_("Git branch path"), required=False, readonly=False,
         description=_(
             "The path of the Git branch containing a snap/snapcraft.yaml, "
-            "snapcraft.yaml, or .snapcraft.yaml recipe at the top level.")))
+            "snapcraft.yaml, or .snapcraft.yaml recipe at the top level."))
+    _api_git_path = exported(
+        TextLine(
+            title=_("Git branch path"), required=False, readonly=False,
+            description=_(
+                "The path of the Git branch containing a snap/snapcraft.yaml, "
+                "snapcraft.yaml, or .snapcraft.yaml recipe at the top "
+                "level.")),
+        exported_as="git_path")
 
     git_ref = exported(Reference(
         IGitRef, title=_("Git branch"), required=False, readonly=False,

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2017-06-29 12:02:11 +0000
+++ lib/lp/snappy/model/snap.py	2017-08-22 12:01:09 +0000
@@ -107,6 +107,7 @@
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
+    BadSnapSource,
     CannotAuthorizeStoreUploads,
     CannotModifySnapProcessor,
     CannotRequestAutoBuilds,
@@ -241,6 +242,19 @@
         return ["snap:build:0.1"]
 
     @property
+    def _api_git_path(self):
+        return self.git_path
+
+    @_api_git_path.setter
+    def _api_git_path(self, value):
+        if self.git_repository is None and self.git_repository_url is None:
+            raise BadSnapSource(
+                "git_path may only be set on a Git-based snap.")
+        if value is None:
+            raise BadSnapSource("git_path may not be set to None.")
+        self.git_path = value
+
+    @property
     def git_ref(self):
         """See `ISnap`."""
         if self.git_repository is not None:

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2017-04-24 13:38:04 +0000
+++ lib/lp/snappy/tests/test_snap.py	2017-08-22 12:01:09 +0000
@@ -22,6 +22,7 @@
 from storm.exceptions import LostObjectError
 from storm.locals import Store
 from testtools.matchers import (
+    ContainsDict,
     Equals,
     MatchesSetwise,
     MatchesStructure,
@@ -1361,6 +1362,53 @@
         self.assertEqual(
             "Test Person is not a member of Other Team.", response.body)
 
+    def test_cannot_set_git_path_for_bzr(self):
+        # Setting git_path on a Bazaar-based Snap fails.
+        snap = self.makeSnap(branch=self.factory.makeAnyBranch())
+        response = self.webservice.patch(
+            snap["self_link"], "application/json",
+            json.dumps({"git_path": "HEAD"}))
+        self.assertEqual(400, response.status)
+
+    def test_cannot_set_git_path_to_None(self):
+        # Setting git_path to None fails.
+        snap = self.makeSnap(git_ref=self.factory.makeGitRefs()[0])
+        response = self.webservice.patch(
+            snap["self_link"], "application/json",
+            json.dumps({"git_path": None}))
+        self.assertEqual(400, response.status)
+
+    def test_set_git_path(self):
+        # Setting git_path on a Git-based Snap works.
+        ref_master, ref_next = self.factory.makeGitRefs(
+            paths=[u"refs/heads/master", u"refs/heads/next"])
+        snap = self.makeSnap(git_ref=ref_master)
+        response = self.webservice.patch(
+            snap["self_link"], "application/json",
+            json.dumps({"git_path": ref_next.path}))
+        self.assertEqual(209, response.status)
+        self.assertThat(response.jsonBody(), ContainsDict({
+            "git_repository_link": Equals(snap["git_repository_link"]),
+            "git_path": Equals(ref_next.path),
+            }))
+
+    def test_set_git_path_external(self):
+        # Setting git_path on a Snap backed by an external Git repository
+        # works.
+        ref = self.factory.makeGitRefRemote()
+        repository_url = ref.repository_url
+        snap = self.factory.makeSnap(
+            registrant=self.person, owner=self.person, git_ref=ref)
+        snap_url = api_url(snap)
+        logout()
+        response = self.webservice.patch(
+            snap_url, "application/json", json.dumps({"git_path": "HEAD"}))
+        self.assertEqual(209, response.status)
+        self.assertThat(response.jsonBody(), ContainsDict({
+            "git_repository_url": Equals(repository_url),
+            "git_path": Equals("HEAD"),
+            }))
+
     def test_getByName(self):
         # lp.snaps.getByName returns a matching Snap.
         snap = self.makeSnap()