grab maximum amount of comments to fix issue with --merge-pr if there any many comments in a PR

[boegel]

This fixes a problem I ran into when merging easybuilders/easybuild-easyconfigs#6677 using eb --merge-pr:

$ eb --merge-pr 6677

== temporary log file in case of crash /Users/kehoste/work/TMP/eb-OIjkhI/easybuild-VQoHGP.log

easybuilders/easybuild-easyconfigs PR #6677 was submitted by akesandgren, you are using GitHub account 'boegel'

Checking eligibility of easybuilders/easybuild-easyconfigs PR #6677 for merging...
* targets develop branch: OK
* test suite passes: OK
* last test report is successful: FAILED => not eligible for merging!
* last test report is successful: FAILED => not eligible for merging!
* last test report is successful: OK
* approved review: OK (by boegel)
* milestone is set: OK (3.7.0)

WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway)

The problem is that this PR has many comments, exceeded the default number of comments that are returned by the GitHub API.

There's still a problem when a PR has more than 100 comments, but to fix that we'll need to make the fetching of comments page-aware, cfr. https://developer.github.com/v3/#pagination + https://developer.github.com/v3/guides/traversing-with-pagination/

[damianam]

@boegel I think it is possible to disable pagination altogether: https://github.com/api-platform/docs/blob/master/core/pagination.md

Whether that makes sense or not..... I am not sure. It could potentially be a problem, but I doubt in reality it will happen, as it would need a very large number of comments to be an issue.

[damianam]

Just a silly comment: Doesn't it make sense to set do_build=False and testing=True? I took a brief look at the _run_mock_eb code (and eb_main, and main), and I don't think it makes any difference, but still.....

[damianam]

Add a comment here stating that a PR with lots of comments (> GITHUB_MAX_PER_PAGE ) might fail?

[migueldiascosta]

more generically (since other types of API calls can also be paginated and the defaults can be different), we could traverse pages whenever they are present (according to the Link header)

I had to slightly change vsc.utils.rest in order for head() to actually return the headers, e.g.

--- a/lib/vsc/utils/rest.py
+++ b/lib/vsc/utils/rest.py
@@ -174,11 +174,14 @@ class Client(object):
         # TODO: in recent python: Context manager
         conn = self.get_connection(method, url, body, headers)
         status = conn.code
-        body = conn.read()
-        try:
-            pybody = json.loads(body)
-        except ValueError:
-            pybody = body
+        if method == self.HEAD:
+            pybody = conn.headers
+        else:
+            body = conn.read()
+            try:
+                pybody = json.loads(body)
+            except ValueError:
+                pybody = body
         fancylogger.getLogger().debug('reponse len: %s ', len(pybody))
         conn.close()
         return status, pybody

but once we have the headers, githup_api_get_request could generically handle pagination with something like

--- a/easybuild/tools/github.py
+++ b/easybuild/tools/github.py
@@ -243,10 +243,36 @@ def github_api_get_request(request_f, github_user=None, token=None, **kwargs):
     url = request_f(RestClient(GITHUB_API_URL, username=github_user, token=token))
 
     try:
-        status, data = url.get(**kwargs)
+        status, headers = url.head(**kwargs)
     except socket.gaierror as err:
-        _log.warning("Error occurred while performing get request: %s", err)
-        status, data = 0, None
+        _log.warning("Error occurred while getting headers from request: %s", err)
+        status, headers = 0, None
+
+    npages = 1
+    if headers:
+        links = headers.getheader('Link')
+        if links:
+            match = re.search(r"page=(\d+)>; rel=\"last\"", links)
+            if match:
+                npages = int(match.groups()[0])
+
+    if npages > 1:
+        data = []
+        for page in range(1, npages+1):
+            kwargs['page'] = page
+            try:
+                status, page_data = url.get(**kwargs)
+            except socket.gaierror as err:
+                _log.warning("Error occurred while performing get request: %s", err)
+                status, page_data = 0, None
+                break
+            data.extend(page_data)
+    else:
+        try:
+            status, data = url.get(**kwargs)
+        except socket.gaierror as err:
+            _log.warning("Error occurred while performing get request: %s", err)
+            status, data = 0, None
 
     _log.debug("get request result for %s: status: %d, data: %s", url, status, data)
     return (status, data)

?

[boegel]

@migueldiascosta Are you up for opening a PR to vsc-base for making that change?

We'll need to keep in mind to port that change into the 4.x branch here where we've ingested vsc-base...