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 Figshare to UI #951

Merged
merged 1 commit into from
Sep 22, 2019
Merged

[MRG] Add Figshare to UI #951

merged 1 commit into from
Sep 22, 2019

Conversation

nuest
Copy link
Contributor

@nuest nuest commented Sep 12, 2019

! This depends on an unreleased version of repo2docker !

This closes #938.

But it's not ready yet :-) Just would like to get some feedback :-), and need help:

With the UI, I get a /master at the end of the URL that I don't want:

image

http://localhost:8585/v2/figshare/10.6084/m9.figshare.9782777.v1/master

Where does that get added? I can't find it :-/.

OTOH, the local test URL without /master

http://localhost:8585/v2/figshare/10.6084/m9.figshare.9782777.v2

gives me "Waiting for build to start" and in the logs there is just

[..]
[D 190912 13:05:18 build:94] 2 build pods
[D 190912 13:05:18 build:141] Build phase summary: {
     "Pending": 2
    }

For several minutes now... shouldn't there be more logs?


Related questions

  • With
python3 -m pip install -e .
./testing/minikube/install-hub

I should have the latest code running in my local kube, right?

  • Is it worth writing down in the docs all the places where something has to be added for a new provider? Though this list might be outdated with a new UI:
    • binderhub/main.py
      • SPEC_NAMES
      • from .repoproviders import
    • repo_providers = Dict(
    • binderhub/event-schemas/launch.json (though unsure what that is for)
    • binderhub/repoproviders.py the actual class FigshareProvider(RepoProvider):
    • binderhub/static/js/index.js the example text
    • binderhub/templates/index.html the item in the dropdown
  • Have you considered generating some bits, e.g. build repro_providers at runtime by asking all the imported instances of RepoProvidder how they want to be added to that list ?
  • Is it worth attempting to generate the UI parts from a service endpoint for the next ()?
  • Is is possible to run the BinderHub outside of Kubernetes to debug the code?

@betatim
Copy link
Member

betatim commented Sep 12, 2019

To get the latest version (or a particular version) of repo2docker you need to specify it. The BinderHub uses (I think) the latest released version.

binderhub/binderhub/app.py

Lines 359 to 365 in 3fe8143

build_image = Unicode(
'jupyter/repo2docker:0.8.0',
help="""
The repo2docker image to be used for doing builds
""",
config=True
)
to see the related traitlet code.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@betatim
Copy link
Member

betatim commented Sep 12, 2019

I think https://github.com/jupyterhub/binderhub/blob/master/binderhub/static/js/index.js#L109-L112 holds the secret to the mysterious trailing master. Did I mention there is "a bit of code duplication and having to change stuff in multiple places"? :-/

@nuest
Copy link
Contributor Author

nuest commented Sep 16, 2019

"Mysterious trailing master" removed, see also #953.

Works now locally for me, ready for review, though this relies on a new repo2docker version, so not sure what the process is then.


Regarding the build stuck at "Waiting for build to start...", the minikube dashboard shows me

image

The error message "Failed to pull image "jupyter-repo2docker:figshare": rpc error: code = Unknown desc = Error response from daemon: pull access denied for jupyter-repo2docker, repository does not exist or may require 'docker login'" helped me so far as to identify the issue a bit further:

  • imagePullPolicy: IfNotPresent is configured in /binderhub/helm-chart/binderhub/templates/deployment.yaml
  • there seems to be a problem with my local r2d image I want to use for testing
  • solution: I have to build my repo2docker image using the Docker daemon of minikube

@nuest nuest changed the title [WIP] Add Figshare to UI [MRG] Add Figshare to UI Sep 16, 2019
@betatim
Copy link
Member

betatim commented Sep 19, 2019

To get the tests to pass this needs rebasing onto the latest commit from master. Required git-fu:

git checkout master
git fetch upstream
git merge upstream/master
git checkout <thisbranch>
git rebase master
git push -f

doc/federation/data-federation.txt
.vscode/
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm not wild about having editor specific ignores. Mostly because I fear we end up with many many different (poorly maintained) ignores for all the different editors people use. I think it is nicer if each of us has their own "global" gitignore a la https://help.github.com/en/articles/ignoring-files#create-a-global-gitignore

@betatim
Copy link
Member

betatim commented Sep 22, 2019

I've rebased this on master, if the tests pass this can be merged.

@nuest
Copy link
Contributor Author

nuest commented Sep 22, 2019

You beat me to it!

@betatim betatim merged commit 31ab3c7 into jupyterhub:master Sep 22, 2019
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Sep 22, 2019
@nuest
Copy link
Contributor Author

nuest commented Sep 22, 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.

Add repoprovider for Figshare
3 participants