Merge lp:~jelmer/brz/github-proposals-fix into lp:brz

[Jelmer Vernooij]

Fix iter_proposals for GitHub and GitLab.

Fix iter_proposals for GitHub.

[Martin Packman (gz)]

I think this is fine. Have a bit of nervousness over the json handling, as the Python 2 and 3 behaviours of that module are silly (both deal in str rather than bytes). So, the code is probably broken on Python 2 with non-ascii pull request titles. Can sort that later.

review: Approve

=== modified file 'breezy/bzr/smart/medium.py'
--- breezy/bzr/smart/medium.py	2019-03-06 14:03:19 +0000
+++ breezy/bzr/smart/medium.py	2019-07-28 23:05:30 +0000
@@ -696,7 +696,7 @@
         self.counts = weakref.WeakKeyDictionary()
         client._SmartClient.hooks.install_named_hook(
             'call', self.increment_call_count, 'hpss call counter')
-        breezy.get_global_state().cleanups.add_cleanup(self.flush_all)
+        breezy.get_global_state().exit_stack.callback(self.flush_all)
 
     def track(self, medium):
         """Start tracking calls made to a medium.

=== modified file 'breezy/git/transportgit.py'
--- breezy/git/transportgit.py	2019-06-29 19:54:32 +0000
+++ breezy/git/transportgit.py	2019-07-28 23:05:30 +0000
@@ -427,11 +427,11 @@
             refs_container = InfoRefsContainer(BytesIO(refs_text))
             try:
                 head = TransportRefsContainer(
-                    self._commontransport).read_loose_ref("HEAD")
+                    self._commontransport).read_loose_ref(b"HEAD")
             except KeyError:
                 pass
             else:
-                refs_container._refs["HEAD"] = head
+                refs_container._refs[b"HEAD"] = head
         else:
             refs_container = TransportRefsContainer(
                 self._commontransport, self._controltransport)

=== modified file 'breezy/plugins/propose/github.py'
--- breezy/plugins/propose/github.py	2019-07-07 00:41:49 +0000
+++ breezy/plugins/propose/github.py	2019-07-28 23:05:30 +0000
@@ -108,7 +108,8 @@
 
 class GitHubMergeProposal(MergeProposal):
 
-    def __init__(self, pr):
+    def __init__(self, gh, pr):
+        self._gh = gh
         self._pr = pr
 
     @property
@@ -130,14 +131,27 @@
     def get_commit_message(self):
         return None
 
+    def set_commit_message(self, message):
+        self._patch({'title': message})
+
+    def _patch(self, data):
+        response = self._gh._api_request(
+            'PATCH', self._pr['url'], body=json.dumps(data).encode('utf-8'))
+        if response != 200:
+            raise InvalidHttpResponse(self._pr['url'], response.text)
+        self._pr = json.loads(response.text)
+
     def set_description(self, description):
-        self._pr.edit(body=description, title=determine_title(description))
+        self._patch({
+            'body': description,
+            'title': determine_title(description),
+            })
 
     def is_merged(self):
-        return self._pr['merged']
+        return self._pr['state'] == 'merged'
 
     def close(self):
-        self._pr.edit(state='closed')
+        self._patch({'state': 'closed'})
 
     def merge(self, commit_message=None):
         # https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button
@@ -178,14 +192,14 @@
     def __repr__(self):
         return "GitHub()"
 
-    def _api_request(self, method, path):
+    def _api_request(self, method, path, body=None):
         headers = {
             'Accept': 'application/vnd.github.v3+json'}
         if self._token:
             headers['Authorization'] = 'token %s' % self._token
         response = self.transport.request(
             method, urlutils.join(self.transport.base, path),
-            headers=headers)
+            headers=headers, body=body)
         if response.status == 401:
             raise GitHubLoginRequired(self)
         return response
@@ -199,6 +213,22 @@
             return json.loads(response.text)
         raise InvalidHttpResponse(path, response.text)
 
+    def _get_repo_pulls(self, path, head=None, state=None):
+        path = 'repos/' + path + '/pulls?'
+        params = {}
+        if head is not None:
+            params['head'] = head
+        if state is not None:
+            params['state'] = state
+        path += ';'.join(['%s=%s' % (k, urlutils.quote(v))
+                         for k, v in params.items()])
+        response = self._api_request('GET', path)
+        if response.status == 404:
+            raise NoSuchProject(path)
+        if response.status == 200:
+            return json.loads(response.text)
+        raise InvalidHttpResponse(path, response.text)
+
     def _get_user(self, username=None):
         if username:
             path = 'users/:%s' % username
@@ -304,28 +334,30 @@
             parse_github_branch_url(source_branch))
         (target_owner, target_repo_name, target_branch_name) = (
             parse_github_branch_url(target_branch))
-        target_repo = self._get_repo(
-            "%s/%s" % (target_owner, target_repo_name))
+        target_repo_path = "%s/%s" % (target_owner, target_repo_name)
+        target_repo = self._get_repo(target_repo_path)
         state = {
             'open': 'open',
             'merged': 'closed',
             'closed': 'closed',
             'all': 'all'}
-        for pull in target_repo.get_pulls(
-                head=target_branch_name,
-                state=state[status]):
-            if (status == 'closed' and pull.merged or
-                    status == 'merged' and not pull.merged):
-                continue
-            if pull.head.ref != source_branch_name:
-                continue
-            if pull.head.repo is None:
+        pulls = self._get_repo_pulls(
+            target_repo_path,
+            head=target_branch_name,
+            state=state[status])
+        for pull in pulls:
+            if (status == 'closed' and pull['merged'] or
+                    status == 'merged' and not pull['merged']):
+                continue
+            if pull['head']['ref'] != source_branch_name:
+                continue
+            if pull['head']['repo'] is None:
                 # Repo has gone the way of the dodo
                 continue
-            if (pull.head.repo.owner.login != source_owner or
-                    pull.head.repo.name != source_repo_name):
+            if (pull['head']['repo']['owner']['login'] != source_owner or
+                    pull['head']['repo']['name'] != source_repo_name):
                 continue
-            yield GitHubMergeProposal(pull)
+            yield GitHubMergeProposal(self, pull)
 
     def hosts(self, branch):
         try:
@@ -366,7 +398,7 @@
             response = self._api_request('GET', url)
             if response.status != 200:
                 raise InvalidHttpResponse(url, response.text)
-            yield GitHubMergeProposal(json.loads(response.text))
+            yield GitHubMergeProposal(self, json.loads(response.text))
 
     def get_proposal_by_url(self, url):
         raise UnsupportedHoster(url)
@@ -430,4 +462,4 @@
         if labels:
             for label in labels:
                 pull_request.issue.labels.append(label)
-        return GitHubMergeProposal(pull_request)
+        return GitHubMergeProposal(self.gh, pull_request)

=== modified file 'breezy/plugins/propose/gitlabs.py'
--- breezy/plugins/propose/gitlabs.py	2019-07-07 15:50:07 +0000
+++ breezy/plugins/propose/gitlabs.py	2019-07-28 23:05:30 +0000
@@ -230,10 +230,10 @@
     def base_url(self):
         return self.transport.base
 
-    def _api_request(self, method, path):
+    def _api_request(self, method, path, data=None):
         return self.transport.request(
             method, urlutils.join(self.base_url, 'api', 'v4', path),
-            headers=self.headers)
+            headers=self.headers, body=data)
 
     def __init__(self, transport, private_token):
         self.transport = transport
@@ -247,13 +247,13 @@
             raise NoSuchProject(project_name)
         if response.status == 200:
             return json.loads(response.data)
-        raise InvalidHttpResponse(path, response.text)
+        raise errors.InvalidHttpResponse(path, response.text)
 
     def _fork_project(self, project_name):
         path = 'projects/%s/fork' % urlutils.quote(str(project_name), '')
         response = self._api_request('POST', path)
         if response != 201:
-            raise InvalidHttpResponse(path, response.text)
+            raise errors.InvalidHttpResponse(path, response.text)
         return json.loads(response.data)
 
     def _get_logged_in_username(self):
@@ -261,7 +261,7 @@
 
     def _list_merge_requests(self, owner=None, project=None, state=None):
         if project is not None:
-            path = 'projects/%s/merge_requests' % urlutils.quote(str(project_name), '')
+            path = 'projects/%s/merge_requests' % urlutils.quote(str(project), '')
         else:
             path = 'merge_requests'
         parameters = {}
@@ -276,27 +276,30 @@
             raise errors.PermissionDenied(response.text)
         if response.status == 200:
             return json.loads(response.data)
-        raise InvalidHttpResponse(path, response.text)
+        raise errors.InvalidHttpResponse(path, response.text)
 
     def _create_mergerequest(
             self, title, source_project_id, target_project_id,
             source_branch_name, target_branch_name, description,
             labels=None):
         path = 'projects/%s/merge_requests' % source_project_id
+        fields = {
+            'title': title,
+            'source_branch': source_branch_name,
+            'target_branch': target_branch_name,
+            'target_project_id': target_project_id,
+            'description': description,
+            }
+        if labels:
+            fields['labels'] = labels
         response = self._api_request(
-            'POST', path, fields={
-                'title': title,
-                'source_branch': source_branch_name,
-                'target_branch': target_branch_name,
-                'target_project_id': target_project_id,
-                'description': description,
-                'labels': labels})
+            'POST', path, data=json.dumps(fields).encode('utf-8'))
         if response.status == 403:
             raise errors.PermissionDenied(response.text)
         if response.status == 409:
             raise MergeProposalExists(self.source_branch.user_url)
-        if response.status == 200:
-            raise InvalidHttpResponse(path, response.text)
+        if response.status != 201:
+            raise errors.InvalidHttpResponse(path, response.text)
         return json.loads(response.data)
 
     def get_push_url(self, branch):
@@ -436,7 +439,7 @@
 
 class GitlabMergeProposalBuilder(MergeProposalBuilder):
 
-    def __init__(self, l, source_branch, target_branch):
+    def __init__(self, gl, source_branch, target_branch):
         self.gl = gl
         self.source_branch = source_branch
         (self.source_host, self.source_project_name, self.source_branch_name) = (
@@ -481,8 +484,8 @@
             'title': title,
             'source_project_id': source_project['id'],
             'target_project_id': target_project['id'],
-            'source_branch': self.source_branch_name,
-            'target_branch': self.target_branch_name,
+            'source_branch_name': self.source_branch_name,
+            'target_branch_name': self.target_branch_name,
             'description': description}
         if labels:
             kwargs['labels'] = ','.join(labels)

=== modified file 'breezy/plugins/propose/launchpad.py'
--- breezy/plugins/propose/launchpad.py	2019-07-07 00:41:01 +0000
+++ breezy/plugins/propose/launchpad.py	2019-07-28 23:05:30 +0000
@@ -188,7 +188,7 @@
             lp_base_url = uris.STAGING_SERVICE_ROOT
         else:
             lp_base_url = uris.LPNET_SERVICE_ROOT
-        self.launchpad = lp_api.connect_launchpad(lp_base_url)
+        self.launchpad = lp_api.connect_launchpad(lp_base_url, version='devel')
 
     @property
     def base_url(self):

=== modified file 'breezy/transport/http/__init__.py'
--- breezy/transport/http/__init__.py	2019-07-15 23:34:05 +0000
+++ breezy/transport/http/__init__.py	2019-07-28 23:05:30 +0000
@@ -1822,6 +1822,7 @@
     """
 
     accepted_errors = [200,  # Ok
+                       201,
                        206,  # Partial content
                        400,
                        403,
@@ -2029,6 +2030,9 @@
             def readlines(self):
                 return self._actual.readlines()
 
+            def readlines(self):
+                return self._actual.readlines()
+
             def readline(self, size=-1):
                 return self._actual.readline(size)