-
Notifications
You must be signed in to change notification settings - Fork 391
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 hydroshare as a repo provider #967
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just wondering about the wording to be consistent with other DOI-based repos.
@@ -56,6 +56,7 @@ <h4 id="form-header" class='row'>Build and launch a repository</h4> | |||
<li class="dropdown-item" value="git"><a href="#">Git repository</a></li> | |||
<li class="dropdown-item" value="zenodo"><a href="#">Zenodo DOI</a></li> | |||
<li class="dropdown-item" value="figshare"><a href="#">Figshare DOI</a></li> | |||
<li class="dropdown-item" value="hydroshare"><a href="#">Hydroshare resource</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure about the wording here, because for Zenodo and Fishare, we also support plain Zenodo URLs (they are just patched through), and it seems the Hydroshare support in repo2docker also supports DOIs...
@betatim What wording should BinderHub use here? Favour "DOI" or "resource" (or "record"). I'd tend towards only talking about DOIs externally, but not failing when direct resource URLs are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used resource because it is a HydroShare term. I'm fine highlighting the DOI, but I do think it'd be good to point to a resource id as well, because resources don't have a functional DOI until they publish the resource. Let me know what you'd like, and I can update it in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if there's a provider-specific wording, we can just use that since a user familiar with it will be expecting that word (as long as it's not too long or complex)
pinging @betatim and @choldgraf. Anything left to do here before merging? |
I can try to take a pass at this something this week. I'm a bit unsure of the details for hydroshare's API etc, so I'll have to take y'alls word on this. @nuest do you think this one is good to go? |
This looks good to me - I'd like to get merged, and then have this PR add some documentation about the HydroShare provider, before merging. Sounds good? |
I'll be happy to update documentation once that PR is merged. Just let me know when I can pull and I'll add a commit for HydroShare. It looks like entries will need to be added only for doc/developer/repoproviders.rst and doc/reference/repoproviders.rst |
OK, we just got #1017 merged, so can you follow the instructions here: In particular the bit about documenting this repoprovider, and then we can merge? cc @sblack-usu |
The repo2docker PR is still outstanding so that needs taking care of. |
I think this is not ready to merge. binderhub/binderhub/builder.py Lines 258 to 259 in 089702b
I think same applies for DataverseProvider PR. |
Those look like they were added 10 days ago, they were not part of the RepoProvider class when I submitted. I'll go ahead and see about implementing them along with adding the new documentation today. |
|
||
async def get_resolved_ref_url(self): | ||
# Hydroshare does not provide a history, resolves to repo url | ||
return self.get_repo_url() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_resolved_spec and get_resolved_ref_url were two methods that I've implemented since the original review. With what I gleaned from the documentation, resolving to get_repo_url() made sense for HydroShare. If this is incorrect, I'll need suggestions to guide a different implementation.
I've resolved the merge conflicts. I'll be testing the newly merged code with a full deploy of this branch and the repo2docker changes tomorrow. |
Great - let us know how it goes! |
Sorry about taking a little longer to get around to testing a full deploy of this. We got it deployed and tested today. It's ready to be merged along with the repo2docker changes. |
ok these are both merged - I guess now we wait for this to make it into mybinder.org and see how hydroshare repos work? |
Sounds good @choldgraf and thanks for merging. I did a deploy and tested the hydroshare repo on google cloud on Friday, so I expect mybinder.org to work fine. Let me know when it's deployed and I can run some manual tests. |
OK I just merged into mybinder.org, so give it a whirl after a few minutes when it is done deploying |
Thanks @sblack-usu for sticking with this one! |
I was able to build from a hydroshare resource, thanks for pushing this through. |
hooray! 🎉🎉🎉🎉 |
repo2docker changes:
jupyterhub/repo2docker#800