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

Adopt binderhub #83

Merged
merged 20 commits into from
May 15, 2024
Merged

Adopt binderhub #83

merged 20 commits into from
May 15, 2024

Conversation

trungleduc
Copy link
Collaborator

@trungleduc trungleduc commented Apr 5, 2024

User facing changes

This PR allows tljh-repo2docker to use binderhub service as the image build backend. By default, it still uses the local docker engine to build image via repo2docker, but if the binderhub_url is specified, it will switch to using binderhub service instead.

tljh

New configurations

  • binderhub_url: The optional URL of the binderhub service. If available, tljh-repo2docker will use this service to build images.
  • db_url: The database's connection string. tljh-repo2docker needs a database to store the image metadata. By default, it will create a sqlite database in the starting directory of the service. To use other databases (PostgreSQL or MySQL), users need to specify the connection string via this config and install the additional drivers (asyncpg or aiomysql).

New environment dialog when using binderhub service

environment-dialog

Note

The binderhub build backend does not support adding custom build arguments and private repository credentials from the interface.

Code changes

  • New database migration and management tools.
  • New handler to build images via binderhub service.
  • New handler to follow the build process.
  • Updated interfaces for building images.
  • New unit tests and UI tests for binderhub build backend.

@jtpio jtpio linked an issue Apr 10, 2024 that may be closed by this pull request
@trungleduc trungleduc force-pushed the binderhub branch 2 times, most recently from dab4e88 to ec22d1f Compare April 12, 2024 09:59
@trungleduc trungleduc force-pushed the binderhub branch 2 times, most recently from 331bc3b to 4e02472 Compare April 15, 2024 09:55
@trungleduc trungleduc added the enhancement New feature or request label Apr 16, 2024
@trungleduc trungleduc requested a review from jtpio April 16, 2024 23:48
@trungleduc trungleduc marked this pull request as ready for review April 16, 2024 23:48
@jtpio
Copy link
Member

jtpio commented Apr 17, 2024

Nice, thanks @trungleduc for working on this!

Also cc @yuvipanda if you would like to help review and / or provide feedback. This should help with 2i2c-org/binderhub-service#78.


On GitHub and GitLab, a user might have to first create an access token with `read` access to use as the password:

![image](https://user-images.githubusercontent.com/591645/107350843-39c3bf80-6aca-11eb-8b82-6fa95ba4c7e4.png)

> [!NOTE]
> The `binderhub` build backend does not support configuring private repositories credentials from the interface.
Copy link
Member

Choose a reason for hiding this comment

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

Should we open a new issue (or comment in #72) to keep track of the current issues with BinderHub and the default git provider?

@yuvipanda
Copy link

This is super exciting to me. How can I help? Would it be via code review? Or something else?

@trungleduc
Copy link
Collaborator Author

Hi @yuvipanda, a quick look at the implementation would be super helpful for us.

c.JupyterHub.hub_ip = public_ips()[0]
c.JupyterHub.cleanup_servers = False
c.JupyterHub.spawner_class = Repo2DockerSpawner
if hookimpl:
Copy link
Member

@jtpio jtpio May 15, 2024

Choose a reason for hiding this comment

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

Maybe we should document why we have to conditionally define the TLJH hooks?

Probably to allow for the package to be used with the BinderHub chart on Kubernetes? If so, maybe the tljh_repo2docker should continue to focus on the TLJH integration (be a TLJH plugin) as before, and have a separate package for use with BinderHub?

Copy link
Member

Choose a reason for hiding this comment

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

This could also likely be done separately, as the diff for this PR is already quite large.

Also the UI for building environments could eventually be its own jupyterhub-environment-builder package, and have tljh_repo2docker depend on it.

Copy link
Member

Choose a reason for hiding this comment

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

Moved to #84

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks @trungleduc, this is great!

I left a small comment to discuss how we would like to architecture the different packages in the long run, if in case this needs a bit more thinking.

Also it looks like it will be easier to test this new feature if the PR is merged and available in a release (which could be a pre-release). So the best for now would probably be to move forward with this, make a new release, and iterate on fixes and adjustments if needed in follow-up PRs.

@jtpio jtpio merged commit 6b144d2 into plasmabio:master May 15, 2024
8 checks passed
@jtpio
Copy link
Member

jtpio commented May 15, 2024

Also @yuvipanda if you would like to start testing this on some deployments and have some feedback or issues, please don't hesitate to reach out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use binderhub as the build backend
3 participants