Update invenio GitHub #99

alejandromumo · 2023-06-21T11:45:37Z

closes #98

This PR is based on the updates from #97

concentrate the configuration of all calls in one place

alejandromumo · 2023-06-28T15:13:15Z

invenio_github/assets/semantic-ui/js/invenio_github/view.js

@@ -0,0 +1,109 @@
+// /*


this code is jquery, it will be removed.
I just commented it for now.

alejandromumo · 2023-06-28T15:13:28Z

invenio_github/assets/semantic-ui/js/invenio_github/init.js

@@ -20,14 +20,14 @@
 * or submit itself to any jurisdiction.


this code is jquery, it will be removed.
I just commented it for now.

invenio_github/config.py

alejandromumo · 2023-06-28T15:30:57Z

invenio_github/api.py

+        )
+
+        return repo_instance
+
    @classmethod
    def _dev_api(cls):


These three class methods were not tested yet by me.

It seems only revoke_token is used by a task

invenio_github/config.py

'ping' column tracked the last time a repository received a ping event. it was never consumed on Zenodo before, so we decided to remove it. the event itself is still persisted in db.

invenio_github/receivers.py

alejandromumo · 2023-06-29T07:33:57Z

invenio_github/views/github.py

+    def decorator(f):
+        @wraps(f)
+        def inner(*args, **kwargs):
+            github = GitHubAPI(user_id=current_user.id)


question: this decorator instantiates a GithubAPI object, while the views that use the decorator start by instantiating it (again). Should we pass the instance? E.g.

kwargs["github_api"] = github return f(*args, **kwargs)

alejandromumo · 2023-06-29T07:34:44Z

invenio_github/views/github.py

+@register_menu(  # TODO modify?
+    blueprint,
+    "settings.github",
+    # TODO substitute github for icon + 'Github'


TODO: the github icon does not show up in the UI. This has to be enabled when working the UI part of the integration

ping @anikachurilova

alejandromumo · 2023-06-29T07:35:41Z

invenio_github/views/github.py

            releases=releases,
            serializer=current_github.record_serializer,
        )
+    except RepositoryAccessError as e:
+        abort(403)


I'm not a big fan of these try/catch and then abort(code). Perhaps we can have error handlers (as we do with resources). For now it works, but there's room for improvement

alejandromumo · 2023-06-29T07:36:17Z

invenio_github/views/github.py


+###
+# TODO to be moved to its own folder (e.g. separate ui / api)


Do we want to separate ui and api endpoints? We do this separation in some modules, it's easier to navigate the code.

alejandromumo · 2023-06-29T07:37:55Z

invenio_github/views/github.py

+        github.sync(async_hooks=False)
+        db.session.commit()
+    except Exception:
+        db.session.rollback()


I moved away from nested sessions, as they were giving me some trouble. Namely, when we have to commit a session before spawning a celery task it would make the nested session error on exit.

run-tests.sh

setup.cfg

alejandromumo · 2023-06-29T07:41:59Z

tests/test_badge.py

@@ -26,17 +26,18 @@

 from flask import url_for

+# TODO uncomment when migrated


Commented for now since I did not touch badges and tests were failing

invenio_github/errors.py

ppanero

I've reviewed this with the premise that is a module migration to work with RDM but is not the intention to leave the code 100% clean and fix previous problems (although many were done anyway). I tried to review as much as possible, but is very hard to make an in-depth review in a sensible time due to its size; so my comments are more on structure than actual code implementation.

Tiny rant, nothing personal:

I'm seeing this (very big PRs, I've done it too), and I think we are setting ourselves for failure. Big PRs stay very long in the review column since they are very intimidating. For future reference, it would be nice to have more self contained PRs.

Black and other migrations
Example app removal
Actual code/functionality changes

Having all in one PR with 54 files and 2.2/2.7k changes is quite unmanagable and very time consuming to review. Half of the brain power goes to understand if the change was black formatting or actual code. We can review commits, but in this case there's 5 on the actual code so there's still a need to jump from commit to commit.

Honestly, even GitHub lags to handle a PR of this size 😂

Comments apply in many places related to layers:

We are importing NoResultsFound in some classes and in some we are not.
There is an api.py module and others get an actual name, e.g. receivers.py, nonetheless many seem to be APIs.
A clear separation between whats a model and what's an API, and even maybe splitting some of those API classes.
I'd suggest to have an /api folder and then tiny classes inside since it seems this module does not need an actual service layer but it can be okay with just an api to bridge services (in rdm-records) to the GH api.

ppanero · 2023-06-29T08:16:14Z

.github/workflows/tests.yml

+    runs-on: ubuntu-20.04
+    strategy:
+      matrix:
+        python-version: [3.7, 3.8, 3.9]


I wonder if we should remove 3.7 and start adding 3.10

README.rst

examples/app.py

invenio_github/__init__.py

ppanero · 2023-06-29T08:22:54Z

invenio_github/admin.py

Will this be migrated to invenio-administration as part of a separate PR?

invenio_github/tasks.py

invenio_github/utils.py

invenio_github/views/github.py

run-tests.sh

invenio_github/api.py

alejandromumo · 2023-07-03T11:00:34Z

invenio_github/config.py

-    'invenio_github.handlers:account_setup'
-GITHUB_REMOTE_APP['params']['request_token_params']['scope'] = \
-    'user,admin:repo_hook,read:org'
+REMOTE_APP["disconnect_handler"] = "invenio_github.handlers:disconnect"


question: importing the REMOTE_APP could be troublesome if it was replaced by another instance. Perhaps we should get the github remote app using the oauth-client extension.

E.g:

remote = LocalProxy( lambda: current_oauthclient.oauth.remote_apps[ current_app.config["GITHUB_WEBHOOK_RECEIVER_ID"] ] )

However this is config, can we lazy load?

Created an issue

utnapischtim added 7 commits May 22, 2023 22:45

migrate setup.py to setup.cfg

cee8e37

migrate to use black as opinionated auto formater

f5b70c3

add .git-blame-ignore-revs

586b67f

fix docs compatibilty problem with Sphinx>=5.0.0

fc046b2

move check_manifest configuration to setup.cfg.

5c945f5

concentrate the configuration of all calls in one place

increase minimal python version to 3.7

9cfcb02

migrate: flask_babelex replaced by invenio_i18n

8e00720

alejandromumo force-pushed the update_invenio_github branch from 9826cc0 to 50a5c99 Compare June 21, 2023 14:39

tests: add tests.yml file

17d51d2

alejandromumo force-pushed the update_invenio_github branch 2 times, most recently from e76934f to 4eb6323 Compare June 21, 2023 14:47

alejandromumo marked this pull request as ready for review June 21, 2023 14:47

alejandromumo force-pushed the update_invenio_github branch 4 times, most recently from b9f65b3 to 6ae5be8 Compare June 28, 2023 15:09

alejandromumo commented Jun 28, 2023

View reviewed changes

invenio_github/config.py Outdated Show resolved Hide resolved

alejandromumo force-pushed the update_invenio_github branch 2 times, most recently from 4b16040 to 9ba7aaa Compare June 28, 2023 15:28

alejandromumo commented Jun 28, 2023

View reviewed changes

invenio_github/config.py Outdated Show resolved Hide resolved

alejandromumo added 5 commits June 28, 2023 17:35

core: update module architecture

8e66aac

models: removed ping

11828f5

'ping' column tracked the last time a repository received a ping event. it was never consumed on Zenodo before, so we decided to remove it. the event itself is still persisted in db.

receiver: added lock for github event receiver

c91f3b8

ui: fixed folder structure.

b3cd96f

admin: removed admin.py file (deprecated)

189ad7f

alejandromumo force-pushed the update_invenio_github branch from 9ba7aaa to 189ad7f Compare June 28, 2023 15:35

alejandromumo commented Jun 28, 2023

View reviewed changes

invenio_github/receivers.py Outdated Show resolved Hide resolved