refactor(gitlab): move all GitLab requests to dedicated client (#685) #685
Conversation
mdonadoni
force-pushed
the
refactor-gitlab-client
branch
from
5c41262
to
ab76dee
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #685 +/- ##
==========================================
+ Coverage 60.06% 60.68% +0.61%
==========================================
Files 32 33 +1
Lines 3308 3416 +108
==========================================
+ Hits 1987 2073 +86
- Misses 1321 1343 +22
|
mdonadoni
changed the title
|
Tested locally by copying valid token from DEV and by accessing the cluster through my CERN hostname. |
mdonadoni
force-pushed
the
refactor-gitlab-client
branch
from
ab76dee
to
664f683
Compare
| from reana_server.config import REANA_GITLAB_HOST | ||
|
|
||
|
|
||
| class GitLabException(Exception): |
There was a problem hiding this comment.
Cosmetics: since GitLabClient is our own, perhaps we can name all exceptions starting with this prefix, to better associate them together and make sure that the exceptions are not "generic" or coming from some other library:
- GitLabClientException
- GitLabClientRequestError
- GitLabClientInvalidToken
- ...
| super().__init__(message) | ||
|
|
||
|
|
||
| class GitLabClient: |
There was a problem hiding this comment.
Hmm, is there some existing GitLab client library which we could perhaps adopt, such as python-gitlab? We don't need much, so we can go with our own. Just wondering for future maintainability purposes, possible versioning when GitLab upgrades, etc.
There was a problem hiding this comment.
I guess both approaches are fine. python-gitlab only needs requests and requests-toolbelt, so it shouldn't be too difficult to keep it up to date and integrated with the rest of REANA. One plus of using an external library would be that it can be used also in r-w-controller, and not only in r-server (even though r-server and r-w-controller might be merged at some point, see all the discussions we have had on the topic).
There was a problem hiding this comment.
In any case, we would probably still need to wrap the library to use our custom errors, or we would need to change the rest of the code to check for the library's errors.
| ).content | ||
|
|
||
| # request access token | ||
| client = GitLabClient() |
There was a problem hiding this comment.
Cosmetics: since we may have a REANA client in the module, it may be good to be more specific and instead of client use gitlab_client everywhere as the symbol name? Currently some lines have shorter name, some longer:
reana_server/rest/gitlab.py:177: client = GitLabClient()
reana_server/rest/gitlab.py:182: authenticated_client = GitLabClient(access_token=access_token)
reana_server/rest/gitlab.py:318: client = GitLabClient.from_k8s_secret(user.id_)
reana_server/rest/gitlab.py:476: client = GitLabClient.from_k8s_secret(user.id_)
reana_server/utils.py:447: client = GitLabClient.from_k8s_secret(user_id)
reana_server/utils.py:467: client = GitLabClient.from_k8s_secret(user.id_)
tests/test_gitlab_client.py:40: gitlab_client = GitLabClient.from_k8s_secret(user_id, host="gitlab.example.org")
tests/test_gitlab_client.py:70: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:92: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:113: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:133: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:166: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:188: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:210: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:235: gitlab_client = GitLabClient(
tests/test_gitlab_client.py:261: gitlab_client = GitLabClient(
Just cosmetics, since the context is usually very clear...
Closes #676
Depends on #684