Update Github backend to use Github API v4 #582

Zlopez · 2018-08-21T14:01:10Z

Implementation of Github API v4 as requested in #567
This should also solve #581

codecov-io · 2018-08-21T14:05:13Z

Codecov Report

Merging #582 into master will increase coverage by 0.35%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   89.65%   90.01%   +0.35%     
==========================================
  Files          55       55              
  Lines        2591     2644      +53     
  Branches      332      341       +9     
==========================================
+ Hits         2323     2380      +57     
+ Misses        203      201       -2     
+ Partials       65       63       -2

Impacted Files	Coverage Δ
anitya/config.py	`100% <ø> (ø)`	⬆️
anitya/lib/backends/github.py	`100% <100%> (+19.04%)`	⬆️
anitya/lib/exceptions.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64e4bde...97bbef1. Read the comment docs.

jeremycline

There's a few things that can be refactored here, but overall it's looking like a big improvement 🍰 ✨

jeremycline · 2018-08-22T19:13:21Z

Vagrantfile.example

@@ -4,7 +4,7 @@
 VAGRANTFILE_API_VERSION = "2"

 Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
- config.vm.box = "fedora/27-cloud-base"
+ config.vm.box = "fedora/28-cloud-base"


It's generally good practice to break this sort of stuff out into a separate commit. I don't see anyone wanting to revert this or anything, but it's a nice separation of concerns thing. Not a huge deal, don't feel like you need to fix it here.

I will move it to separate commit.

jeremycline · 2018-08-22T19:16:16Z

anitya/lib/backends/__init__.py

-            req.add_header('User-Agent', user_agent)
-            req.add_header('From', from_email)
+            req.add_header('User-Agent', headers.user_agent)
+            req.add_header('From', headers.from_email)


headers is a dictionary, right? I don't think this works.

Yes, you are right. This means, that there was not test for this, I will add it.

jeremycline · 2018-08-22T19:21:20Z

anitya/lib/backends/github.py

+        if url:
+            (owner, repo) = url.split('/')
+
+        if (not owner) or (not repo):


It's exactly equivalent, but if not (owner and repo) might read a little cleaner.

Also, I know this is just a refactor of what was there before, but it makes more sense to validate this before it goes in the database, rather than right here. It'd tell the user up-front they've put in incorrect data. Might be a nice refactor for a future PR.

Yes, this should be probably validated on frontend in javascript, but you also need to validate it before saving it to db (in case somebody is trying something nasty). This could be different for every backend, so it will be good to have validation method as part of backend and the db method could call it before inserting the data.

What do you think?

That sounds reasonable, yes. It can be called during the pre-commit event.

jeremycline · 2018-08-22T19:25:36Z

anitya/lib/backends/github.py

+        return versions
+
+    @classmethod
+    def prepare_query(cls, owner, repo, after=''):


There's no reason for this to be a class method. There's also no reason for the class inheritance structure in the backends generally, so while this matches the general style, it's not a good style. You can either make it a private instance method or just rip this out into a private module-level function.

I will change it to private method. It doesn't need to be public.

jeremycline · 2018-08-22T19:27:09Z

anitya/lib/backends/github.py

+    def prepare_query(cls, owner, repo, after=''):
+        ''' Method for preparing GraphQL query for specified repository
+
+        :arg owner: owner of the repository


I find the "Google" style of comments more pleasant to write, you might as well.

I used the style, that was there before, but I will change it.

Yeah, it was the old style and I was slowly converting it all as I touched things

jeremycline · 2018-08-22T19:33:27Z

anitya/lib/backends/github.py

+        return query
+
+    @classmethod
+    def call_url_post(cls, url, json, insecure=False, use_token=False):


I think you can just factor this into the get_versions call. I think it's reasonable for the token to be required, and we should never use insecure=True with GitHub so it boils down to a single call to another function.

The get_headers interface (while matching the existing style) is more complicated than just setting a default set of headers somewhere, then making a copy here and injecting the access token. I recommend doing that.

I didn't understand the second paragraph in your comment. You want me to copy the get_headers method to this backend.
The get_headers method is new method in BaseBackend and it only fills basic header fields.

Ah, what I'm saying is I don't like the use of inheritance in the backends. It's complicated and adds no value, but I never got a chance to refactor it. Rather than add to it, this could be implemented as:

In constants or BaseBackend's module or something:

REQUEST_HEADERS = <default_headers>

Then for your Github backend you just make a copy of REQUEST_HEADERS and add your auth token:

github_headers = REQUEST_HEADERS.copy() github_headers['Authentication'] = ...

There's no need for a get_headers method that subclasses override. The whole call_url function has no business being a class method at all. If should just be a module-level function that acts as a wrapper that handles the special FTP case and otherwise is the requests API.

I thought that the inheritance is intentional, otherwise BaseBackend doesn't have much sense. I thought it is used as some kind of interface, that should be implemented by every backend.

Inheritance is fine to define the interface, but the only interface we're really interested in is for a given project, new versions are returned. Everything else is an implementation detail of the particular backend and doesn't need to be part of the class.

jeremycline · 2018-08-22T19:36:05Z

anitya/lib/backends/github.py

+                '%s: Server responded with status "%s": "%s"' % (
+                    project.name, resp.status_code, resp.reason))
+
+        if 'errors' in json:


Consider making a function that accepts the json and spits out the versions (or raises an exception). That'll make testing easier, and make this function a digestible length.

Ok, I will do this. It sounds good.

jeremycline · 2018-08-22T19:37:54Z

anitya/lib/backends/github.py

+        # cursor = json['data']['repository']['refs']['edges']['cursor']
+
+        remaining = json['data']['rateLimit']['remaining']
+        reset_time = json['data']['rateLimit']['resetAt']


We should handle the rate limiting gracefully. If we're getting rate limited, it should raise an exception (RateLimitError or similar) that contains the time the rate limit is over so that the caller can handle it by rescheduling for later (or exploding)

This looks like elegant solution to handle the rate limit.

jeremycline · 2018-08-24T17:21:15Z

anitya/lib/backends/github.py

+from anitya.lib.backends import BaseBackend, http_session, REQUEST_HEADERS
+from anitya.lib.exceptions import AnityaPluginException, RateLimitException
+from anitya.config import config
+import logging


Historically we've followed PEP 8, which includes rules for import ordering: the first group is stdlib imports, the second group is third-party libs, and the final group is imports from the current package. Each group is separated by a blank line.

jeremycline

A few small comments, but the only thing I think really needs to change is the decoding of FTP responses.

jeremycline · 2018-08-24T17:23:31Z

anitya/lib/backends/github.py

+
+        try:
+            headers = REQUEST_HEADERS.copy()
+            token = config.get('GITHUB_ACCESS_TOKEN')


Since you set a default for the token, you don't need to use get.

You are right

jeremycline · 2018-08-24T17:26:53Z

anitya/lib/exceptions.py

+    def __init__(self, reset_time):
+        self._reset_time = reset_time
+
+    @property


I recommend just calling self._reset_time self.reset_time and not making a property. Properties are handy if you need to do some sort of transformation or you want your internal representation to differ from the public API in some way, but here it's the same.

jeremycline · 2018-08-24T17:28:03Z

anitya/tests/lib/backends/test_init.py


+import unittest


Following the PEP 8 rules, unittest should stay up in the top group and six should move to this group

Didn't know this

jeremycline · 2018-08-24T17:31:23Z

.../tests/request-data/anitya.tests.lib.backends.test_init.BaseBackendTests.test_expand_subdirs

+      date: ['Thu, 23 Aug 2018 09:39:54 GMT']
+      keep-alive: ['timeout=5, max=100']
+      server: [Apache/2.2.22 (Fedora)]
+      x-clacks-overhead: [GNU Terry Pratchett]


I was surprised by this value when I fetched the URL.

In case you're not familiar with Terry Pratchett's Discworld books, one of his books includes a lot of amusing references to the Free Software movement and this HTTP header is in reference to one of his books.

I know his work. I read almost all his books from Discworld, but I was surprised to see the header field, I never saw it before.

jeremycline · 2018-08-24T17:32:20Z

anitya/lib/backends/github.py

+
+    _log.debug('Received %s tags for %s' % (total_count, project.name))
+
+    # TODO: Save cursor to project and use it to fetch only newer versions


I recommend turning these TODOs into tickets and removing them from the code

jeremycline · 2018-08-24T17:39:57Z

anitya/lib/backends/__init__.py

+            try:
+                dir_listing = resp.text
+            except AttributeError:
+                dir_listing = resp.decode('utf-8')


I just skimmed https://tools.ietf.org/html/rfc2640, but it sounds like there's no guarantees that the response will be UTF-8 encoded, and even if the spec said it MUST be, I'd be willing to bet there are some non-conforming servers out there. If it should only be ASCII/UTF-8 then this is fine, but this should handle an Unicode decoding exception and raise the appropriate Anitya exception explaining that the FTP service is misbehaving.

I will add another try-except when trying to decode the binary string and if it's not UTF-8, I will just raise AnityaPluginException.
Maybe there is some way, how to get the encodings of the binary string and then use the appropriate decode method. This will be nice way how to do it.
I will check this.
It will be much easier, if Requests library could work with FTP.

There is a requests-ftp extension, although I'm not sure if it works with current requests. It'd be nice to drop the FTP support, but I'm not sure if there's still a number of projects only available over FTP. I think just bailing is a reasonable approach. The specification indicates it should only ever be ASCII or UTF-8, but because FTP is so old I have no doubt there are servers out there not following the latest RFC about that.

I looked at it, but it's only a template, not real library. It's only some basic functionality for future work.

jeremycline · 2018-08-27T14:08:10Z

anitya/lib/backends/__init__.py

+                            url, e.reason))
+            except UnicodeDecodeError:
+                raise AnityaPluginException(
+                        'FTP response is not unicode: %s' % url)


A slightly more accurate error message would be 'FTP response cannot be decoded with UTF-8' since Unicode defines the code points and UTF-8 is a particular way to map Unicode code points to a binary representation, but this is also fine

Sounds better than current error message.

jeremycline · 2018-08-27T14:10:03Z

anitya/lib/exceptions.py

@@ -115,7 +115,6 @@ class RateLimitException(AnityaException):
    def __init__(self, reset_time):
        self._reset_time = reset_time

-    @property
    def reset_time(self):


Ah, what I meant was just delete this method and change the name from self._reset_time to self.reset_time

I only want the time to be retrievable by the code which catches it.

Maybe bad habit from other object oriented languages.
I'm used to have getter and setter (when needed) for every attribute the object has.

But in this case it could be probably better to have direct access to reset_time.

I wouldn't really call it a bad habit (this is the only way to do things in Java), but it's not "Pythonic". Python uses _ it indicate a variable, method, function, etc is private and might change at any time, but doesn't enforce it. If the interface you'd like to present to the public differs from the one you'd like to manage internally, it's a perfect place for the setter/getter pattern with properties. Otherwise, just exposing the variable itself is fine.

Zlopez · 2018-08-28T08:23:45Z

Update with latest master git rebase master

jeremycline

✨ 🍰 ✨

jeremycline suggested changes Aug 22, 2018

View reviewed changes

Zlopez force-pushed the github_API branch 3 times, most recently from 2bad5d5 to d8cb24b Compare August 24, 2018 11:54

jeremycline reviewed Aug 24, 2018

View reviewed changes

jeremycline suggested changes Aug 24, 2018

View reviewed changes

jeremycline reviewed Aug 27, 2018

View reviewed changes

Zlopez force-pushed the github_API branch from 04cef28 to ced8663 Compare August 28, 2018 08:15

Zlopez added 8 commits August 28, 2018 10:22

Update Github backend to use Github API v4

a7956a0

Raise test coverage

552212e

Update dev environment

920669e

Refactoring of github API

ca30545

Change docstrings to Google style

831d558

Raise code coverage

7754a66

Update FTP call

7de64d7

Minor tweaks

97bbef1

Zlopez force-pushed the github_API branch from ced8663 to 97bbef1 Compare August 28, 2018 08:23

jeremycline approved these changes Aug 28, 2018

View reviewed changes

mergify bot merged commit bcb24f7 into fedora-infra:master Aug 28, 2018

Zlopez deleted the github_API branch August 28, 2018 17:05

This was referenced Sep 20, 2018

bodhi: no upstream version found. - https://github.com/fedora-infra/bodhi/tags - class="tag-name">([^<]*)</span #511

Closed

Github backend always raises "no upstream version found" #412

Closed


		_log.debug('Received %s tags for %s' % (total_count, project.name))

		# TODO: Save cursor to project and use it to fetch only newer versions

Update Github backend to use Github API v4 #582

Update Github backend to use Github API v4 #582

Conversation

Zlopez commented Aug 21, 2018

codecov-io commented Aug 21, 2018 • edited Loading

Codecov Report

jeremycline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremycline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez commented Aug 28, 2018

jeremycline left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 21, 2018 •

edited

Loading

Zlopez Aug 23, 2018 •

edited

Loading

Zlopez Aug 23, 2018 •

edited

Loading