Hack in fragile GitLab support #879
Conversation
Any domains that start with `gitlab` will use a handler that assumes an environment variable GITLAB_TOKEN which is your private access token for a v4 GitLab API.
|
This definitely looks like a really good start to me though! It actually looks like it hits all of the right places, as far as I can tell. In fact I'm not actually convinced it shouldn't be merged. I guess we can discuss that more if you want (keep in mind that I only have triage access not write access). I guess in order to view trees and not just notebooks it would be necessary or at least useful to write a gitlab client analogous to the github client for interfacing more thoroughly with the GitLab API. I've always suspected it might actually be fairly easy to copy and modify the code for the GitHub client and handlers to work for GitLab if one had knowledge of the GitLab API. I've never tried it myself though because I don't have any experience and don't know how to get acquainted with it. Anyway though GitLab integration with NBViewer would actually be a really useful feature e.g. at NERSC where a lot of repos are hosted on GitLab. @rcthomas Does NERSC have private Gitlab tokens that they might want to use with this? Yeah actually I'm not sure why this shouldn't be merged -- it doesn't seem like it affects any default behavior, except for URLs beginning with |
krinsman
added
tag:GitLab
There was a problem hiding this comment.
I recall @parente being somewhat uncomfortable about adding the provider handlers as traitlets configurables, but inasmuch as it was my bad idea I approve of it being extended here to include a new GitLab provider. This also uses default_rewrites and uri_rewrites much more effectively/intelligently than I ever did. The structure of the GitLab provider also looks really good, all of the code is in the right place in the right folders for example, the GET methods seem to demonstrate really good understanding of the GitLab URL scheme, there's good exception handling, and the changes are non-intrusive anyway since they only affect viewing GitLab notebooks, the default_handlers is all used correctly. And if it was merged we could let it be tested in the wild for a while before updating the public NBViewer service to incorporate it, although I wouldn't anticipate there being any issues if it was incorporated. Admittedly my judgment might be clouded by the fact that I'm flattered that the changes I made were understandable enough that someone else was able to copy, modify, and extend them, but regardless this actually looks really good to me. I certainly approve.
We'll be using `path_type` to identify whether to render the notebook directly or render a list view.
- Lookup blobs directly where possible - Fall back to searching project trees - Add logs for HTTP Errors - Remove info log messages with private tokens in URLs
|
@cbowdon Maybe this would also be even more likely to be merged if you copy-pasted some of the tests from I'm not sure which specific tests should be copied and modified at the moment, although if you want, and I can find the free time, I could possibly look into it for you. |
|
@krinsman Sure, I can do that. I've actually managed to find a little time to do the client refactoring you suggested and start to implement tree support. Progress will be very slow I'm afraid, but I'm blown away by the more welcoming and enthusiastic response than I expected 😄 |
|
@cbowdon Honestly I'm kind of blown away that you didn't expect an enthusiastic response, haha! This is a really good PR. Anyway that sounds great! GitLab support is a feature I have really wanted to see added to NBViewer, and I am glad that someone is working on it! |
Adds support for rendering the directory view with breadcrumbs. Last modified time is not included yet.
|
@cbowdon Oh wow these extra new commits look really good! These are really impressive actually. When you said that "progress will be very slow", I thought you meant months, not days! Please feel free to let me know when you are finished and want me to review the final product again. |
Also prevents the fallback lookup method failing because we hit the number of project search results.
|
@cbowdon I like these new changes! Let me know when you want me to look at the code for this PR again, i.e. when you think it might be ready for merging. |
|
@krinsman Thanks! I'm dogfooding this PR internally and keep discovering little problems, not all fixed yet. Will let you know as soon as it's ready for review. |
|
Haha, very nice, I just learned that term ("dogfooding") today from you. Looking forward to hear back about any updates! |
|
@krinsman sorry for taking so long on this. Also sorry, I don't understand why that check has failed - would appreciate a hand on that. I'm worried that if I leave this too long, merge conflicts will start to build up. Would you be open to merging it as-is? The outstanding problems are:
The entire PR should only affect URLs starting with |
|
@cbowdon Hi! The PR looks really interesting... just 2 questions:
Clarifying the latter Gitlab docs mention that when doing calls, e.g. the In other words one can encode the following url: https://gitlab.cs.washington.edu/prescience/public-notebooks The code seems to create I can try a fix but I'd like to first make sure I'm not wrong... (happen to have a PhD in being wrong 😛). |
|
Hi! Any update on the gitlab support and this issue? I've wanted to add a viewer in my README for a project and that would be just the thing I need =) Thanks for the help |
Hi! This isn't merge-worthy I'm afraid, but I'm submitting the PR in case it helps someone else who wants to do the same thing. The changes add support for viewing notebooks on private GitLab instances (but sadly only notebooks, not trees or anything else).
With this PR, any domains that start with
gitlabwill use a handler that assumes an environment variableGITLAB_TOKENwhich is your private access token for a v4 GitLab API.