Add support for github gists #306

mariusvniekerk · 2017-11-28T20:00:42Z

No description provided.

choldgraf · 2017-11-28T20:07:03Z

binderhub/repoproviders.py

+        parts = self.spec.split('/')
+        self.user, self.gist_id, *_ = parts
+        if len(parts) > 2:
+            self.unresolved_ref = parts[1]


what's this usecase? A versioned gist? And just double-checking that here unresolved_ref would be set to gist_id...is that correct?

gists are just git repositories, and so we should version them the same too!

that should be parts[2] doh

yuvipanda

w00t, this is awesome. Some comments about enforcing versioning, but that's about it.

yuvipanda · 2017-11-28T20:47:21Z

binderhub/repoproviders.py

+        parts = self.spec.split('/')
+        self.user, self.gist_id, *_ = parts
+        if len(parts) > 2:
+            self.unresolved_ref = parts[1]


gists are just git repositories, and so we should version them the same too!

yuvipanda · 2017-11-28T20:47:25Z

binderhub/repoproviders.py

+
+    Users must provide a spec that matches the following form (similar to github)
+
+    <username>/<gist-id>[/<ref>]


Does this mean the ref is optional? I'd ideally like it to not be optional in the URL (we can ofc construct it in the JS). Git cloning says the default branch is master, so we should require a ref and go through the usual process of resolving it in the backend IMO.

so there is no branch called master as far as the upstream gist repo is concerned.

we can use master as a sentinal value meaning latest in the history chain if we want

Ah, hmm. I did a git clone and got to 'master'. Perhaps that's git clone's behavior? Am unsure. Am +1 using master as sentinel if that's not the case.

There is nothing in the gist api it seems to resolve branch names, probably doesn't really have any of them. Going to treat '' and 'master' as just meaning latest

yuvipanda · 2017-11-28T20:54:28Z

binderhub/repoproviders.py

+        return self.resolved_ref
+
+    def get_build_slug(self):
+        return self.gist_id


This will coalesce multiple revisions of gists building at the same time, which isn't what we want. It should involve both the id and the resolved ref IMO.

we dont use the resolved ref for any of the other build slugs, that gets added later

Ah, whoops. Sorry about that.

betatim · 2017-11-28T22:59:13Z

binderhub/app.py

@@ -19,7 +19,7 @@
 from .launcher import Launcher
 from .registry import DockerRegistry
 from .main import MainHandler, ParameterizedMainHandler, LegacyRedirectHandler
-from .repoproviders import GitHubRepoProvider, GitRepoProvider, GitLabRepoProvider
+from .repoproviders import GitHubRepoProvider, GitRepoProvider, GitLabRepoProvider, GitHubGistRepoProvider


nitpick: can this be GitHubGistProvider or even GistProvider? I don't know anywhere except GitHub that has Gists.

betatim · 2017-11-28T23:01:45Z

We should add a flag that lets the owner of the binderhub enable/disable private gist support.

mariusvniekerk · 2017-11-29T15:21:04Z

@betatim done

betatim · 2017-11-29T15:47:26Z

binderhub/repoproviders.py

+        ref_info = json.loads(resp.body.decode('utf-8'))
+
+        if (not self.allow_secret_gist) and (not ref_info['public']):
+            raise ValueError("gist is marked as secret, this is unsupported")


-> "Gist is marked as secret, to enable secret Gist support set GistRepoProvider.allow_secret_gist = True"?

Let's add a comment above line 382 or in the docstring that calls out the default behavior too.

For ValueError message:

You seem to want to use a secret Gist, but do not have permission to do so. To enable secret Gist support, set (or have an administrator set) 'GistRepoProvider.allow_secret_gist = True'

willingc

Looking good. One suggestion about commenting and one about error message.

willingc · 2017-11-29T16:09:27Z

binderhub/repoproviders.py

+        ref_info = json.loads(resp.body.decode('utf-8'))
+
+        if (not self.allow_secret_gist) and (not ref_info['public']):
+            raise ValueError("gist is marked as secret, this is unsupported")


Let's add a comment above line 382 or in the docstring that calls out the default behavior too.

For ValueError message:

You seem to want to use a secret Gist, but do not have permission to do so. To enable secret Gist support, set (or have an administrator set) 'GistRepoProvider.allow_secret_gist = True'

mariusvniekerk · 2017-11-29T20:24:29Z

@willingc @betatim updated

willingc

Thanks @mariusvniekerk for making the changes I suggested.

yuvipanda · 2017-12-03T20:51:59Z

Sorry about the delay in merging! TYVM for the PR!

willingc · 2017-12-04T03:02:08Z

Congrats on the PR being merged @mariusvniekerk. We appreciate the contribution. 👍

betatim · 2017-12-04T15:24:11Z

🎉

Should we deploy (and enable) this to mybinder.org?

choldgraf · 2017-12-04T17:00:54Z

I think we should deploy this, test it out a bit, and open an issue to write documentation for it (+ gitlab + raw git repos)

yuvipanda · 2017-12-04T17:19:15Z

Note that users can't actually use any of this yet since we don't have any UI for it.

choldgraf · 2017-12-04T17:25:02Z

yeah I know, but to me that's a feature rather than a bug :-) IMO we should plan to document and/or add UI around it in a couple of weeks once we've had time to see the feature in action, WDYT?

…

On Mon, Dec 4, 2017 at 9:19 AM Yuvi Panda ***@***.***> wrote: Note that users can't actually use any of this yet since we don't have any UI for it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#306 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwSHacflR5l0GpXZXvSgkSkf_213oC5ks5s9CmTgaJpZM4Qt1B7> .

yuvipanda · 2017-12-04T17:33:08Z

what do you mean by see the feature in action? :) The only way to use them right now is to write your own JavaScript...

choldgraf · 2017-12-04T17:44:41Z

oops - maybe I was mistaken, I thought this PR adds the URL entrypoint for gists, does it not?

yuvipanda · 2017-12-04T17:49:01Z

Nope, we don't have URL endpoints for gist nor gitlab :)

willingc · 2017-12-05T00:22:29Z

@yuvipanda does #322 cover the remaining work needed for gist, gitlab, and other support?

betatim · 2017-12-05T06:53:20Z

binderhub/app.py

@@ -165,6 +165,7 @@ def _add_slash(self, proposal):
    repo_providers = Dict(
        {
            'gh': GitHubRepoProvider,
+            'gist': GistRepoProvider,


Doesn't this add the endpoint? If not could someone point me to the bit of code that would need editing?

only for the API. If you look at main.py's ParameterizedMainHandler and templates/index.html, you'll see that the code simply dumps it into the appropriate textboxes and triggers 'launch' by fake-clicking the launch button. And if you look at index.js it's hardcoded to use gh....

Ah! I hadn't read the code closely enough and assumed that the ParametrizedMainHandler kicks off the build :)

It's not that smart yet :)

Since this work needs to touch the js, should we finish up and deploy #252 first?

Add support for github gists

e96241e

choldgraf reviewed Nov 28, 2017

View reviewed changes

yuvipanda requested changes Nov 28, 2017

View reviewed changes

mariusvniekerk added 2 commits November 28, 2017 16:50

fix typo

67ce6d8

gistprovider add support for master sentinal value

2c79502

betatim reviewed Nov 28, 2017

View reviewed changes

This was referenced Nov 28, 2017

Support for pointing to gists #62

Closed

Support non-GitHub git repo hosts #184

Closed

Add support to exclude secret gists

3805210

betatim reviewed Nov 29, 2017

View reviewed changes

willingc reviewed Nov 29, 2017

View reviewed changes

Update secret gist error message

62a8c42

willingc approved these changes Nov 29, 2017

View reviewed changes

yuvipanda approved these changes Dec 3, 2017

View reviewed changes

yuvipanda merged commit 5cdfc38 into jupyterhub:master Dec 3, 2017

betatim reviewed Dec 5, 2017

View reviewed changes

kikocorreoso mentioned this pull request May 10, 2018

Allow for multiple, different dependencies per repository #555

Closed


		Users must provide a spec that matches the following form (similar to github)

		<username>/<gist-id>[/<ref>]

Add support for github gists #306

Add support for github gists #306

Conversation

mariusvniekerk commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvipanda 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

betatim commented Nov 28, 2017

mariusvniekerk commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusvniekerk commented Nov 29, 2017

willingc left a comment

Choose a reason for hiding this comment

yuvipanda commented Dec 3, 2017

willingc commented Dec 4, 2017

betatim commented Dec 4, 2017

choldgraf commented Dec 4, 2017

yuvipanda commented Dec 4, 2017

choldgraf commented Dec 4, 2017 via email

yuvipanda commented Dec 4, 2017

choldgraf commented Dec 4, 2017

yuvipanda commented Dec 4, 2017

willingc commented Dec 5, 2017

betatim Dec 5, 2017 • 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

betatim Dec 5, 2017 •

edited

Loading