Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Add Zenodo provider #870

Merged
merged 4 commits into from
Jun 13, 2019
Merged

[MRG] Add Zenodo provider #870

merged 4 commits into from
Jun 13, 2019

Conversation

betatim
Copy link
Member

@betatim betatim commented Jun 10, 2019

This makes the Zenodo content provider from repo2docker available in the UI.

@lukasheinrich
Copy link

this looks very nice! One comment: zenodo records have versions, where there is a top-level DOI that never changes per record but which can have multiple revision (each rev has its own DOI). This can probably be mapped more or less to rev's in the repo case.

@betatim
Copy link
Member Author

betatim commented Jun 10, 2019

Do you have an example of a DOI that has versions? I tried to find one to see what happens and as far as I can tell if you take the "top level" DOI it will send you to the latest version. This is what should happen right? If someone wants to refer to a specific version they should use the DOI for that version.

Either way a DOI that has "versions" would be nice for testing.

@lukasheinrich
Copy link

lukasheinrich commented Jun 10, 2019

pyhf has versions (and is set up for binder):

https://zenodo.org/record/1464139

see right hand side.. these are the DOIs

10.5281/zenodo.1464139
10.5281/zenodo.1172961
10.5281/zenodo.1169740 

@betatim
Copy link
Member Author

betatim commented Jun 10, 2019

The way I understand the versions is that each of the DOIs you listed should point to their specific version and that 10.5281/zenodo.1169739 is the DOI to use if you always want the latest one. It is somehow the "master" DOI(?).

This means we should probably change the repoprovider here to use the Zenodo record ID as the key when deciding if something needs building or not. Similar to how we resolve tags and branches to git commit SHAs. Right now the DOI is used, which is a bug. It means that if today 10.5281/zenodo.1169739 had two versions (records 123 and 234) and got built and then tomorrow a third version gets added (record 321) we would not rebuild 10.5281/zenodo.1169739 because it already has an image in the cache. Where as if we use the record ID we would.

@lukasheinrich
Copy link

yes, as far as I understand

https://zenodo.org/record/1169739

10.5281/zenodo.1169739

are persistent identifiers to the overall record. At any point in time they redirect to the latest version (currently https://zenodo.org/record/1464139 and 10.5281/zenodo.1464139 )

but the only real persistent identifiers are the individual DOIs for each version.

I agree that is is analogous to the ref and commit hash.

ref: master ~ overall DOI
commit hash version DOI that the overall DOI currently resolves to. Should be taken as the cache id to decide whether a rebuild is necessary.

@betatim betatim changed the title [WIP] Add Zenodo provider [MRG] Add Zenodo provider Jun 11, 2019
@betatim
Copy link
Member Author

betatim commented Jun 11, 2019

Ready for review now. Short GIF showing this in action:

Kapture 2019-06-11 at 22 06 33

Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that my approval counts for anything, but the changes are quite minimal and the video looks great!

@betatim
Copy link
Member Author

betatim commented Jun 11, 2019

Trying to add a test, but stumped by why the async voodoo isn't performing.

binderhub/app.py Outdated Show resolved Hide resolved
@betatim
Copy link
Member Author

betatim commented Jun 12, 2019

@yuvipanda do you understand why the test just hangs? On the face of it it seems to be a copy&paste of the other provider tests :-/

@betatim betatim force-pushed the zenodo-provider branch 3 times, most recently from 8a2b9e1 to 5574074 Compare June 13, 2019 05:30
The simple HTTP client doesn't support everything we need for the Zenodo
content provider.
@betatim
Copy link
Member Author

betatim commented Jun 13, 2019

The problem seems to have been that the simple HTTP client doesn't follow the redirect or otherwise gets stuck. Switching to the curl based one fixed things. It is also the one we use in the docker image of BinderHub which is (I think) why we didn't see this when running a hub and manually testing.

@minrk minrk merged commit 908c443 into jupyterhub:master Jun 13, 2019
@minrk
Copy link
Member

minrk commented Jun 13, 2019

Hooray!

@betatim betatim deleted the zenodo-provider branch June 13, 2019 09:40
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants