att / rcloud

Collaborative data analysis and visualization
http://rcloud.social
MIT License
432 stars 142 forks source link

github doesn't always use hashes for gists #513

Open s-u opened 10 years ago

s-u commented 10 years ago

This rather a heads-up: github also uses non-hash references to gists. When you create a public gist on current GHE it has a short integer reference instead. This impacts discoverability and code that assumes gists IDs are hashes (e.g. where we distinguish a notebook name from the id).

cscheid commented 10 years ago

The same thing happens on public github, fwiw.

cscheid commented 10 years ago

(In what sense does this impact discoverability, though?)

s-u commented 10 years ago

It's much easier to guess the gist name so we cannot hide it anymore.

cscheid commented 10 years ago

Ah. Let's get our terminologies in sync here: I've been using "discoverability" as a positive thing, in the sense of "RCloud enables discoverability of work in your organization".

You're concerned about leaking secrets. You're right, but 1) that's by design on github, 2) that's why the default setting for create_gist is public=false: https://developer.github.com/v3/gists/#create-a-gist

How did this become a problem for you?

gordonwoodhull commented 10 years ago

Yeah, these are public gists, therefore not hidden.

gordonwoodhull commented 10 years ago

I think the thing to document here would be the three different kinds of public/private:

s-u commented 10 years ago

@cscheid it has never been a problem for me, I don't care :)

@gordonwoodhull yes, I agree, this should be probably documented somewhere.

My main point here is that, technically, we cannot assume hashes.

cscheid commented 10 years ago

Does this trigger a bug somewhere in RCloud right now?

s-u commented 10 years ago

I'm pretty sure such notebooks cannot be used in notebook.R by ID, because the distinction between by-id and by-name calls is whether it has a hash form or not:

## is this first part a notebook hash?
if (grepl("^[0-9a-f]{20}$", pex[1L])) {
cscheid commented 10 years ago

@s-u, great. I'll open an issue for that.